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

Reorganize translation references names #20207

Merged
merged 1 commit into from Dec 19, 2022

Conversation

dragunoff
Copy link
Contributor

@dragunoff dragunoff commented Aug 19, 2022

The significant part here is the reorganization of the translation IDs in en.ftl. They are more descriptive to provide better context. Generally the template is thing-id where "thing" is usually the UI element (label, button, notification, dialog, dropdown, checkbox, etc.). The aim of this PR is primarily to change the names. Later PRs may seek to consolidate some redundant strings and to better organize the translation file.

See this comment further down for more detail on the rationale behind the renaming and prefixing of keys: #20207 (comment)

Depends on #20048
Depends on #20400

@dragunoff
Copy link
Contributor Author

Rebased.

@dragunoff dragunoff marked this pull request as ready for review September 2, 2022 13:19
@dragunoff dragunoff changed the title WIP: Reorganize translation references names Reorganize translation references names Sep 2, 2022
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I glossed though all the changes and looked for regressions in game. I noted down some things for this pr and a few less related optimisations

mods/common/languages/en.ftl Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
OpenRA.Game/Server/Server.cs Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
@Mailaender
Copy link
Member

Neither https://projectfluent.org/fluent/guide/ nor https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers use any kind of redundant prefixes. I would recommend against adding those as well.

@dragunoff
Copy link
Contributor Author

Neither https://projectfluent.org/fluent/guide/ nor https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers use any kind of redundant prefixes. I would recommend against adding those as well.

These are contrived documentation examples and I think that approach doesn't scale well for a large project. I think we should have a look at how translation is handled in the wild. If we look at the l10n files for Firefox for example we can see plenty of redundancy in translation IDs (they use suffixes but that's besides the point). And the point is that this redundancy is there to provide context. I'll try to illustrate with an nice example from OpenRA's codebase:

close = Close

Is that a verb or another part of speech (e.g. "Close a dialog" or "Close to something")? It lacks context so we can't really tell what is the meaning thus we can't provide adequate translation. In our case the meaning we're conveying is "Camera zoom level that shows a close view of the battlefield". So I'd argue that adding context to the translation ID reduces ambiguity significantly and avoids conflicts (e.g. if we want to use "Close" as a button label). The translation ID can provide even more context by stating the type of UI element that this message is used in. So a better way to write this is:

label-camera-close = Close

I can see how this quickly becomes quite verbose and repetitive so an even better way to write this may be with the help of attributes (I don't know if they are supported in our implementation) and optionally a comment bound to the translation:

# A dropdown to choose the zoom level for the battlefield camera
dropdown-camera =
    .option-close = Close
    .option-medium = Medium
    .option-far = Far
    .option-furthest = Furthest

This approach works nicely for other UI elements that have a lot of related messages, such as dialogs:

dialog-settings-restart =
    .title = Restart Now?
    .prompt = Some changes will not be applied until the game is restarted. Restart now?
    .accept = Restart Now
    .cancel = Restart Later

And to avoid some of the redundancy (for example with common button label names) we can pass a reference to another message:

dialog-cant-play-video =
    .title = Unable to play video
    .prompt = Something went wrong during video playback.
    .accept = { button-back }

button-back = Back

I'd be fine with cutting down on some of the prefixes for messages that are not so context sensitive and conflicts are unlikely. But I don't see harm in keeping them and following a convention with regards to translation IDs (whether we use prefixes or suffixes doesn't matter much).

As the translation files are gonna grow larger we should think how to better organize them. Perhaps group translation by type of UI element instead of location in the source, maybe split the translation files and think of how we can generate them automatically from the source code 🤔

@dragunoff dragunoff force-pushed the feature/ftl-translation-ids branch 2 times, most recently from d4095b5 to fcb82dd Compare September 7, 2022 16:49
@dragunoff
Copy link
Contributor Author

Update: rebased and addressed #20207 (comment) and #20207 (comment)

@PunkPun
Copy link
Member

PunkPun commented Sep 9, 2022

Needs a rebase

@dragunoff
Copy link
Contributor Author

Update: rebased and implemented attributes (see #20207 (comment)). I've expanded the translation linting to account for attributes and included a testcase commit that triggers all errors and warnings.

@dragunoff dragunoff marked this pull request as ready for review October 2, 2022 10:26
@PunkPun
Copy link
Member

PunkPun commented Dec 7, 2022

Dependancy merged

@PunkPun PunkPun modified the milestones: Next release, Next + 1 Dec 8, 2022
@dragunoff
Copy link
Contributor Author

dragunoff commented Dec 12, 2022

Update: rebased and updated some translation keys.

I decided to split the "dropdown" from its "options" because the options can be used in other contexts. So we have for example:

dropdown-game-speed =
    .label = Game Speed
    .description = The rate at which time passes

options-game-speed =
    .slowest = Slowest
    .slower = Slower
    .normal = Normal
    .fast = Fast
    .faster = Faster
    .fastest = Fastest

@dragunoff dragunoff marked this pull request as ready for review December 12, 2022 12:59
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
mods/common/languages/en.ftl Outdated Show resolved Hide resolved
- Add prefixes to all message keys to provide context
- Use messages with attributes for some UI elements (dropdowns, dialogs, checkboxes, menus)
- Rename some class fields for consistency with translation keys
@dragunoff
Copy link
Contributor Author

Update: addressed formatting nits

@PunkPun
Copy link
Member

PunkPun commented Dec 17, 2022

other than that LGTM

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but a question before merging: I thought we wanted this on prep to avoid an unnecessary major API break between releases? Or has that changed?

@PunkPun
Copy link
Member

PunkPun commented Dec 19, 2022

We do not want this for prep, unfortunately this didn't make the cut. This PR is based on #20400 which is also not on prep.

The current translations on prep only work with UI and are still hardcoded to english, so only mods with custom UI might feel a change between releases, that is if they first decide to use the API. I think it's unlikely for the API break to have massive consequences so I'm fine with the state of linguini on prep

@pchote
Copy link
Member

pchote commented Dec 19, 2022

The issue isn't the API changes to the UI, but to the translators. It is inevitable that, even if we warn against it, there are going to be people that put a lot of effort into creating translations only to find that their work is wasted/incompatible with the next release.

@PunkPun
Copy link
Member

PunkPun commented Dec 19, 2022

That as well. Hopefully the lack of things to translate discourages people from doing it

@PunkPun
Copy link
Member

PunkPun commented Dec 19, 2022

And limits the amount of wasted work / work to make it compatible again to a minimum

@pchote
Copy link
Member

pchote commented Dec 19, 2022

I think thats a bit of a shame, but ok 👍

@pchote pchote merged commit a0f17b1 into OpenRA:bleed Dec 19, 2022
@pchote
Copy link
Member

pchote commented Dec 19, 2022

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/222afde629b5e42878e5b65bb4203deb2a3e88f3

@dragunoff dragunoff deleted the feature/ftl-translation-ids branch December 19, 2022 09:45
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.

None yet

5 participants