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

Maintenance: Remove unused string from localization #1232

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wutschel
Copy link
Collaborator

@wutschel wutschel commented Jan 13, 2025

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

@wutschel wutschel force-pushed the remove_unused_string branch from 02cbacb to 9145b1f Compare January 16, 2025 21:29
@wutschel wutschel marked this pull request as ready for review January 18, 2025 10:29
@wutschel
Copy link
Collaborator Author

All supported languages now have accepted or at least proposed translations. We can remove the old string now.

@wutschel wutschel force-pushed the remove_unused_string branch 2 times, most recently from 779cd66 to def7b43 Compare January 19, 2025 16:29
@wutschel wutschel marked this pull request as draft January 19, 2025 16:48
@wutschel wutschel force-pushed the remove_unused_string branch from def7b43 to f792947 Compare January 19, 2025 18:38
@wutschel wutschel marked this pull request as ready for review January 21, 2025 18:04
@wutschel
Copy link
Collaborator Author

@kambala-decapitator, the bug when using Turkish language was a really great example of bad design.

@kambala-decapitator
Copy link
Collaborator

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

@wutschel
Copy link
Collaborator Author

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 UIAlertAction is not directly supporting tag or something else. So I would need to create a new class which has an additional member tag and in the handler check for this instead of the action's string.

@kambala-decapitator
Copy link
Collaborator

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 objc_setAssociatedObject().

@wutschel
Copy link
Collaborator Author

it's almost trivial to modify handler to accept internal identifier, for example:

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.

@kambala-decapitator
Copy link
Collaborator

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?

@wutschel
Copy link
Collaborator Author

wutschel commented Jan 28, 2025

So this would require a bunch of the following actions which covers all needed actions:

UIAlertAction *action1 = [UIAlertAction ... handler:^{ [self do_this] }];
UIAlertAction *action2 = [UIAlertAction ... handler:^{ [self do_that] }];

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:

menu.sheetActions = @[
    action_play,
    action_queue,
];

DetailViewController:

UIAlertAction *action1 = [UIAlertAction ... handler:^{ [self play] }];
UIAlertAction *action2 = [UIAlertAction ... handler:^{ [self queue] }];

for (actionID in sheetActions) |
    switch (actionID) {
        case action_play:
            [alertCtrl addAction: action1];
            break;
        case action_queue:
            [alertCtrl addAction: action2];
            break;
       default:
           break;
    }
}

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 %@".
@wutschel wutschel force-pushed the remove_unused_string branch from f792947 to 78ba741 Compare January 29, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove outdated AppInfo string after main translations for updated string arrived
2 participants