-
Notifications
You must be signed in to change notification settings - Fork 104
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
Maintenance: Remove unused string from localization #1232
base: master
Are you sure you want to change the base?
Conversation
02cbacb
to
9145b1f
Compare
All supported languages now have accepted or at least proposed translations. We can remove the old string now. |
779cd66
to
def7b43
Compare
def7b43
to
f792947
Compare
@kambala-decapitator, the bug when using Turkish language was a really great example of bad design. |
I think we already had this discussion about checking user-visible strings vs some kind of internal identifier (numeric or string) :) |
Yes, we had this kind of problem twice in the past years. Seems |
it's almost trivial to modify handler to accept internal identifier, for example: UIAlertAction* action1 = [UIAlertAction ... handler:^{ [self handler:Action1EnumCase] }];
UIAlertAction* action2 = [UIAlertAction ... handler:^{ [self handler:Action2EnumCase] }]; you could also use objc runtime magic to attach arbitrary data, see |
Hmm, often the action sheet is built in a loop from all defined action strings. In such case I would need to create an array or dictionary for each action to also hold the enum. Nothing I had on my agenda. |
yes, array of pairs <key, localization> sounds good. But you'd rather ask yourself: do you really have/need such a generic handler? Maybe easier to write respective code in each separate UIAlertAction handler? |
So this would require a bunch of the following actions which covers all needed actions:
In this case I would need to move the logic of what I am finally showing in the action sheet (currently a simple array of action strings which is majorly defined in AppDelegate) to the place where I am building the sheet. And ideally replacing the string in AppDelegate with enums. Something like this: AppDelegate:
DetailViewController:
Let me create an issue to the backlog. Should anyway not be done here in this PR. |
Instead of comparing two separately translated strings (which is faulty for Turkish, and in general not robust), use the same string as base to identify the "Resume from" pattern for all languages This also allows to remove the string "Resume from" completely as we already have "Resume from %@".
f792947
to
78ba741
Compare
Description
Resolves #1231.
One string was used in
AppInfoViewController
and replaced in #1214. The content only changed in a very minor way, so it was kept for a while to allow translators have it as a reference in Kodi Weblate. Other obsolete strings were formerly used in former releases instead of icons or when user were asked to confirm power or library related actions. Also, there were two strings maintained around the scrubbing speed change, which I reduced the only valid one.In one interesting case I found even a logical error around the resume function which made this always fail when running the UI in Turkish. This is also fixed by a code change which now generates a
resumeKey
from the localized string"Resume from %@"
and compares this with the user selected action's name, which always shows from which position to resume, e.g."Resume from 15:23"
.Summary for release notes
Maintenance: Remove unused string from localization
Bugfix: Make resume work when using Turkish localization