aissat / easy_localization

Easy and Fast internationalizing your Flutter Apps
https://pub.dev/packages/easy_localization
MIT License
908 stars 325 forks source link

Fix #648: allow using "old" (e.g. pre-3.0.4) plural rules evaluation #668

Closed mauriziopinotti closed 4 months ago

mauriziopinotti commented 5 months ago

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 commented 5 months ago

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

mauriziopinotti commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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!

mauriziopinotti commented 5 months ago

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:

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

Thanks!

bw-flagship commented 5 months ago

@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

mauriziopinotti commented 5 months ago

@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.

bw-flagship commented 5 months ago

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