Skip to content

Conversation

@asashour
Copy link
Contributor

@asashour asashour commented Jul 9, 2021

This is based on top of #85116 (since the expectations build on each other).

Addresses some of the issues raised in #75094
Fixes #81981

It supports plurals and select when they are inside a text.

Please note that the previous test case for helloWorldsInterpolation for ES was wrong, since we didn't process the prefix string, but now we do. This is also fixed in this PR.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 9, 2021
@google-cla google-cla bot added the cla: yes label Jul 9, 2021
@ened
Copy link
Contributor

ened commented Jul 14, 2021

It looks like this PR would also help with #81981.

"helloWorldPopulation": "{ES - count,plural, =1{ES - Hello World of {population} citizens} =2{ES - Hello two worlds with {population} total citizens} many{ES - Hello all {count} worlds, with a total of {population} citizens} other{ES - Hello other {count} worlds, with a total of {population} citizens}}",
"helloWorldInterpolation": "ES - [{hello}] #{world}#",
"helloWorldsInterpolation": "ES - {count,plural, other {ES - [{hello}] -{world}- #{count}#}}",
"helloWorldsInterpolation": "{count,plural, other {ES - [{hello}] -{world}- #{count}#}}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this been changed?

Copy link
Contributor Author

@asashour asashour Jul 14, 2021

Choose a reason for hiding this comment

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

Please note that the previous test case for helloWorldsInterpolation for ES was wrong, since we didn't process the prefix string, but now we do.

If this wasn't changed, we need to change the expectations to include this added "ES - ", but this scenario with "ES" is tested with pluralInString.

@asashour asashour force-pushed the 75094_nested_plurals branch from 58543f1 to 13f6eec Compare July 14, 2021 15:19
@Delgan
Copy link

Delgan commented Jul 14, 2021

Hey there!

So the other day I discovered this great PR by @asashour which helped me a lot implementing internationalization in my Flutter app!

On my way, I encountered a few "gotchas". For example, defining a string containing both plural and normal placeholder generate an invalid output.
I first planned to patch and submit as an update to this PR.... Before I realized that I actually had to change the whole formatting logic with the aim of providing even more effective internationalization.

I based my work on the this PR which led me to the following commit: https://github.com/Delgan/flutter/commit/1398287af59c0f49ae445606e896e17f098f819b

Due to the refactor I made, I guess these modifications need to be discussed further.
Do you think I should open a new pull request towards flutter:master or towards asashour:75094_nested_plurals?

I just want to avoid duplicating the review and correction work if it is already taken into account in the commit I proposed. 👍

@asashour asashour force-pushed the 75094_nested_plurals branch from 090cfdc to 0976fd9 Compare July 15, 2021 07:40
@zanderso zanderso requested a review from HansMuller July 15, 2021 20:52
@asashour asashour mentioned this pull request Jul 15, 2021
8 tasks
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

This looks like a huge improvement!

Unless I've overlooked something (quite possible), we seem to be missing a test for the case shown in the original issue: #75094 (comment). In that case the plural's count argument specifies a format so I believe the formatted count string should be used for substitutions in the plural.

@asashour asashour force-pushed the 75094_nested_plurals branch from 0976fd9 to a705d59 Compare July 17, 2021 07:24
@asashour
Copy link
Contributor Author

asashour commented Jul 17, 2021

Actually, this PR doesn't change the behavior of the plural, there are already many tests for it, e.g.

"helloWorlds": "{count,plural, =0{Hello} =1{Hello World} =2{Hello two worlds} few{Hello {count} worlds} many{Hello all {count} worlds} other{Hello other {count} worlds}}",

The PR only handles the strings around it. On the other hand, it also adds the functionality of select.

But there is no harm to change the new tests to match that, which was done.

Another thing discovered in the last commit, we should escape the variable if the suffix can be merged with it, e.g.

    return 'ES - Oh, she found ${pluralString}ES - !';

instead of

    return 'ES - Oh, she found $pluralStringES - !';

@asashour asashour force-pushed the 75094_nested_plurals branch 3 times, most recently from 8b5a54b to 9a38656 Compare July 17, 2021 08:09
@asashour asashour force-pushed the 75094_nested_plurals branch from 797670d to ffc2048 Compare July 17, 2021 19:02
@HansMuller
Copy link
Contributor

@asashour - @jonahwilliams provided some feedback on #85116 - #85116 (review) which may still apply here.

@asashour
Copy link
Contributor Author

This looks good; just some random formatting stuff to deal with. That and the problem noted in #86167 (review) which is OK to handle in a separate PR.

Thanks for your feedback.

If I understand correctly, there is a case

"pluralInString": "Oh, she found {count, plural, =1 {1 item} other {all {count} items} }!",

in which other has {count} variable, and this is what I understood from the comment in the original case.

If this is not enough, please put a comment/issue with a test case, and cc/assign me.

@asashour
Copy link
Contributor Author

Thanks @jonahwilliams for feedback, I also changed already existing tests for the uneeded try/catch.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

This is a nice improvement to the i18n system, thanks for sticking with it!
We'll need to update the i18n user's guide - flutter.dev/go/i18n-user-guide - to cover the new features.

@HansMuller
Copy link
Contributor

@asashour - Per #86167 (comment), I changed "fixes" to "addresses some of the issues" when referring to
#75094 because, AFAICT, the countString substitution needed by the sample in that issue isn't covered by the changes here.

If you're willing to tackle the rest of #75094, please assign yourself to it. I tried to do so just now; for some reason GitHub wouldn't allow it.

@asashour
Copy link
Contributor Author

Now I see the issue, it is whether count or countSring in

  other: 'all ${count} items',

I would work on it, should be delivered in 24 hours, either in this PR (if not merged), or another one.

It seems that issues can not be assigned to non-team members.

@HansMuller
Copy link
Contributor

Wow, we certainly would appreciate that. I think a separate PR would be best, since this one is already queued up for a landing.

@fluttergithubbot fluttergithubbot merged commit f568d92 into flutter:master Jul 21, 2021
@ened
Copy link
Contributor

ened commented Jul 21, 2021

@HansMuller any chance this PR can make it to the next (upcoming) stable version?

@HansMuller
Copy link
Contributor

The next stable release is a ways off, but yes it should be part of that.

@asashour
Copy link
Contributor Author

@Delgan

Since this is merged, you can raise another PR to discuss your changes.

I guess the next big thing is supporting plurals inside selects and vice versa. I guess regular expressions wouldn't help much, and possibly a parser needs to be built, but I think this should be discussed with the team before putting efforts.

@Knupper
Copy link

Knupper commented Jul 22, 2021

@HansMuller a hot fix for the current releases are not planned? This is the recommended way for multi language in flutter and it would be helpful to support all tags there as fast as possible, because the intl_translation package can not added as dependencie to flutter2 projects trough incompatible intl packages and null saftey.

Thank you @asashour for your effort to this pull request.

@Delgan
Copy link

Delgan commented Jul 22, 2021

@asashour Thanks for your answer. You nailed it: I had to replace regexes with mini-parser to implement my POC for multiple/nested select and plurals. This change is not trivial and I will open a ticket to discuss it, now that your PR is merged. Thanks again for your work. ;)

@HansMuller
Copy link
Contributor

@Knupper - A new beta release is due out any minute now and it's possible that would could apply this PR (and #86842) as a hot-fix to that. Would that help, or do you need to wait for a stable release?

@Knupper
Copy link

Knupper commented Jul 23, 2021

@HansMuller this would help to verify that it now works correctly with our use cases. But we are not going to a beta release for our product so we need to wait until it is part of a stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[proposal] gen_l10n should support select

6 participants