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

Fix localization keys for local account names in Account package #3991

Merged
merged 1 commit into from Jun 4, 2023

Conversation

J-rg
Copy link
Contributor

@J-rg J-rg commented May 28, 2023

No description provided.

@brentsimmons
Copy link
Collaborator

@stuartbreckenridge As the localization expert, would you review this PR?

@stuartbreckenridge
Copy link
Member

Account Names are currently localized via the main app's .strings files. As it stands, replacing the keys in the Account package will not do anything.

func localizedAccountName() -> String {
		
		switch self {
		case .onMyMac:
			let defaultName: String
			#if os(macOS)
			defaultName = NSLocalizedString("account.name.mac", comment: "On My Mac")
			#else
			if UIDevice.current.userInterfaceIdiom == .pad {
				defaultName = NSLocalizedString("account.name.ipad", comment: "On My iPad")
			} else {
				defaultName = NSLocalizedString("account.name.iphone", comment: "On My iPhone")
			}
			#endif
			return defaultName
		
		/* The below account names are not localized as they are product names. */
			
		case .bazQux:
			return "BazQux"
		case .cloudKit:
			return "iCloud"
		case .feedbin:
			return "Feedbin"
		case .feedly:
			return "Feedly"
		case .freshRSS:
			return "FreshRSS"
		case .inoreader:
			return "Inoreader"
		case .newsBlur:
			return "NewsBlur"
		case .theOldReader:
			return "The Old Reader"
		}
	}

@brentsimmons
Copy link
Collaborator

@stuartbreckenridge Does that mean this PR should be closed?

@stuartbreckenridge
Copy link
Member

Yes. For the moment, they are localized in the app.

@J-rg
Copy link
Contributor Author

J-rg commented May 30, 2023

But they are not? NSLocalizedString is looking for the “On My X” keys in the Localizable.strings file, which do not exist. It’s not noticeable in the English localization because it uses the key as a fallback.

@stuartbreckenridge
Copy link
Member

If we take this code from AddAccountListView on iOS:

Text(verbatim: AccountType.onMyMac.localizedAccountName())

The localizedAccountName() uses the app's Localizable.strings file:

/* On My iPad */
"account.name.ipad" = "On My iPad";

/* On My iPhone */
"account.name.iphone" = "On My iPhone";

/* On My Mac */
"account.name.mac" = "On My Mac";

@stuartbreckenridge
Copy link
Member

Note that there is a TODO to move localizedAccountName to the Account package.

@J-rg
Copy link
Contributor Author

J-rg commented May 30, 2023

I’m not sure we’re talking about the same thing? This PR is not about localizedAccountName, that one works fine (because it uses the correct keys), I never doubted that.

@stuartbreckenridge
Copy link
Member

That's my point @J-rg — we do not need to localize the account names in the Account package just now because it's handled in the main app.

If that changes in the future, this PR is 100% correct in that we'd need to re-key the strings in code, but we'd also need to add translations in the Account package's Localizable.strings.

@J-rg
Copy link
Contributor Author

J-rg commented May 31, 2023

Okay, I think I see where the misunderstanding is.
- There is user-facing text that uses Account’s nameForDisplay, defaultName or defaultLocalAccountName and not localizedAccountName (sidebar header and account preferences on macOS, Reddit extension description on iOS). This text is currently not localizable with the provided keys.
- If NSLocalizedString is used in a framework with a key that is not present in the framework’s Localizable.strings, the value from the application’s Localizable.strings will be used (provided the key exists there).
So this does fix things (you can verify this easily in the new Simplified Chinese localization), until localizedAccountName is moved into the Account package and type.localizedAccountName() can be returned in nameForDisplay.

@stuartbreckenridge
Copy link
Member

@brentsimmons Let's approve this. @J-rg has valid points and this is an additive PR. I will do some follow-up work to move Account name-related localizations from the main app to the Account package.

@brentsimmons brentsimmons merged commit b7f1167 into Ranchero-Software:main Jun 4, 2023
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

3 participants