Fonts: add support for extracting code points #638

robbiet480 · 2019-05-15T04:09:41Z

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): SwiftGen/StencilSwiftKit#121

I've started my branch from the develop branch (gitflow)
- And I've selected develop as the "base" branch for this Pull Request I'm about to create
I've added an entry in the CHANGELOG.md file to explain my changes and credit myself
(or added #trivial to the PR title if I think it doesn't warrant a mention in the CHANGELOG)
- Add the entry in the appropriate section (Bug Fix / New Feature / Breaking Change / Internal Change)
- Add a period and 2 spaces at the end of your short entry description
- Add links to your GitHub profile to credit yourself and to the related PR and issue after your description

robbiet480 · 2019-05-15T04:22:28Z

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.

AliSoftware · 2020-05-17T14:19:55Z

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-Regular for example, there are multiple glyphs with the same name (in my tests it crashes with Fatal 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?

djbe · 2020-05-17T20:01:54Z

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.

…ful for icon fonts

SwiftGen-Eve · 2020-06-11T05:30:19Z

	1 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 🚫 Danger

robbiet480 · 2020-06-11T05:41:38Z

@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.

robbiet480 · 2020-06-11T05:53:54Z

Never mind, all figured out! Think this is good to go?

robbiet480 · 2020-06-11T08:26:06Z

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 7132bca. 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

djbe · 2020-06-11T19:44:01Z

@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).

robbiet480 · 2020-06-11T19:46:07Z

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?

djbe · 2020-06-11T19:46:08Z

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

AliSoftware · 2020-06-11T19:46:32Z

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

robbiet480 · 2020-06-11T20:01:59Z

@AliSoftware That's hopefully the pointer I needed, thanks!!!

robbiet480 · 2020-06-11T23:19:07Z

Okay, think it's now all good.

AliSoftware

Quick review (from phone so might have missed things?) of those latest changes, looking good!

Left a couple of suggestions, and one or two interrogations about things we might want to just conform to be sure, but otherwise LGTM 👍

AliSoftware · 2020-06-11T23:27:20Z

CHANGELOG.md

@@ -53,6 +53,9 @@
  [#609](https://github.com/SwiftGen/SwiftGen/issue/609)
  [#593](https://github.com/SwiftGen/SwiftGen/pull/593)
  [#610](https://github.com/SwiftGen/SwiftGen/pull/610)
+* Fonts: extract all codepoints of a font and make available to templates. Useful for icon fonts.  


Worth adjusting that CHANGELOG entry to reflect that this is opt-in

Would also rather mention that it extract "glyph names" rather than talking about codepoints, as glyph names are what are more relevant to users here

AliSoftware · 2020-06-11T23:30:14Z

Sources/SwiftGenKit/Parsers/Fonts/CTFont.swift

    return descRefs.compactMap { desc -> Fonts.Font? in
      let font = CTFontCreateWithFontDescriptorAndOptions(desc, 0.0, nil, [.preventAutoActivation])
      let postScriptName = CTFontCopyPostScriptName(font) as String
      guard let familyName = CTFontCopyAttribute(font, kCTFontFamilyNameAttribute) as? String,
        let style = CTFontCopyAttribute(font, kCTFontStyleNameAttribute) as? String else { return nil }

      let relPath = parent.flatMap { file.relative(to: $0) } ?? file
+
+      let characterSet = CTFontCopyCharacterSet(font) as CharacterSet
+      let cgFont = CTFontCopyGraphicsFont(font, nil)


Why not put these 2 inside the if extractCodePoints?
That might not take that long to run (though maybe that does depending on the font file size?) but better avoid needless calls when the flag is off 🙃

Also, not completely sure because maybe the bridging of that API to Swift takes care of that, but worth double-checking: since those two calls are …Copy… don't they need a balancing CTRelease (or CFRelease?) call or similar to avoid leaks?

AliSoftware · 2020-06-11T23:36:32Z

Sources/SwiftGenKit/Parsers/Fonts/CTFont.swift

+        for unicode in characterSet.codePoints {
+          guard let unicodeScalar = UnicodeScalar(unicode) else { continue }
+
+          let utf16codepoints = Array(Character(unicodeScalar).utf16)


Wondering if there isn't a more efficient way to go from codepoint to utf16 than CodePoint>UnicodeScalar>Character>.utf16 🤔
Will have to look at the various String/Character/Unicode APIs to double check, as this is often tricky stuff with edge cases so better be sure.

Actually there is, and it's more correct: Array(unicodeScalar.utf16)

AliSoftware · 2020-06-11T23:37:32Z

Sources/SwiftGenKit/Parsers/Fonts/CTFont.swift

+
+          // Gets the Glyph
+          guard CTFontGetGlyphsForCharacters(font, utf16codepoints, &glyphs, utf16codepoints.count) == true else {
+            print("Failed to get glyph for character", unicode, utf16codepoints, unicodeScalar)


We have a logMessage function to generate warnings and errors during parsing, you should use that instead of print here

AliSoftware · 2020-06-11T23:37:43Z

Sources/SwiftGenKit/Parsers/Fonts/CTFont.swift

+          }
+
+          if glyphs.isEmpty {
+            print("Glyph set was empty!")


Ditto re:logMessage

AliSoftware · 2020-06-11T23:43:26Z

Sources/SwiftGenKit/Parsers/Fonts/FontsParser.swift

+    static let codePoints = ParserOption(
+      key: "codePoints",
+      defaultValue: "false",
+      help: "If enabled, will extract code points from the font. Only useful for icon fonts."


Suggested change

help: "If enabled, will extract code points from the font. Only useful for icon fonts."

help: "If enabled, will extract the glyph names from the font. Only useful for icon fonts."

AliSoftware · 2020-06-11T23:44:24Z

Sources/SwiftGenKit/Parsers/Fonts/FontsParser.swift

@@ -9,6 +9,14 @@ import Foundation
 import PathKit

 public enum Fonts {
+  public enum Option {
+    static let codePoints = ParserOption(


Maybe name the option glyphNames instead of codePoints?

Also, this parser option needs to be documented in Documentation/Parsers/fonts.md in the "Supported Options" section (you might need to merge develop into your branch first, as we renamed that documentation folder in a recent PR that was open and merged after you opened yours)

AliSoftware · 2020-06-11T23:47:35Z

Sources/SwiftGenKit/Parsers/Fonts/FontsParser.swift

@@ -32,7 +41,13 @@ public enum Fonts {
        return
      }

-      let fonts = CTFont.parse(file: path, relativeTo: parent)
+      let getCodePoints: Bool = self.options[Option.codePoints] == "true"


We need to check how we do string-to-boolean for other config flags. I don't think we have many Boolean Parser Options that we can use as reference but we do have some template parameters that are Boolean (e.g. forceNameSpaces for the Assets templates); if there is similar cases, better ensure we do the conversion the same way (== "true" vs != "false", case-insensitivity, etc)

Might actually be a nice addition to add to ParserOptionValues by overloading the subscript si that if the return type is inverted to Bool, that ParserOptionValues is the one doing the string-to-Bool conversion. That would allow us to ensure that all future Boolean parser options are treated the same way.

AliSoftware · 2020-06-11T23:50:10Z

Sources/SwiftGenKit/Stencil/FontsContext.swift

-              "style": font.style
+              "style": font.style,
+              // swiftlint:disable:next force_unwrapping
+              "icons": font.icons.map { ["name": $0.key, "unicode": $0.value] }.sorted { $0["name"]! < $1["name"]! }


Even if the force-unwrap is technically never supposed to be nil in that specific case, you could avoid the SwiftLint violation using

($0["name"] ?? "") < ($1["name"] ?? "")

AliSoftware · 2020-06-11T23:53:54Z

Sources/SwiftGenKit/Stencil/FontsContext.swift

-              "style": font.style
+              "style": font.style,
+              // swiftlint:disable:next force_unwrapping
+              "icons": font.icons.map { ["name": $0.key, "unicode": $0.value] }.sorted { $0["name"]! < $1["name"]! }


You'll need to document that additional key in the documentation for contexts (in Documentation/StencilSwiftKit Contexts/fonts.md)

AliSoftware · 2020-06-12T00:05:47Z

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.stencil that exposes those icons and glyph names in the generated code
or modify the existing templates so that they accept an includeGlyphNames parameter, 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)

Once SwiftGen/SwiftGen#638 lands, we can update Iconic to use a newer version and everything becomes better. Fixes #663.

AliSoftware · 2020-06-24T23:33:07Z

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… 🤔

This was referenced May 15, 2019

Swift 4.2 and quality of life improvements home-assistant/Iconic#82

Merged

Add a new filter to output a unicode codepoint SwiftGen/StencilSwiftKit#121

Open

AliSoftware added status: awaiting input type: enhancement labels May 17, 2020

djbe added this to the 6.3.0 milestone May 17, 2020

Add support for extracting font code points for use in templates. Use…

9b62847

…ful for icon fonts

robbiet480 force-pushed the feature/icon-font-support branch from 20e76d5 to 9b62847 Compare June 11, 2020 05:25

Put code point extraction behind a option

cd9790a

robbiet480 mentioned this pull request Jun 11, 2020

Fix building MaterialDesignIcons home-assistant/iOS#602

Merged

Add test for code point extraction option

ef703ea

robbiet480 force-pushed the feature/icon-font-support branch from 76afd6c to ef703ea Compare June 11, 2020 06:27

robbiet480 added 2 commits June 11, 2020 01:29

Change option type to string so it works on CLI

8dffcb5

Add breaking font

7132bca

Fix glyph extraction to support 32bit code points

3b3ff33

AliSoftware self-requested a review June 11, 2020 23:25

AliSoftware reviewed Jun 11, 2020

View reviewed changes

zacwest added a commit to home-assistant/iOS that referenced this pull request Jun 17, 2020

Replace generated MDI.swift with a manual one (temporarily)

92391ce

Once SwiftGen/SwiftGen#638 lands, we can update Iconic to use a newer version and everything becomes better. Fixes #663.

zacwest mentioned this pull request Jun 17, 2020

Replace generated MDI.swift with a manual one (temporarily) home-assistant/iOS#667

Merged

zacwest added a commit to home-assistant/iOS that referenced this pull request Jun 17, 2020

Replace generated MDI.swift with a manual one (temporarily) (#667)

7efb95e

Once SwiftGen/SwiftGen#638 lands, we can update Iconic to use a newer version and everything becomes better. Fixes #663.

djbe modified the milestones: 6.3.0, 6.4.0 Jul 6, 2020

djbe mentioned this pull request Aug 2, 2020

Support System Images #756

Closed

djbe changed the title ~~Add support for extracting font code points for use in templates.~~ Fonts: add support for extracting code points Oct 4, 2020

djbe modified the milestones: 6.5.0, Next minor (new features) Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fonts: add support for extracting code points #638

Fonts: add support for extracting code points #638

robbiet480 commented May 15, 2019 •

edited

robbiet480 commented May 15, 2019 •

edited

AliSoftware commented May 17, 2020

djbe commented May 17, 2020

SwiftGen-Eve commented Jun 11, 2020 •

edited

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020 •

edited

djbe commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

djbe commented Jun 11, 2020

AliSoftware commented Jun 11, 2020 •

edited

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

AliSoftware left a comment •

edited

AliSoftware Jun 11, 2020 •

edited

AliSoftware Jun 11, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 24, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 11, 2020 •

edited

AliSoftware Jun 11, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 12, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 12, 2020

AliSoftware Jun 11, 2020

AliSoftware Jun 11, 2020

AliSoftware commented Jun 12, 2020 •

edited

AliSoftware commented Jun 24, 2020

	help: "If enabled, will extract code points from the font. Only useful for icon fonts."
	help: "If enabled, will extract the glyph names from the font. Only useful for icon fonts."

Fonts: add support for extracting code points #638

Are you sure you want to change the base?

Fonts: add support for extracting code points #638

Conversation

robbiet480 commented May 15, 2019 • edited

robbiet480 commented May 15, 2019 • edited

AliSoftware commented May 17, 2020

djbe commented May 17, 2020

SwiftGen-Eve commented Jun 11, 2020 • edited

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020 • edited

djbe commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

djbe commented Jun 11, 2020

AliSoftware commented Jun 11, 2020 • edited

robbiet480 commented Jun 11, 2020

robbiet480 commented Jun 11, 2020

AliSoftware left a comment • edited

Choose a reason for hiding this comment

AliSoftware Jun 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Jun 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Jun 12, 2020 • edited

AliSoftware commented Jun 24, 2020

robbiet480 commented May 15, 2019 •

edited

robbiet480 commented May 15, 2019 •

edited

SwiftGen-Eve commented Jun 11, 2020 •

edited

robbiet480 commented Jun 11, 2020 •

edited

AliSoftware commented Jun 11, 2020 •

edited

AliSoftware left a comment •

edited

AliSoftware Jun 11, 2020 •

edited

AliSoftware Jun 11, 2020 •

edited

AliSoftware commented Jun 12, 2020 •

edited