Adding new symbols

[StevenSorial]

Adding new symbols from 2.2, 3.0, and 3.1 versions of SFSymbols

[fredpi]

@Stevenmagdy Thank you very much for the PR!

I just spent a quick look on this and wanted to share my observations:

• I've noticed that you often just amend new changes to the previous commit and perform a force push afterwards. For the sake of comprehensibility I would suggest to rather keep changes in individual commits (also allowing to revert a specific change more easily). Would that be okay for you?
• The CI was failing due to a misconfiguration, but that is fixed now, meaning the CI passes.
• In the CI, there are some warnings about if #available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.0, *) warnings not being needed, because the availability is already ensured by the outer context. Could you make it so that no availability checks are added to the generated code if availability is already ensured via the outer scope?
• If I understand it correctly, you have changed SFSymbol+CaseIterable.swift, optimizing for a smaller file size. However, with the new implementation, computation power (array composition & sorting) is always needed when SFSymbol.allCases is accessed and I don't believe that's a good library design. I would rather go with the previous variant, where the full allCases array is specified in its entirety for each iOS version. To reduce file size, we may add each allCases array to the respective Symbols file. I'm just wondering whether it would be possible to do that in Swift? Maybe with a helper variable?
• Some symbol previews in SFSymbol+3.0.swift and SFSymbol+3.1.swift don't render correctly (i. e. just as a ? in a box), at least on my machine. Is there a way for us to deal with this problem? I assume it's Apple's "fault", but want to make sure.

[StevenSorial]

@fredpi Thanks for the quick review!

• I've noticed that you often just amend new changes to the previous commit and perform a force push afterwards. For the sake of comprehensibility I would suggest to rather keep changes in individual commits (also allowing to revert a specific change more easily). Would that be okay for you?

Agreed.

• In the CI, there are some warnings about if #available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.0, *) warnings not being needed, because the availability is already ensured by the outer context. Could you make it so that no availability checks are added to the generated code if availability is already ensured via the outer scope?

Sure.

• If I understand it correctly, you have changed SFSymbol+CaseIterable.swift, optimizing for a smaller file size. However, with the new implementation, computation power (array composition & sorting) is always needed when SFSymbol.allCases is accessed and I don't believe that's a good library design. I would rather go with the previous variant, where the full allCases array is specified in its entirety for each iOS version. To reduce file size, we may add each allCases array to the respective Symbols file. I'm just wondering whether it would be possible to do that in Swift? Maybe with a helper variable?

Indeed, this change is because the file size exceeded 30k LOC and it was making Xcode freeze. I thought the performance overhead was justified because it only computed once (it's a self-executed closure).

I think we should discuss if we even need CaseIterable conformance because:

• SFSymbol was converted to a struct so CaseIterable makes less sense.
• I can't think of a use case that a user would need to loop over 3000+ symbols
• The array currently can have more than 3 elements for the same symbols. eg. star.lefthalf.fill , star.leadinghalf.fill, and star.leadinghalf.filled are all for the same symbol.

I think we should either remove CaseIterable conformance or just make it a Set instead of Array (CaseIterable is allowed to be any Collection), so at least users would not have O(n) (which is 3000+) performance when looking for a symbol.

• Some symbol previews in SFSymbol+3.0.swift and SFSymbol+3.1.swift don't render correctly (i. e. just as a ? in a box), at least on my machine. Is there a way for us to deal with this problem? I assume it's Apple's "fault", but want to make sure.

That happened for me too but it was fixed automatically 😄 . I thought its because I'm on Big Sur which does not have the new symbols so Xcode was struggling to render them.

[fredpi]

@Stevenmagdy Thanks for the quick response! 😉

Indeed, this change is because the file size exceeded 30k LOC and it was making Xcode freeze. I thought the performance overhead was justified because it only computed once (it's a self-executed closure).

That's a fair point, I didn't see that you implemented it as a self-executed closured. Still, I'm wondering how much time it takes to call SFSymbol.allCases for the first time. Maybe we can do a quick profiling just to make sure it doesn't take too long?

I think we should discuss if we even need CaseIterable conformance because:

• SFSymbol was converted to a struct so CaseIterable makes less sense.
• I can't think of a use case that a user would need to loop over 3000+ symbols
• The array currently can have more than 3 elements for the same symbols. eg. star.lefthalf.fill , star.leadinghalf.fill, and star.leadinghalf.filled are all for the same symbol.

I think we should either remove CaseIterable conformance or just make it a Set instead of Array (CaseIterable is allowed to be any Collection), so at least users would not have O(n) (which is 3000+) performance when looking for a symbol.

Good catch! allCases was primarily introduced to allow testing the library. But after the v2 update, allCases could not be retrieved automatically via the CaseIterable protocol conformance any longer and had to be specified manually, contradicting the idea of reliable automatic testing. I suggest to remove CaseIterable conformance and move the content of allCases (or how we want to call it then; maybe allSymbolsWithAliases?) to the test target. This way, it isn't exposed to the user any longer but remains available to the tests.

Just recently I learned about the use case of randomly picking one symbol: https://twitter.com/MattPfeiffer/status/1435238086716297228 I believe this is a valid use case and we should keep support for it. However, if the array has more than one element for the same symbol, the likelihood is distorted, meaning we should only add the latest symbol name to the array. Also, as you said, Set is probably better than Array for this use case. The new, public variable shall have a name different to allCases – rather something like allSymbols. And the variable should be documented in the code so that it is clear to the user what it provides and what it doesn't provide.

Summarized: Drop CaseIterable conformance, move allCases to the test target and additionally add a new public property allSymbols.

• Some symbol previews in SFSymbol+3.0.swift and SFSymbol+3.1.swift don't render correctly (i. e. just as a ? in a box), at least on my machine. Is there a way for us to deal with this problem? I assume it's Apple's "fault", but want to make sure.

That happened for me too but it was fixed automatically 😄 . I thought its because I'm on Big Sur which does not have the new symbols so Xcode was struggling to render them.

What do you mean by saying it was fixed automatically? Has it been fixed by updating your macOS / Xcode? I'm running Monterey but am still seeing the issue:

[StevenSorial]

Summarized: Drop CaseIterable conformance, move allCases to the test target and additionally add a new public property allSymbols.

if we are going to provide the property (with a different name) anyway, then I think it's not justified enough to drop the conformance and break source compatibility, just for a name change.

I think it should be enough to make it a Set, only provide the latest name of each symbol, and provide documentation explaining what is included in the collection.

What do you mean by saying it was fixed automatically? Has it been fixed by updating your macOS / Xcode? I'm running Monterey but am still seeing the issue

To be honest, I don't know what fixed it. I didn't update Xcode or macOS, but maybe when I updated SFSymbols.app to 3.1, it installed the new font to the system

[fredpi]

@Stevenmagdy

if we are going to provide the property (with a different name) anyway, then I think it's not justified enough to drop the conformance and break source compatibility, just for a name change.

I think it should be enough to make it a Set, only provide the latest name of each symbol, and provide documentation explaining what is included in the collection.

I understand this view, however, if we no longer include duplicates in the allCases variable, that's quite a significant change that users should know about – and the best way of letting them know about it IMHO is renaming the variable in a major release (or maybe keeping the old variable s. t. it contains the same as the new variable, but deprecating it and pointing to the new variable):

@available(*, deprecated, renamed: "allSymbols", message: "")
static var allCases: [SFSymbol] { return Array(SFSymbol.allSymbols) }

Also, I think that the name allCases doesn't properly reflect the content of the variable, if some duplicate cases are excluded. allSymbols describes it better IMO.

But at the end of the day, this is not such an important question, so decide whatever you think is best and I will be fine with it. 👍

To be honest, I don't know what fixed it. I didn't update Xcode or macOS, but maybe when I updated SFSymbols.app to 3.1, it installed the new font to the system

Yeah, that's indeed the case! Updating SFSymbols.app to 3.1 solved it for me, too.

[StevenSorial]

@fredpi Done.

btw, this PR also contains a fix for wrong(?) deprecation annotations for multiple renamings. Example:
starLefthalfFill (13.0) -> starLeadinghalfFill (14.0) -> starLeadinghalfFilled (15.0)

Previously, starLefthalfFill and starLeadinghalfFill both would be marked as deprecated: 15.0, renamed: "starLeadinghalfFilled". so if a user is using starLefthalfFill and then raised the minimum requirement to iOS 14.0, he would still not get the warning, and would be using the iOS 13.0 name, preventing them from upgrading to the iOS 14.0 name.

It's now fixed, starLefthalfFill is marked as deprecated: 14.0, renamed: "starLeadinghalfFill" and starLeadinghalfFill is marked as deprecated: 15.0, renamed: "starLeadinghalfFilled"

[fredpi]

Thanks @Stevenmagdy 🎉