Skip to content

Conversation

nielsenko
Copy link

Make the Locale a member field on the generated localization class, and use its canonicalized name when invoking Intl.message. This allows clients to handle translations for multiple locales, using different instances of the generated localization class.

and use its canonicalized name when invoking Intl.message.
This allows clients to handle translations for multiple locales, using
different instances of the generated localization class.
@aleksakrstic
Copy link

Hi @nielsenko, thanks for the PR!

Seems this creates a breaking change on how some API is used till now, for example how you can get current locale (till now it was by calling Intl.getCurrentLocale()).

Maybe to postpone this change till the next major version release

@aleksakrstic
Copy link

Linked issue from IntelliJ IDE plugin: localizely/flutter-intl-intellij#26

@nielsenko
Copy link
Author

You can still get the current locale by calling Intl.getCurrentLocale. load works the same as before. But now you have the option, if you wish, to hold onto a previously loaded instance with a different locale than the current.

@Claudishe
Copy link

+1 , merging this would indeed solve the multiple localization issue.

@lzoran
Copy link
Collaborator

lzoran commented Jan 21, 2021

Hi @nielsenko, @Claudishe,

Thank you for your contribution and feedback!

I've tested the PR code, and it looks like a solution for the multiple localization issue.
The problem with this approach is that it will create a breaking change for existing users. So far, we were suggesting usage of S.load(Locale(...)); for programmatically changing a locale within an app and usage of S.of(context).stringKey for referencing the keys in the Dart code. With this merge, all users who followed our docs will have a problem, and their code won't work.

I agree with the @aleksakrstic that we should postpone this change till the next major version release as it solves the mentioned issue, and maybe to try to find a way to ease this transition.

In case I missed something, or if there is a way to avoid mentioned breaking change, please let us know.

@nielsenko
Copy link
Author

Hi @lzoran

Thanks for your reply, and sorry for the delayed response.

Yes, I noticed that you advocate calling S.load(newLocale) directly, which in turn sets Intl.defaultLocale to the new locale *, and that many users are probably following this advice. I think it is more fluttery to manipulate the MaterialApp. localeListResolutionCallback to return the new locale, or to inject a Localization widget with the new locale explicitly in your widget tree. If you do that, then all S.of(context).xxx calls will work fine, and this PR will not cause a breaking change.

However, since you ask explicitly, howto allow direct S.load(newLocale) calls to work (which obviously doesn't cause the nearest Localization widget to change its locale), and still have S.of(context) (which is really just a wrapper around Localization.of<S>(context, S) return an S that uses the Intl.defaultLocale instead of the one given on load, then one way could be to hide the new behaviour behind a static configuration flag on S, and let users opt-in explicitly.

The idea is to ensure that the field localeName is set to null depending on a new static boolean property S.trackIntlDefaultLocale that defaults to true. If trackIntlDefaultLocale is false, then store the canonicalized name in localeName otherwise leave it as null. In that case the behaviour is identical to before, since the previously omitted locale parameter on the calls to Intl.message defaults to null anyway, and users can opt-in to the new behaviour, by setting S.trackIntlDefaultLocale = false before using S.of(context) the first time**.

If you agree with this idea, then I can implement it tomorrow. It is a very minor change.

* Actually I don't like that S.load sets Intl.defaultLocale at all, but that is a discussion for another day.

** I'm not sure trackIntlDefaultLocale is the best name, but I have yet to come up with a better one.

@aleksakrstic
Copy link

Hi @nielsenko

Using Localization widget in the widget tree sounds like a solution. Though localeListResolutionCallback seems to be useful just for initial locale resolution based on device's configuration.

I am more for using something like flutter_intl.version: 2 param in pubspec.yaml for configuring how to generate the code, and to allow next major version (2) to use just one approach (proposed in this PR). That way we might document latest version in the current links, and create old approach documentation in additional links. Otherwise users of this package will be definitely easily confused with different approaches.

I am for leaving this PR open till we come to the moment to release next major version (date not yet known), since the PR contains useful code changes.

@narumi147
Copy link

narumi147 commented Mar 13, 2022

Some complain:
"Waiting for next major version" has been told more than 1 year, flutter has already been changed a lot. But this one did keep telling waiting for next major version without any prerelease or related commit. This pr is not difficult to modify and merge into beta branch.


https://github.com/chaldea-center/intl_utils
This folk aims to solved two problem:

1. multi-localization

simply modified the solution of @nielsenko . Here are some changes:

  • canonicalizedName call toString() directly when scriptCode is non-null, alos Intl.canonicalizedLocale cannot handle script code. May be improved furthermore.
  • S.load(Locale locale, {bool override = false}) add a param to decide whether to override S._current. If users access from S.of(context), nothing changed. But if you get used to use S.current, you need to manually call S.load(locale, override:true) to override the static _current.
  static Future<S> load(Locale locale, {bool override = false}) {
    final localeName = locale.canonicalizedName;
    return initializeMessages(localeName).then((_) {
      final localizations = S(locale);
      if (override || S._current == null) {
        Intl.defaultLocale = localeName;
        S._current = localizations;
      }
      return localizations;
    });
  }

To get current locale:

  • global locale: Intl.getCurrentLocale() and S.current.locale/localeName should work.
  • context locale: Localizations.localeOf(context)

2. hot-reload

See:

add a function called reloadMessages in messages_all.dart

Map<String, LibraryLoader> get _deferredLibraries => {
      'ar': () => new Future.value(null),
      'en': () => new Future.value(null),
      'es': () => new Future.value(null),
      'ja': () => new Future.value(null),
      'ko': () => new Future.value(null),
      'zh': () => new Future.value(null),
      'zh_Hant': () => new Future.value(null),
    };

Future<void> reloadMessages() async {
  for (final lib in _deferredLibraries.values) {
    await lib();
  }
  messages_ar.messages.messages.clear();
  messages_ar.messages.messages.addAll(messages_ar.MessageLookup().messages);
  messages_en.messages.messages.clear();
  messages_en.messages.messages.addAll(messages_en.MessageLookup().messages);
  // more languages...
}

Changes:

  • _deferredLibraries now is a getter not final. When you add/delete language or change defer-loading behaviour, a final field will never update.
  • if you want to automatically reloadMessages when hot-reload, call it in your root StatefuleWidget's reassemble method. reassemble only called in debug mode, so don't worry in release mode.
// root StatefuleWidget
  @override
  void reassemble() {
    super.reassemble();
    reloadMessages();
  }

Sure, if you are using vscode/jetbrain plugin, you should change to use something like File Watcher to watch l10n directory and run flutter pub run intl_utils:generate.

@goranluledzija
Copy link
Contributor

Hi @narumishi
I understand, but code changes are just part of all changes needed for such releases.
And it is not yet good moment for those.
Appreciate your contribution!

@ThexXTURBOXx
Copy link

ThexXTURBOXx commented May 4, 2022

I will join the complaint bandwagon on this issue. I have been searching for a solution to this issue for a month now since I did not think about the underlying issue being in this package.
A few hours ago, I finally found this issue: flutter/flutter#40353
It links to this PR: flutter/website#3013
Changing the following part of the official documentation: https://docs.flutter.dev/development/accessibility-and-localization/internationalization#defining-a-class-for-the-apps-localized-resources
As you can see, it is not good practice to create a singleton in this case (most of the time, singletons are considered anti-patterns anyway... So it is a good time to finally get rid of them!).
Accordingly, I fixed the issue myself: ThexXTURBOXx@4504de3
It was only after that that I found this PR and wonder why this has not been merged in over a year!
I am pretty sure that I am not the only person who has spent such a considerable amount of time trying to find the underlying issue.

I disagree with the overall opinion here that this PR is not worthy being merged yet because a new major release would be needed to announce the breaking change.
Yes, a new major release is definetly needed, but also kind of inevitable.
What stops us from just merging either this PR or (even better) @narumishi's changes and publishing them in a new major release?
Their branch does not remove functionality, but rather fixes a (in my eyes) major bug with the current system.
Yes, some code changes might be needed in some applications that use this library, but a major release stops automatic upgrades anyway.
So, noone will be able to complain about such changes anyways.

Please, fix this issue already!

@Knupper
Copy link

Knupper commented Dec 17, 2024

Any news on this bug? It is still an issue and it would be great if it can be fixed.

@ThexXTURBOXx
Copy link

I am still actively maintaining my fork (and hence this PR) as I think this is the right way to fix this issue. Ever since opening my PR, I am using my fork instead of the official package.
Alas, I believe that this will never get merged into the official version

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.

8 participants