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

Code Quality: Adding the possibility of plural translation using ICU format #15433

Merged
merged 9 commits into from
May 21, 2024

Conversation

XTorLukas
Copy link
Contributor

@XTorLukas XTorLukas commented May 20, 2024

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Testing plural implementation for text strings using ICU format
  2. Adding for all necessary strings

Screenshot
image
image

@XTorLukas XTorLukas marked this pull request as ready for review May 20, 2024 17:41
@XTorLukas
Copy link
Contributor Author

XTorLukas commented May 20, 2024

How to use it

Plural.ItemsSelected.Text -> {0, plural, one {# item selected} other {# items selected}}

// Auto incremental placeholder '0' 0,1,...
"Plural/ItemsSelected/Text".GetLocalizedFormatResource(selectedItems.Count);
// OR
"Plural/ItemsSelected/Text".GetLocalizedFormatResource(0.ToFormatPairs(selectedItems.Count));
// OR
"Plural/ItemsSelected/Text".GetLocalizedFormatResource("0".ToFormatPairs(selectedItems.Count));
// OR
"Plural/ItemsSelected/Text".GetLocalizedFormatResource(new Dictionary<string, object?>() { ["0"] = selectedItems.Count });

Results:

0 items selected
1 item selected
2 items selected
...
10 items selected

Plural.ItemsCount.Text -> {0, plural, one {item} other {items}}

// Auto incremental placeholder '0' 0,1,...
"Plural/ItemsCount/Text".GetLocalizedFormatResource(FilesystemViewModel.FilesAndFolders.Count);
// OR
"Plural/ItemsCount/Text".GetLocalizedFormatResource(0.ToFormatPairs(FilesystemViewModel.FilesAndFolders.Count));
// OR
"Plural/ItemsCount/Text".GetLocalizedFormatResource("0".ToFormatPairs(FilesystemViewModel.FilesAndFolders.Count));
// OR
"Plural/ItemsCount/Text".GetLocalizedFormatResource(new Dictionary<string, object?>() { ["0"] = FilesystemViewModel.FilesAndFolders.Count });

Results:

items
item
items
...
items

@XTorLukas
Copy link
Contributor Author

Okay I'm done, we could try just these two for now, we'll see how the translators handle it. I could finish the others in the next PR

@yaira2 yaira2 changed the title Feature: Adding the possibility of plural translation using ICU format Code Quality: Adding the possibility of plural translation using ICU format May 20, 2024
@XTorLukas
Copy link
Contributor Author

XTorLukas commented May 20, 2024

Okay I'm done, we could try just these two for now, we'll see how the translators handle it. I could finish the others in the next PR

It might be a good idea to add a link to the basic usage documentation in Crowdin's details: Crowdin ICU Message Syntax | Plural
image

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Codebase quality reviews.
If you’ve tested in en-US for them, LGTM.
Japanese doesn’t have this kind of difference(singular and plural).

src/Files.App/Strings/en-US/Resources.resw Outdated Show resolved Hide resolved
src/Files.App/Files.App.csproj Show resolved Hide resolved
@0x5bfa
Copy link
Member

0x5bfa commented May 21, 2024

I’m not sure why but there’re duplicated functionalities including RespurceString, GetLocalized, GetLocalizedResource, LocalizationService, and this.
We have to refactor…

@XTorLukas
Copy link
Contributor Author

XTorLukas commented May 21, 2024

Codebase quality reviews. If you’ve tested in en-US for them, LGTM. Japanese doesn’t have this kind of difference(singular and plural).

If not necessary, only 'other' is used https://www.unicode.org/cldr/charts/45/supplemental/language_plural_rules.html#ja

{0, plural, other {# アイテム}}

@XTorLukas
Copy link
Contributor Author

If you’ve tested in en-US for them,

I always test en-US (two options) and cs-CZ (three options) using plural

yaira2
yaira2 previously approved these changes May 21, 2024
@yaira2 yaira2 force-pushed the xtorlukas/CQ-AddMessageFormat branch from 3a2c78d to 0bdb970 Compare May 21, 2024 16:37
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 21, 2024
@yaira2 yaira2 merged commit e442bb7 into files-community:main May 21, 2024
5 checks passed
@XTorLukas XTorLukas deleted the xtorlukas/CQ-AddMessageFormat branch May 21, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants