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 #648: allow using "old" (e.g. pre-3.0.4) plural rules evaluation #668

Merged
merged 7 commits into from
May 9, 2024

Conversation

mauriziopinotti
Copy link
Collaborator

Add forcePluralCaseFallback option to force evaluation of fallback plural rules, i.e.

forcePluralCaseFallback: false
Default behavior, will use "zero" rule for 0 only if the language is set to do so (e.g. for "lt" but not for "en").

forcePluralCaseFallback: true
Force using "zero" rule for 0 even if the language doesn't use it by default (e.g. "en"). If "zero" localization for that string doesn't exist, "other" is still used as fallback.

…ation

Add forcePluralCaseFallback option to force evaluation of fallback
plural rules, i.e.

* forcePluralCaseFallback: false
Default behavior, will use "zero" rule for 0 only if the language
is set to do so (e.g. for "lt" but not for "en").

* forcePluralCaseFallback: true
Force using "zero" rule for 0 even if the language doesn't use it
by default (e.g. "en"). If "zero" localization for that string
doesn't exist, "other" is still used as fallback.
@bw-flagship
Copy link
Collaborator

Why would we allow modifying a language's plural behavior? What is the use case?

@mauriziopinotti
Copy link
Collaborator Author

Why would we allow modifying a language's plural behavior? What is the use case?

This is to restore the old behavior that made sense in a lot of use cases, where "zero" and "one" have better (more meaningful) variations than "other".

For example:

  "label_members": {
    "zero": "nobody",
    "one": "{} member",
    "two": "{} members",
    "other": "{} members"
  },

Here, "nobody" is better than "0 members" but without this fix I can't have it in English.
Also, this would create inconsistencies between English (displaying "0 members" and other languages such Latvian (displaying "nobody").

Another example:

  "label_attachments_count": {
    "zero": "No attachments",
    "one": "One single attachment",
    "two": "{} attachments",
    "other": "{} attachments"
  },

Here I have special versions of "zero" and "one" that would sound better for the user.

@bw-flagship
Copy link
Collaborator

Thanks for the explanation!

I understand why that's helpful for your situation.

However, it feels wrong to do so because it missuses plurals.

Imho the correct solution would be:

  {
  "label_members_none": "nobody",
  "label_members": {
    "one": "{} member",
    "other": "{} members"
  },

Open for opinions, what do the others think?

@mauriziopinotti
Copy link
Collaborator Author

mauriziopinotti commented Apr 29, 2024

Thanks for the explanation!

I understand why that's helpful for your situation.

However, it feels wrong to do so because it missuses plurals.

Imho the correct solution would be:

  {
  "label_members_none": "nobody",
  "label_members": {
    "one": "{} member",
    "other": "{} members"
  },

Open for opinions, what do the others think?

But the usage of this would be

final string = qty == 0 ? LocaleKeys.label_members_none.tr() : LocaleKeys.label_members_none.plural(qty);

which is a bit unreadable and it gets even worst with the second scenario:

final string = qty == 0 ? LocaleKeys.label_members_none.tr() : 
    qty == 1 ? LocaleKeys.label_members_one.tr() : 
        LocaleKeys.label_members_none.plural(qty);

Since the behavior is behind an optional flag I think it's good to let developers choose: if anyone disagrees they just have to do nothing, because out-of-the-box the behavior is the "correct" one.

Also notice that for the end user it's totally transparent: users will just see the desired text and they don't care how it's built under the hood.

@bw-flagship
Copy link
Collaborator

Well, what is the scope of easy_localization?

Your goal is to show two entirely different texts based on the count of a variable.
"nobody" is not a plural form of "{} members", it is a different text.
Lets not abuse the plural engine for this.

I would not want to make this a feature of easy_localization.

@mauriziopinotti
Copy link
Collaborator Author

Well, what is the scope of easy_localization?

Your goal is to show two entirely different texts based on the count of a variable. "nobody" is not a plural form of "{} members", it is a different text. Lets not abuse the plural engine for this.

I would not want to make this a feature of easy_localization.

Well, the original issue has 5 thumb-ups so I'm clearly not the only one that feels this is a regression...

@mauriziopinotti
Copy link
Collaborator Author

mauriziopinotti commented May 4, 2024

Furthermore, the official flutter packages behave in the same way, for example if I use

{
  "pushed": "You have pushed the button this many times:",
  "nTimes": "{count, plural, =0{never} =1{once} other{{count} times}}"
}
Text(AppLocalizations.of(context)!.nTimes(_counter)),

Here's the output:

For "0":
image

For "1":
image

For "2":
image

So, the previous behavior was legit.

Full example: https://github.com/easyhour/easy_localization/tree/i18n_demo

Official Flutter docs: https://docs.flutter.dev/ui/accessibility-and-internationalization/internationalization

@bw-flagship
Copy link
Collaborator

Thanks for elaborating on this. The argument that the official flutter translations implementation has this behavior is very convincing.
One thing I would suggest is to rename the property and call it
ignorePluralRules instead of forcePluralCaseFallback

This would make the behavior more clear imho. Wdyt @mauriziopinotti ?

@mauriziopinotti
Copy link
Collaborator Author

Thanks for elaborating on this. The argument that the official flutter translations implementation has this behavior is very convincing. One thing I would suggest is to rename the property and call it ignorePluralRules instead of forcePluralCaseFallback

This would make the behavior more clear imho. Wdyt @mauriziopinotti ?

Sure, thanks, I'll submit an updated patch in the next days!

Parameter forcePluralCaseFallback has been renamed to ignorePluralRules
for clarity as suggested by bw-flagship. Also, the logic is now reversed
(e.g. false will implement the old behavior and true will implement the
new behavior). Finally, the default value is now false (old behavior).
@mauriziopinotti
Copy link
Collaborator Author

Hi @bw-flagship , I've pushed a new commit with the renamed parameter.

While working on this I had two doubts

  1. Should also the logic be reversed, i.e. before forcePluralCaseFallback=true is the modified behavior but now it seems that the modified behavior should be ignorePluralRules=false.
  2. Should the default value be the pre-3.0.4 evaluation?

So, to clarify, now the patch works like this:

  • By default, the behavior is the pre-3.0.4 one: "none, once, 2 times" (ignorePluralRules=false)
  • Passing ignorePluralRules=true will cause the after-3.0.5 behavior: "0 times, 1 times, 2 times"

Let me know if this is OK or more changes are needed.

Thanks!

@@ -114,6 +119,9 @@ class Localization {
}

static PluralRule? _pluralRule(String? locale, num howMany) {
if (!instance._ignorePluralRules) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instance._ignorePluralRules

@bw-flagship
Copy link
Collaborator

@mauriziopinotti I would reverse it:

ignorePluralRules is for me the same as ForceUsingFallback

ignorePluralRules = true should cause the behavior if 3.0.4 and earlier (and can be default)
ignorePluralRules = false should cause the "new" 3.0.5 behavior

ignorePluralRules=true, "old" beahvior (default)
ignorePluralRules=false, "new" beahvior
@mauriziopinotti
Copy link
Collaborator Author

@mauriziopinotti I would reverse it:

ignorePluralRules is for me the same as ForceUsingFallback

ignorePluralRules = true should cause the behavior if 3.0.4 and earlier (and can be default) ignorePluralRules = false should cause the "new" 3.0.5 behavior

OK, it should be good now.

lib/src/localization.dart Outdated Show resolved Hide resolved
lib/src/localization.dart Outdated Show resolved Hide resolved
lib/src/localization.dart Show resolved Hide resolved
@bw-flagship
Copy link
Collaborator

the logic looks good! Found some formating things and you need to integrate the latest dev changes, thanks!

@bw-flagship bw-flagship merged commit 977a72f into aissat:develop May 9, 2024
4 checks passed
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.

2 participants