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

Support explicit symbol localization #90

Closed
wants to merge 9 commits into from

Conversation

knothed
Copy link
Collaborator

@knothed knothed commented Dec 8, 2021

Implements #87.

Following this suggestion by @Stevenmagdy, one protocol per localization per availability is created.

The decision for the protocol names fell on hi and hi_v30 as these are shorter, easier to read and look better than HiLocalizable and HiLocalizableV30. What do you think?

Additionally, allSymbols was made into an SFSymbolSet which allows to localize all symbols (via allSymbols.hi) and which exposes allSymbolVariants containing every localization variant of every symbol.

The symbol documentations were adapted to match the actual exposed localizations: when deprecated symbols do not provide some localizations, but only their new versions do, these localizations are not in the documentation of the deprecated symbol to match its actual type. (example in the image)
image

@knothed
Copy link
Collaborator Author

knothed commented Dec 8, 2021

There is still a problem regarding SFSymbolSet (causing the failing tests): checking whether a LocalizedSymbol is ar always yields true, no matter whether the symbol actually supports ar-localization.
I will solve this later today by adding an enum Localizations and then providing each SFSymbol with the information about how localizable it actually is.

@StevenSorial
Copy link
Member

StevenSorial commented Dec 8, 2021

@knothed Thanks for the PR.

I took a quick look and have a few questions:

Documentation

The symbol documentations were adapted to match the actual exposed localizations.

As explained in #82, Localizations are available for old names, just not explicitly. I suggest mentioning for every localization whether it's explicit or implicit:

    /// Localizations:
    /// - Chinese (Implicit) (iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0)
    /// - Hindi (Explicit) (iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0)

Localization Protocols

  • I noticed that some localization protocols are not used (eg: rtl_v30). We don't need to generate a localization protocol for every availability, as long as we have a default one for every localization (eg: rtl). I suggest generating only the protocols that are needed.
  • I agree that the naming scheme in HiLocalizable and HiLocalizableV30 is a bit long but I don't think hi and hi_v30 are clear enough. How about HiSymbol and HiSymbol3_0 ?
  • Is there a reason we don't provide a default implementation for the protocols? it will make LocalizableSymbol simpler and make custom localized new symbols (inheriting SFSymbol) easier.

SFSymbolSet

Can you please explain more the reasoning behind this new type? is it for testing? tbh I think it's a bit overkill. allSymbols does not need to know anything about localizations.

@knothed
Copy link
Collaborator Author

knothed commented Dec 8, 2021

I suggest mentioning for every localization whether it's explicit or implicit

Sounds good, will do.

I suggest generating only the protocols that are needed.

Also good. It really doesn’t matter if they’re there or not.

How about HiSymbol and HiSymbol3_0 ?

Hmm… If there is a symbol with 9 localiztions, this will produce quite a long type. I think hi is clear enough as it is only used in conjunction with SFSymbol.

Is there a reason we don't provide a default implementation for the protocols?

Yes, it’s impossible – as hi is not an SFSymbol nor a RawRepresentable, it doesn’t have access to rawValue.

SFSymbolSet

Well, if we support SFSymbol.allSymbols we should also support localizing this symbol collection explicitly. There‘s no reason not to. The same use-case justifying allSymbols also justifies localizing them in my opinion.

Also, yes, it’s quite handy for testing.

@StevenSorial
Copy link
Member

StevenSorial commented Dec 8, 2021

Yes, it’s impossible – as hi is not an SFSymbol nor a RawRepresentable, it doesn’t have access to rawValue.

You can constraint the extension:

extension hi where Self: SFSymbol {
    var hi: SFSymbol { .init(rawValue: "\(rawValue).hi") }
}

or even better, constraint the protocol itself. This will also solve the naming issue because the constraint itself will be the explanation (we don't need to include "symbol" in the name):

public protocol hi where Self: SFSymbol {
    var hi: SFSymbol { get }
}

extension hi  {
    var hi: SFSymbol { .init(rawValue: "\(rawValue).hi") }
}

and I think its enough to only make the extension for the default localization protocols (I'm not sure)

we should also support localizing this symbol collection explicitly

Hmm… I'm not sure. if you look at the SFSymbols.app, the "All" tab does not have the localized versions. I think allSymbols, like the "All" tab, in my opinion, means all base symbols, not all possible variations of each symbol.

@fredpi
Copy link
Member

fredpi commented Dec 8, 2021

Good work @knothed, good suggestions @Stevenmagdy! 👍

Regarding the allSymbols discussion: Say I wanted to pick a random symbol, but only in arabic localization (or no localization): I think this is a valid use case just as it is a valid use case to choose an explicitly arabic localized variant of a single symbol. Also, with the SFSymbolSet, it will be possible to retrieve all symbols for which arabic localizations are available: allSymbols.ar "-" allSymbols.someOtherLocalization.

Before this PR gets merged, we also still need some README-documentation on the availability of explicit localizations (and maybe on the SFSymbolSet if we decide not to remove it).

@fredpi
Copy link
Member

fredpi commented Dec 8, 2021

I forgot to mention that I'm rather against specifying whether the localization is available explicitly or implicitly. Even users with advanced knowledge won't really understand what this means at first sight...

@StevenSorial
Copy link
Member

StevenSorial commented Dec 8, 2021

I'm rather against specifying whether the localization is available explicitly or implicitly.

@fredpi I agree, it's confusing. but I think it's more important to mention the implicit localizations because that is what most users will care about. How about keeping the localizations docs as it is (before the PR) and maybe explaining the explicit localized properties in the README?

@knothed
Copy link
Collaborator Author

knothed commented Dec 8, 2021

You can constraint the extension, or even better, constraint the protocol itself.

Nice, haven't thought of that!
But what use do we have in providing a default implementation? I don't see an advantage of declaring the implementations in the respective protocols over declaring them in LocalizableSymbol as long as LocalizableSymbol is the only inhabitant of these protocols. Or was it somehow for supporting the user in declaring their own localizable symbols? If so, then we would need to give the default implementation in all the protocol variants for consistency.

How about keeping the localizations docs as it is (before the PR) and maybe explaining the explicit localized properties in the README?

Hmm, I think it's most confusing if the documentation says "Localizable in Arabic" but then there is no .ar available for the symbol. Would seem like a bug to me. I don't think putting it in the README saves us here.

I think allSymbols, like the "All" tab, in my opinion, means all base symbols, not all possible variations of each symbol.

Yes, writing SFSymbol.allSymbols will give you all base symbols. But if you really want all possible localization variants, you can write SFSymbol.allSymbols.allLocalizationVariants, or when you want all base symbols but localized in Hindi where possible, you write SFSymbol.allSymbols.hi. Cool, right?

@knothed
Copy link
Collaborator Author

knothed commented Dec 8, 2021

I propose to add a public has(localization: Localization) -> Bool method to SFSymbol which would allow users to question whether a localization is available for the current platform.
This is a sensible addition to the localization protocols due to the limitation that symbol is ar will yield true for every LocalizedSymbol no matter whether it should actually expose an ar localization or not, meaning there are false positives.

So, if the user had some SFSymbol, say by drawing a random element from allSymbols, they could write:

if symbol.has(.ar) {
    let localized = (symbol as! ar).ar
    // use localized
} else {
    // use symbol
}

The new Localization struct would look like this:

struct Localization {
    case ar
    ...
    case zhTraditional
}

I will solve this later today by adding an enum Localizations and then providing each SFSymbol with the information about how localizable it actually is.

I would use this inside SFSymbolSet, but I thought maybe the user can also profit from this method. What do you think?

@j-f1
Copy link

j-f1 commented Dec 9, 2021

Proposal:

if let localized = symbol.localized(to: .ar) {
    // use localized
} else {
    // use symbol
}

@knothed
Copy link
Collaborator Author

knothed commented Dec 9, 2021

I like it @j-f1.

This would make the SFSymbolSet compltely redundant as users can just write:

symbols.map {
    $0.localized(to: .ar) ?? $0
}

We could still make an extension Set where Element: SFSymbol for this, but the magic would also be accessible to the user and not be hidden inside SFSymbolSet.

@knothed
Copy link
Collaborator Author

knothed commented Dec 9, 2021

If @fredpi and @Stevenmagdy have no objection, I would implement this as suggested by @j-f1. This would get rid of SFSymbolSet and allow the user, in addition to static localization via symbol.ar, to perform dynamic, i.e. runtime localization via symbol.localized(to: .ar).

@StevenSorial
Copy link
Member

StevenSorial commented Dec 9, 2021

we would need to give the default implementation in all the protocol variants for consistency.

I agree, we should provide them for all protocols for consistency and to make it easier for conformance.

due to the limitation that symbol is ar will yield true for every LocalizedSymbol.

Yes, we didn't think of this limitation/bug. I think we should rethink the protocol approach because of this.

I propose to add a public has(localization: Localization) -> Bool method to SFSymbol

Proposal: if let localized = symbol.localized(to: .ar) {

Are those proposals a replacement for the protocol approach? That is very similar to my option 2 in #87 (first post) which was not chosen because of the optionality. I actually like it, I'm just curious.
also, where do you plan to store the actual localization information?

@StevenSorial
Copy link
Member

@knothed sorry I didn't see your last comment.

in addition to static localization via symbol.ar, to perform dynamic, i.e. runtime localization via symbol.localized(to: .ar)

I'm against providing both approaches, this is too complicated. I think we should only provide one intuitive approach.

@knothed
Copy link
Collaborator Author

knothed commented Dec 9, 2021

Yes, we didn't think of this limitation/bug. I think we should rethink the protocol approach because of this.

I like the protocol approach, it’s pretty simple to use, but we cannot avoid this limitation. Even if we would introduce a new class per localization availability combination, we still couldn’t do symbol is ar, because symbol could be ar or ar_v2 or ar_v21 etc.

So to see whether an arbitraty SFSymbol is ar-localizable, we will need some kind of dynamic localization.

Are those proposals a replacement for the protocol approach?

No, I do not like the optionality. It would just be a supplement to the static localization, which is pretty nice as is.

also, where do you plan to store the actual localization information?`

Internally inside SFSymbol.

extension SFSymbol {
    static let arSymbol: SFSymbol & ar & hi_v30 = LocalizedSymbol(„symbol“, localizations: [.ar(.v1), .hi(.v30)])
}

The dynamic check (arSymbol.localize(to: .hi)) will then use the platform version of the calling code to determine whether hi is supported on this device or not.

@knothed
Copy link
Collaborator Author

knothed commented Dec 15, 2021

Should I move forward implementing this?

@StevenSorial
Copy link
Member

@knothed Sorry for the delayed response.

Should I move forward implementing this?

I'm still strongly against providing two APIs for localization, even if each one addresses a different use case. IMO this is an indication that the protocol approach was not a good fit in the first place.

I think we should release a new version ASAP (SFSymbols 3.2 is out), so I suggest postponing this feature until the next major version (we don't have to wait for SFSymbols 4.0). Until then, we can think of an API that can serve all use cases.

Other options to discuss:

  • Go with the protocol approach only but remove SFSymbolSet, we would still have the problem of is returning true for any localization which might cause confusion for the users.
  • Go with the runtime approach mentioned in Supporting explicit symbol localization #87 (option 3, first post) which doesn't offer much safety over the native UIImage.init which also is optional for invalid symbols, but at least the user would not have to write the symbol name manually.

@knothed @fredpi what do you think?

@knothed
Copy link
Collaborator Author

knothed commented Dec 25, 2021

Happy holidays!

I don’t really like the two options you stated. Optionality is kind of a deal-breaker as it provides no benefits over using raw strings.

We should think about a better approach before moving forward.

@fredpi
Copy link
Member

fredpi commented Jan 5, 2022

@Stevenmagdy @knothed Sorry for the period of inactivity!

@Stevenmagdy: "I think we should release a new version ASAP (SFSymbols 3.2 is out), so I suggest postponing this feature until the next major version (we don't have to wait for SFSymbols 4.0). Until then, we can think of an API that can serve all use cases."

I would suggest to try to quickly finish the implementation of the localized symbol supported before releasing version 3, because if there are breaking changes, the 3.0 release would be the best fit for them (there are already breaking changes in it).

@knothed: "I don’t really like the two options you stated. Optionality is kind of a deal-breaker as it provides no benefits over using raw strings."

I agree with this; I'm not very happy with these options.

I revisited #87 and found this suggestion. What about using it instead of the protocol approach? Then the user can't write symbol is ar but we still have static localization.

I would also implement dynamic localization as suggested in this comment by @knothed.

@Stevenmagdy: "I'm still strongly against providing two APIs for localization, even if each one addresses a different use case. IMO this is an indication that the protocol approach was not a good fit in the first place."

I don't think this would be an indication that the protocol approach is bad. Both use cases (dynamic and static) are valid. A dynamic approach is needed e. g. for operations on a SFSymbol Collection; a static approach is the only intuitive API design with regard to the existing way in which symbols are addressed: I think that users intuitively expect that the "abc.ar" symbol can be accessed in the same way in code, i. e. as .abc.ar. What do you think?

BTW: With #91 merged, this PR also needs a rebase.

@StevenSorial
Copy link
Member

I revisited #87 and found this suggestion. What about using it instead of the protocol approach?

The post ends up using LocalizableSFSymbol<OnlyArabicLocalizable> which means that we would have to define a localization struct for every localization combination, i.e, ArHeLocalizable , ArHiLocalizable, etc. which would be too much. @j-f1 please correct me if I got it wrong.

Here is another suggestion:

@dynamicMemberLookup
public class Localizable1Symbol<L1: SymbolLocalization>: SFSymbol {
  subscript(dynamicMember keyPath: KeyPath<L1, SFSymbol>) -> SFSymbol {
    L1(source: self)[keyPath: keyPath]
  }
}

@dynamicMemberLookup
public class Localizable2Symbol<L1: SymbolLocalization, L2: SymbolLocalization>: SFSymbol {
  subscript(dynamicMember keyPath: KeyPath<L1, SFSymbol>) -> SFSymbol {
    L1(source: self)[keyPath: keyPath]
  }
  subscript(dynamicMember keyPath: KeyPath<L2, SFSymbol>) -> SFSymbol {
    L2(source: self)[keyPath: keyPath]
  }
}

// Localizable3Symbol
// Localizable4Symbol
// Localizable5Symbol
// Localizable6Symbol
// Localizable7Symbol
// Localizable8Symbol

public protocol SymbolLocalization {
  init(source: SFSymbol)
}

public struct Ar: SymbolLocalization {
  let source: SFSymbol
  public init(source: SFSymbol) { self.source = source }
  public var ar: SFSymbol { .init(rawValue: "\(source.rawValue).ar") }
}

public struct Zh3_0: SymbolLocalization {
  let source: SFSymbol
  public init(source: SFSymbol) { self.source = source }
  @available(iOS 15.0, *)
  public var zh: SFSymbol { .init(rawValue: "\(source.rawValue).zh") }
}

extension SFSymbol {
    static let symbolWithAr = Localizable1Symbol<Ar>(rawValue: "symbolWithAr")
    static let symbolWithArAndZh = Localizable2Symbol<Ar, Zh3_0>(rawValue: "symbolWithArAndZh")
}

let exampleImage = UIImage(systemSymbol: . symbolWithArAndZh.ar)

Swift does not have Variadic Generics yet. Which will let us write
class LocalizableSymbol<L...>: SFSymbol where L: SymbolLocalization instead of the 8 redundant LocalizableXSymbol classes.

Variadic Generics is being discussed right now. But until its released, we can do what Apple does and duplicate the class/struct with a different number.

There is another workaround that I don't like, which is providing only Localizable8Symbol and a dummy localization to fill the remaining generic arguments:

public struct None: SymbolLocalization {
  let source: SFSymbol
  public init(source: SFSymbol) { self.source = source }
}

extension SFSymbol {
    static let symbolWithAr = Localizable8Symbol<Ar, None, None, None, None, None, None, None>(rawValue: "symbolWithAr")
    static let symbolWithArAndZh = Localizable8Symbol<Ar, Zh3_0, None, None, None, None, None, None>(rawValue: "symbolWithArAndZh")
}

What do you think? @fredpi @knothed @j-f1

@j-f1
Copy link

j-f1 commented Jan 13, 2022

I think that's reasonable! And if variadic genetics are added a year or two down the line (and assuming the compiler supports backdeploying them) it should be reasonable to switch to using them.

@knothed
Copy link
Collaborator Author

knothed commented Jan 14, 2022

And if variadic genetics are added a year or two down the line

I'm not quite sure how I should feel about this, sounds like a very sensitive issue to me. Especially if a compiler is involved. With variadic generics I'd be fine on the other hand...

Here is another suggestion: ...

I really like this approach @Stevenmagdy. How does it help us in performing a localisation check at runtime however? Wouldn't we have to basically switch over the type of the LocalizableXSymbol<...> we'd be dealing with?

@StevenSorial
Copy link
Member

StevenSorial commented Jan 15, 2022

How does it help us in performing a localization check at runtime, however?

@knothed This was only a suggestion for a replacement to the protocol approach to solve the is issue. My understanding is that you want to implement the runtime localization using an enum.

extension SFSymbol {
   static let arSymbol: SFSymbol & ar & hi_v30 = LocalizedSymbol(„symbol“, localizations: [.ar(.v1), .hi(.v30)])
}

If you want to derive the runtime checking from the static information, we can add additional information to the class/protocol:

protocol LocalizableSymbol where Self: SFSymbol {
  var localizations: [SymbolLocalization.Type] { get }
}

@dynamicMemberLookup
public class Localizable2Symbol<L1: SymbolLocalization, L2: SymbolLocalization>: SFSymbol, LocalizableSymbol {
  var localizations: [SymbolLocalization.Type] { [L1.self, L2.self] }
  subscript(dynamicMember keyPath: KeyPath<L1, SFSymbol>) -> SFSymbol {
    L1(source: self)[keyPath: keyPath]
  }
  subscript(dynamicMember keyPath: KeyPath<L2, SFSymbol>) -> SFSymbol {
    L2(source: self)[keyPath: keyPath]
  }
}

extension SFSymbol {
    public static let symbolWithAr = Localizable1Symbol<Ar>(rawValue: "symbolWithAr")
    public static let symbolWithArAndZh = Localizable2Symbol<Ar, Zh>(rawValue: "symbolWithArAndZh")
}

let allSymbols: [LocalizableSymbol] = [SFSymbol.symbolWithAr, SFSymbol.symbolWithArAndZh]
let arSymbol = allSymbols.filter { $0.localizations.contains { $0 == Ar.self } }
let zhSymbol = allSymbols.filter { $0.localizations.contains { $0 == Zh.self } }

But I think that should be used only for internal code (SymbolSet and Tests), for the public runtime API (although I'm still not fully convinced of its use case), we should use an outside-of-SFSymbol Dictionary, to make SFSymbol(rawValue: "character") has the same runtime localization information as SFSymbol.character, which will also be useful for codable symbols that lost its type information due to serialization.

@StevenSorial
Copy link
Member

@knothed Are you going to try and implement this approach?
@fredpi What do you think about this generic approach?

@knothed
Copy link
Collaborator Author

knothed commented Feb 5, 2022

Sorry, no time atm.

@knothed
Copy link
Collaborator Author

knothed commented Mar 19, 2022

@Stevenmagdy I‘d be ready to tackle this in the near future. Are there updates on the proposed implementation?

@StevenSorial
Copy link
Member

@knothed No, that is the best implementation that I can come up with, even though I don't like it very much.

@knothed
Copy link
Collaborator Author

knothed commented Mar 23, 2022

@Stevenmagdy I do quite like it, assuming variadic generics will make their way into Swift in the future.

@knothed
Copy link
Collaborator Author

knothed commented Mar 23, 2022

Deprecated via #98

@knothed knothed closed this Mar 23, 2022
@knothed knothed closed this Mar 23, 2022
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.

None yet

4 participants