Add LocalizedStringKey initializer to Recorder to support localizing the title #161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rtharston
commented
Jan 13, 2024
| @@ -151,18 +174,18 @@ extension KeyboardShortcuts.Recorder { | |||
|
|
|||
| @available(macOS 10.15, *) | |||
| #Preview { | |||
| KeyboardShortcuts.Recorder(for: .init("xcodePreview")) | |||
| KeyboardShortcuts.Recorder("record_shortcut", name: .init("xcodePreview")) | |||
There was a problem hiding this comment.
I used the key of an existing string so the preview wouldn't require a new string to be localized.
rtharston
commented
Jan 13, 2024
| - Parameter name: Strongly-typed keyboard shortcut name. | ||
| - Parameter onChange: Callback which will be called when the keyboard shortcut is changed/removed by the user. This can be useful when you need more control. For example, when migrating from a different keyboard shortcut solution and you need to store the keyboard shortcut somewhere yourself instead of relying on the built-in storage. However, it's strongly recommended to just rely on the built-in storage when possible. | ||
| */ | ||
| @_disfavoredOverload |
There was a problem hiding this comment.
See the description for an explanation of @_disfavoredOverload.
rtharston
commented
Jan 13, 2024
| @@ -114,7 +114,7 @@ extension KeyboardShortcuts.Recorder<Text> { | |||
| - Parameter onChange: Callback which will be called when the keyboard shortcut is changed/removed by the user. This can be useful when you need more control. For example, when migrating from a different keyboard shortcut solution and you need to store the keyboard shortcut somewhere yourself instead of relying on the built-in storage. However, it's strongly recommended to just rely on the built-in storage when possible. | |||
| */ | |||
| public init( | |||
| _ title: String, | |||
| _ title: LocalizedStringKey, | |||
There was a problem hiding this comment.
If @_disfavoredOverload can't be used in this package, this line can be changed to something like: key title: LocalizedStringKey to remove the conflict between this init and the init that accepts StringProtocol.
rtharston
commented
Jan 13, 2024
|
Oops. Sorry about the wrong spacing. I was careful about the lines I made changes on, but forgot to check that whole block. Thanks for fixing it! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently I am using a Text view manually to localize the title to my shortcut recorder, because the existing initializer only takes a String and isn't localized automatically.
This PR adds a second initializer to Recorder so the caller can continue to use String (as currently supported) or a LocalizedStringKey (as Text does) for localization.
NOTE: This doesn't add any localization burden to this package. This only enabled localization to work properly in the application using this package.
To get the compiler to prefer the LocalizedStringKey initializer instead of the String version, SwiftUI labels the String version of the Text view initializer with
@_disfavoredOverload. I copied that attribute directly from SwiftUI.Text's initializer, and it is necessary for this change to work.(As explained here.)
If using a private attribute isn't acceptable for this package then another option would be to rename the initializer parameter of the LocalizedStringKey version to include 'localizationKey' or something like that. That would require the user to know to use the right version of init, but at least it makes it available, which is better than the status quo.
I also added a title to the previews so the change could be visible in Xcode with just the package open, but the preview doesn't load for me. I tested in my own application and confirmed it works there.
(The SwiftUI.Text LocalizedStringKey initializer also has a compiler attribute,
@_semantics, but it is just used for optimizations of some kind so I left it off in the PR. What do you think of including it too? This blog post discusses how it is used.)