Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Font Awesome 6.7.1 #62

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Conversation

pepijnve
Copy link
Contributor

An attempt to update to the Font Awesome 6.7.0 icon set

@jessedoyle
Copy link
Owner

Hey @pepijnve, thanks for the contribution! At a high level everything appears to be in order (glad to see you figured out the tool/converter stuff).

Just going to do some due diligence before merging this upstream. Expect a turnaround time around ~1 week. Thanks!

@pepijnve
Copy link
Contributor Author

@jessedoyle thanks for the quick response. Feel free to double check everything of course. Can't be too careful with binary blobs.

Here's a summary of the steps I did:

  • Downloaded the Font Awesome 6.7.0 free web bundle
  • Replaced the .tff files with the new versions from the web bundle
  • Replaced the license files with the current version from the web bundle
  • Ran the icons.yml converter tool as is, but noticed a number of names seemed to have gone missing
  • Checked the contents of icons.yml, noticed the aliases entries, and then added support for those to the converter
  • Sorted the icon names before writing the icon data file to get an easier to review diff
  • Ran the specs and simply adjusted the expected values. I did not do a detailed investigation regarding why the output value is different.

@pepijnve
Copy link
Contributor Author

I should add that I'm still in the process of testing this myself (indirectly through asciidoctor). At the moment I'm still getting errors that I'm in the process of investigating

asciidoctor: ERROR: cannot fit formatted text on page: <unicode for pen-to-square>
asciidoctor: ERROR: cannot fit formatted text on page: <unicode for lightbulb>

Also remove explicit installation of Bundler.
setup-ruby already handles this
@pepijnve
Copy link
Contributor Author

Findings so far are that there seems to be a slight difference in the font metrics in the TTF fonts across the versions. I tried using the dekstop OTFs instead, but those did not work at all. Prawn seemed to process them without any issues and the resulting PDF seemed to have them embedded, but the symbols did not actually render correctly.

Continuing with the TTF investigation...

@pepijnve
Copy link
Contributor Author

I was able to figure it out. Long story short due to differences in font metrics when a 23.8pt sized icon (for instance) is requested, the actual size based on the font metrics is 24.8pt. If an icon is rendered inline with text and the line height is restricted this can cause the icon to exceed the allow line height and not get rendered. This is probably more an asciidoctor-pdf problem than a prawn-icon one.

I'm going to add a local workaround that uses Prawn's :overflow => :shrink_to_fit functionality to work around the problem.

* Update all font files + legends from version 6.7.0 to 6.7.1
@jessedoyle
Copy link
Owner

Okay, circling back to this! It looks like the latest version is 6.7.1, so just FYI that I'll be pushing a few commits to your branch to pull in the font files for this version 👍

@jessedoyle jessedoyle changed the title Update to Font Awesome 6.7.0 Update to Font Awesome 6.7.1 Nov 26, 2024
Previously versions of Prawn < 2.5.0 mishandled unicode characters
and would truncate codepoints greater than \xFFFF. This impacts the
material design icons and the related specs.

This commit updates the test case to correctly look for the truncated
value on older Prawn versions (which is expected behaviour at this
point).

see: prawnpdf/prawn#1327 (comment)
Copy link
Owner

@jessedoyle jessedoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I pushed a few commits to:

  • Update fonts to the latest version of 6.7.1
  • Fix a failing spec

I'm also going to push a commit to do some housekeeping for the next release while we're at it. Thanks again for the contribution!

.github/workflows/ci.yml Show resolved Hide resolved
spec/integration/icon_spec.rb Outdated Show resolved Hide resolved
tool/fontawesome/converter.rb Show resolved Hide resolved
* Bump the compatibility layer warning to mention a non-specific
  future version.
* Bump version to 4.0
* Update the changelog to include release notes (thanks @pepijnve)
Copy link
Owner

@jessedoyle jessedoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contribution!

@jessedoyle jessedoyle merged commit 3e45255 into jessedoyle:master Nov 26, 2024
4 checks passed
@jessedoyle
Copy link
Owner

This has been released in prawn-icon version 4.0.0. Thanks!

@pepijnve
Copy link
Contributor Author

@jessedoyle I've been talking to the asciidoctor-pdf maintainer in the meantime about the sizing issue. At the moment prawn-icon passes the icon size parameter down to prawn as a font size. Due to differences in the Font Awesome truetype fonts this doesn't produce the same result with the v5 font vs the v6 font. I wrote some code to visualize what's going on that you can see below. Given font size 24 points, the actually rendered size of the same glyph is exactly 24 points in v5, but 25 and a bit in v6.

I was wondering if it would be a good idea to do the necessary calculations in prawn-icon to compute the font size value that results in the icon being rendered at the requested size. If you think that's a good idea I can make a PR for that as well.

@jessedoyle
Copy link
Owner

Hey @pepijnve!

My first thought is that is that the difference in result between v5 and v6 isn't something that Prawn::Icon should attempt to correct for. We're using the off-the-shelf fonts from the v6 bundle, and the same results should be apparent if those fonts were used in any other medium.

The major versions of both prawn-icon and font-awesome were bumped - so one should expect visual differences when updating.

With that being said, I think there's an argument to be made to provide a simple correction constant (or equivalent) for compatibility purposes. Is that in line with what you were thinking?

@jessedoyle
Copy link
Owner

Probably could have articulated that a bit better:

What I'm hoping to avoid is a scenario where prawn-icon is responsible for maintaining pixel-perfect rendering compatibility between multiple major releases of a particular font family. We can provide helpers/tools to enable folks on their upgrade path between a single major release though (so long as they're low complexity).

@pepijnve
Copy link
Contributor Author

What I had in mind was an option that modifies how the size option is interpreted. Currently, and that would remain the default, it's the font size. This can result in an icon that's larger than size in the PDF. The alternative would be to interpret size as the icon size. In that case you would derive the font size from the metrics in the font and the resulting icon would fit nicely in a size x size bounding box.

@jessedoyle
Copy link
Owner

jessedoyle commented Nov 30, 2024

What I had in mind was an option that modifies how the size option is interpreted.

I'd entertain a PR that implemented this with the following criteria:

  • The compatibility layer is fully opt-in
  • The implementation is low-complexity and straightforward to maintain
  • The compatibility layer may be removed in a future major release (as necessary)

Happy to offer support, but definitely think this falls into the "I need to see it before accepting it" category.

@pepijnve
Copy link
Contributor Author

That's fair. I'll make a first draft PR to make the discussion a bit more concrete and we can take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants