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

Clean up Localizations #518

Closed
LePips opened this issue Aug 8, 2022 · 10 comments · Fixed by #1372
Closed

Clean up Localizations #518

LePips opened this issue Aug 8, 2022 · 10 comments · Fixed by #1372
Labels
developer Developer facing issues enhancement New feature or request

Comments

@LePips
Copy link
Member

LePips commented Aug 8, 2022

The base file Translations/en.lproj/Localizable.strings needs to be cleaned up as:

1 - there are many unused strings
2 - strings have just always been appended to the end of the file. This has lead to merge conflicts and has no organization.

So we need to find the unused strings, remove them, and then sort the remaining strings by some organization format. I was thinking sections by purpose and strings are alphabetized within each section:

/* General */
"close" = "Close";
...

/* QuickConnect */
"quickConnect" = "QuickConnect"
"quickConnectStep1" = "..."
...

As far as I know, re-organizing the strings does not cause Weblate to break.

@LePips LePips added enhancement New feature or request developer Developer facing issues labels Aug 8, 2022
@JPKribs
Copy link
Member

JPKribs commented May 25, 2024

@LePips I can go through and do this. I was starting to work on this for the PR I was working on but it should be pretty straight forward for me to apply this to the other labels as well. I can make a PR for this, but does this layout make sense:

In-Progress Labels

Screenshot 2024-05-25 at 3 53 45 PM

At this time, my testing is that I do a search for the label before I organize it. If not found, I comment out labels and try a clean build for both tvOS and iOS to see if the label didn't show up in the original search on the project.

@JPKribs
Copy link
Member

JPKribs commented May 25, 2024

As a note, the indentation does not interfere with the build. Let me know if this looks too busy with the indentation. This is what looked good to me but I'm game for whatever.

@LePips
Copy link
Member Author

LePips commented May 25, 2024

Thank you for wanting to take a look at this. I like the comments for each string, but let's not indent anything.

The main issue that has prevented me from working on this over the years is determining how this interacts with Weblate/other languages and whether it will either move all of the strings or just nuke them. If it does the latter, then we cannot do this. If we have to manually perform the same movements in all files, then we'll probably need a script.

For determining which strings are unused, you can use one of the following which detects unused code and you can just see what it outputs for the Strings.swift file:

@JPKribs
Copy link
Member

JPKribs commented May 25, 2024

Thank you for wanting to take a look at this. I like the comments for each string, but let's not indent anything.

The main issue that has prevented me from working on this over the years is determining how this interacts with Weblate/other languages and whether it will either move all of the strings or just nuke them. If it does the latter, then we cannot do this. If we have to manually perform the same movements in all files, then we'll probably need a script.

For determining which strings are unused, you can use one of the following which detects unused code and you can just see what it outputs for the Strings.swift file:

* https://github.com/PaulTaykalo/swift-scripts

* https://github.com/peripheryapp/periphery

Sounds good. I can throw together a version of this and try testing for weblate. I'm assuming you mean this would break from the user side not from the build side? Otherwise, I can test to see if these localization are still active when I build and switch languages?

@JPKribs JPKribs mentioned this issue May 26, 2024
4 tasks
@LePips
Copy link
Member Author

LePips commented May 26, 2024

Yes, the issue isn't building, but that we just don't lose translated strings.

@JPKribs
Copy link
Member

JPKribs commented May 26, 2024

I'd be happy to do the re-organization and then, if the translations wouldn't map, we can toss the PR. I'm just looking for things I can help with while I'm getting better at SwiftUI on the side. So no hard feelings from me if this doesn't work out

@JPKribs
Copy link
Member

JPKribs commented Dec 12, 2024

I was going to make another issue for looking at this then I remembered this one! I am the biggest offender for this but I wanted to discuss before I started making changes. Primarily, I think fixing up some of the casing is a decent place to start:

Image

I can go in and start getting some of our English localizations worked out to be more consistent but I wanted to make sure I was doing this right.

Option 1

Handle all localizations in lowercase, or titlecase or uppercase, whichever is the best for storage. Then, I know SwiftUI has some functions for Strings like:

  • localizedCapitalized
  • localizedUppercase
  • localizedLowercase
  • uppercased
  • lowercased

That we could use based on usage?

Option 2

Move all localizations to Sentence case or whatever our most common usage will be. Then, as needed, we can create Title localizations for instances where we need Title Case Strings vs Sentence case strings .


My thought it, when we feel ready to proceed, I can knock out localization organizing, deleting unused ones, and get casing knocked out all at once.

Let me know what you think is best (and when would be a good time to look at this again). No rush at all!

@JPKribs
Copy link
Member

JPKribs commented Dec 13, 2024

I've scripted this out in Python. Well, ChatGPT and I scripted this out in Python... But, I am doing:

  1. Find all usages of a localization
  2. Find all strings that have existing localizations
  3. Find all strings that do not have localizations
  4. Create a new Localizable.strings that is alphabetized and looks like this:
/// Who's watching?
/// Total Usages: 0
"WhosWatching" = "Who's watching?";

/// About
/// Total Usages: 4
"about" = "About";

/// Absolute
/// Total Usages: 1
"absolute" = "Absolute";
...
  1. Create a log file that contains all strings that have localizations and where they should go:
Video Player Type (use L10n.videoPlayerType instead):
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/Strings/Strings.swift (Line 1418)

The video profile is not supported (use L10n.videoProfileNotSupported instead):
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/Strings/Strings.swift (Line 1420)

The video range type is not supported (use L10n.videoRangeTypeNotSupported instead):
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/Strings/Strings.swift (Line 1422)

Video remuxing (use L10n.videoRemuxing instead):
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/Strings/Strings.swift (Line 1424)

The video resolution is not supported (use L10n.videoResolutionNotSupported instead):
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/Strings/Strings.swift (Line 1426)
...
  1. Create a log of ALL strings in Swiftfin and where they are located:
"Double Touch" found at:
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Swiftfin/Views/SettingsView/GestureSettingsView.swift (Line 51)

"Download" found at:
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Swiftfin/Views/DownloadTaskView/DownloadTaskContentView.swift (Line 51)

"Download task does not have media url for item: \(downloadTask.item.displayTitle)" found at:
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Shared/ViewModels/VideoPlayerManager/DownloadVideoPlayerManager.swift (Line 18)

"Downloaded items are only playable through the Swiftfin video player." found at:
  - /Users/jpkribs/Projects/StringTest/Swiftfin/Swiftfin/Views/DownloadTaskView/DownloadTaskContentView.swift (Line 119)
...

This makes it very easy whenever we're ready to look at localizations. My only question is do we like that Localizable.strings format where it's just the String and a count of usages?

Script attached. Nvm it's probably smart that you can't attach .pys like that. Script available on request!

@LePips
Copy link
Member Author

LePips commented Dec 13, 2024

Option 1 is best since not everything should necessarily be lowercased since proper nouns, like Jellyfin, should be capitalized. Like you've noted, for places where we would still need title case, like section or navigation headers, we would use the localizedCapitalized helpers.

However we still have discretion over what counts as a proper noun (thing). For example, in that screenshot:

  • Random Image -> Random image, since image is not a proper noun
  • Favorites, Next Up and Recently Added are names for sections of media, and should stay capitalized. Depending on where these names may be in the strings we wouldn't be able to use localizedCapitalized or other modifiers if these are all sentence case.

I do like the organization provided by that script but would not include the count of usages. My only consideration for usages would be whether to remove unused strings.

@JPKribs
Copy link
Member

JPKribs commented Dec 13, 2024

Like you've noted, for places where we would still need title case, like section or navigation headers, we would use the localizedCapitalized helpers.

Literally my only issue localizedCapitalized is TV. Since it turns TV Shows into Tv Shows. That is a very very very minor reason to not like this haha.

I just committed #1362 which removed unused localizations and alphabetizes all used localizations. I think that's a decent starting point where I can manually go in and start putting everything in sentence case with proper nouns.

Either that, or I have a list of "Strings" that I can start localizing all the hardcoded values?

Also, do we want to store the script anywhere if we need it later? I was not super hard to do so I don't feel supper attached to it and I don't know if we'll need to rerun this in the future.

Lastly, can we just do a viewModifer that uses localizedCapitalized for .navigationTitle or will that need to be updated Since that might screw up movies/shows ItemView

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Developer facing issues enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants