SwiftGen
SwiftGen copied to clipboard
Fonts: add support for extracting code points
This adds support to the SwiftGen fonts context for extracting code points of a font to make them available to the template. This is useful for generating templates for icon fonts. I am doing this work to modernize Iconic which until this point used a old, modified, Swift 2.3 only version of SwiftGen which meant that recent users have been unable to update their icon fonts. I have adapted the Iconic SwiftGen template but have not submitted it as part of this PR because I was unsure if it was out of scope. If you don't think it will be I will be happy to submit it. I quickly put it up as a Gist if you want to take a look.
Related PR (required to run the linked template, not required for this PR to be merged): https://github.com/SwiftGen/StencilSwiftKit/pull/121
- [X] I've started my branch from the
developbranch (gitflow)- [X] And I've selected
developas the "base" branch for this Pull Request I'm about to create
- [X] And I've selected
- [X] I've added an entry in the
CHANGELOG.mdfile to explain my changes and credit myself
(or added#trivialto the PR title if I think it doesn't warrant a mention in the CHANGELOG)- [X] Add the entry in the appropriate section (Bug Fix / New Feature / Breaking Change / Internal Change)
- [X] Add a period and 2 spaces at the end of your short entry description
- [X] Add links to your GitHub profile to credit yourself and to the related PR and issue after your description
~~Seems Circle is broken on the branch. I did have to remove the opt in rule lower_acl_than_parent from the SwiftLint config to get things building locally. All the ACL issues were in files that I didn't touch though.~~
Thanks for the PR (and sorry for the late reply)
Could you update the contexts files with this new code so we can see how the updated YAML contexts look like, and also so that the unit tests are updated accordingly? It's just a matter of using Cmd-U on the "SwiftGenKit - Generate Contexts" scheme in Xcode
I have two main concerns with this change:
- [ ] If I understand the code correctly, it will generate a HUGE list of names for standard fonts. For fonts that are limited to only a couple of glyphs, like "icon fonts" that might be OK. But for a normal font that contains tons of glyphs for almost all possible codepoints in unicode, that is gonna generate a HUGE list. We likely don't want that for most fonts
- [ ] I tried to re-generate the SwiftGenKit contexts (by running the scheme as pointed above) to run the unit tests and it crashes. This is because for regular fonts like the ones we have in our unit tests, like
.SFNSDisplay-Regularfor example, there are multiple glyphs with the same name (in my tests it crashes withFatal error: Duplicate values for key: 'space')
So in the end I'm not sure if this makes sense to generate those lists. I understand the use case, but that won't work for regular fonts though. Maybe we could make use of parser options so that the user can specify when they want those list of glyph names generated for which fonts, and default to not generate them? That way users can opt-in for this behaviour only for their icon-fonts and avoid the list of glyphs for regular fonts?
Ideally we'd have a mechanism for lazily calculating certain context properties (+ cache, so lazy var?), such as the "code points" in this PR. But that doesn't really work with how our contexts are generated right now (plain dictionaries), and how we serialize them for testing purposes.
| 1 Warning | |
|---|---|
| :warning: | Big PR |
Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖
Thanks a lot for your contribution!
Seems like everything is in order 👍 You did a good job here! 🤝
Generated by :no_entry_sign: Danger
@AliSoftware I've gone ahead and added a option for extracting code points. Not sure what I need to do in the tests to get it actually testing though.
Never mind, all figured out! Think this is good to go?
Now that I'm actually integrating this, I've realized that this can't yet be merged. For one thing, the option values need to be String so it works with CLI. But now I'm facing this issue and unsure how to get past it. Just pushed the font that has this code point at 7132bca11afd8784766b8694817bf8213f97a384. Suggestions welcome!
let point: UInt32 = 0xF0000
let scal = Unicode.Scalar(point)!
let char = UniChar(scal.value) // Fatal error: Not enough bits to represent the passed value
@robbiet480 Not sure what you're trying to do there, but that code point 0xF0000 does not fit inside an Int16 (which UniChar is a typealias of).
Yeah its a codepoint that came out of the test icon font I'm using, MaterialDesignIcons. Guess they have so many icons the code points go out of UInt16 space?
You can write a unicode character in a Swift string like this:
let dollarSign = "\u{24}" // $, Unicode scalar U+0024
let blackHeart = "\u{2665}" // ♥, Unicode scalar U+2665
let sparklingHeart = "\u{1F496}" // 💖, Unicode scalar U+1F496
Yeah what you probably want is to convert your Unicode.Scalar into a Character, not a UniChar
https://developer.apple.com/documentation/swift/character/2906502-init
in Unicode, code points are not limited to 2^16 so it's not logical to constrain them to UInt16. That's why we use Character in Swift when manipulating strings, and not UniChar which are more a relic of the past from C before full-unicode compliance
@AliSoftware That's hopefully the pointer I needed, thanks!!!
Okay, think it's now all good.
Also, you mention in the PR description that you have a template that includes those glyph names but kept it as a gist and didn't include it in the PR, but I think it might be worth it to make some use of it.
I think we should either:
- create a separate template eg
iconfont-swift5.stencilthat exposes those icons and glyph names in the generated code - or modify the existing templates so that they accept an
includeGlyphNamesparameter, or even better, maybe we don't need the explicit parameter and can just do something like{% if icons.empty %}// List of glyphs not generated. If this is an icon font and you want the list of glyphs there, use "option: glyphNames" as an option for the parser in your config file{% else %}let glyphNames = …code to generate the list{% endif %} - or keep the template as a gist but mention the URL to the gist somewhere in the documentation. But at that point I think we would be better off including the template in the built in templates
So I'd vote for 1 or 2, using solution 2 only if it's a small and localized change to the existing templates that won't make them too convoluted or to hard to maintain. Otherwise or if the code for icon fonts makes more sense to be very specific and different from normal fonts, go with 1 and have a dedicated template for them, which could actually make more sense in practice and would be easier to do given you already have a gist for it.
(Note that we could keep that addition of your template for icon fonts as a separate PR if you prefer to keep things separate and easier to review)
So… I tested the code with SF-Text-Pro-Regular.otf (SFSymbols) and I've managed to make it list the glyph names but only the ones for the common textual characters. It won't output any of the SFSymbol names like, say, camera.fill or similar… worth investigating why and if there's a solution for that.
I've actually managed to get the unicode code point of one of the SFSymbols (drag & drop one of the icon from the SF Symbols macOS app into a new file in a text editor, save in utf16, use xxd to show the utf16 hex values) but when I try to call CTFontGetGlyphsForCharacters with that surrogate pair of utf16 code units, it fails (the method returns false). So no idea how we could get a CGGlyph for those characters. And cgFont.numberOfGlyphs returns 7645 which is the number of case that the current code generates, which means that this cgFont.numberOfGlyphs doesn't take the SFSymbol glyphs present in the SFSymbol font into account anyway… 🤔