refactor: migrate from iso_countries to l10n_countries #6541
Conversation
github-project-automation
bot
moved this to 💬 To discuss and validate
in 🤳🥫 The Open Food Facts mobile app (Android & iOS)
github-actions
bot
added
🥫 Product page
dependencies
✏️ Editing
There was a problem hiding this comment.
Hi @pavanlashkari!
Please have a look at my comments.
Let me write again my strong suggestions from #6523:
- create a specific file for the
3-letter-ISO-code (uppercase) / OpenFoodFactsCountryOpenFoodFactsCountry / 3-letter-ISO-code (uppercase) map - we'll have to move it to openfoodfacts-dart later - create a specific file where you put the
getEnglishNameandgetLocalizedNamemethods - probably in both files, using an
extensiononOpenFoodFactsCountrywould be appropriate
We'll probably need package l10n_countries only in one of those extension files.
There was a problem hiding this comment.
Very surprised by this file. We're supposed to use a package that does that for us, aren't we?
| @@ -0,0 +1,262 @@ | |||
| import 'package:openfoodfacts/openfoodfacts.dart'; | |||
|
|
|||
| const Map<String, OpenFoodFactsCountry> iso3ToCountry = { | |||
There was a problem hiding this comment.
We need it the other way around: Map<OpenFoodFactsCountry, String>
And please name it explicitly as a temporary class (e.g. tmpCountryIso3) as we'll eventually move part of the code to openfoodfacts-dart.
| 'ZMB': OpenFoodFactsCountry.ZAMBIA, | ||
| 'ZWE': OpenFoodFactsCountry.ZIMBABWE, | ||
| }; | ||
| class Country { |
There was a problem hiding this comment.
Not sure we need this class.
There was a problem hiding this comment.
Hey @monsieurtanuki 👋
I’m actually not sure what to do here, so just wanted to ask for some clarity.
The Country class we were using earlier came from the iso_countries package, and it handled both the country name and the code — which was helpful since the selector and provider logic rely on having both.
Now that iso_countries is removed and we’re not supposed to create our own Country class, I’m not sure what structure I should be using to replace it.
Should I just use OpenFoodFactsCountry directly and get the name from an extension like getLocalizedName(locale)?
Or is it okay to return something like (String name, String code) instead of a full class?
Just want to be sure before I rework the rest — happy to adjust based on what makes most sense to you.
There was a problem hiding this comment.
Hi @pavanlashkari!
- in openfoodfacts-dart and in Smoothie, our reference enum is
OpenFoodFactsCountry, that includes the 2-letter iso code - in Smoothie, we need localized country names, that's why we need to use a package like
iso_countriesorl10n_countries - if we use
l10n_countries, we need the 3-letter iso code to make the link withOpenFoodFactsCountry- that's why a solution may be to add the 3-letter iso code as an extension toOpenFoodFactsCountry(not a bad idea to enrich our reference data with standard codes when relevant)
That should be your starting point.
And then, link each OpenFoodFactsCountry with its localization in specific languages (local and English). That's what we need to display countries ordered by English name or local name.
| final String? iso3 = iso3ToCountry.entries | ||
| .firstWhere( | ||
| (entry) => entry.value == _country, | ||
| orElse: () => const MapEntry('', OpenFoodFactsCountry.UNITED_KINGDOM), |
packages/smooth_app/pubspec.yaml
Outdated
| @@ -36,7 +35,7 @@ dependencies: | |||
| photo_view: 0.15.0 | |||
| uuid: 4.5.1 | |||
| provider: 6.1.4 | |||
| sentry_flutter: 8.14.1 | |||
| sentry_flutter: 8.12.0 | |||
There was a problem hiding this comment.
| sentry_flutter: 8.12.0 | |
| sentry_flutter: 8.14.1 |
packages/smooth_app/pubspec.yaml
Outdated
| @@ -101,6 +100,7 @@ dependencies: | |||
|
|
|||
|
|
|||
| openfoodfacts: 3.20.0 | |||
| l10n_countries: ^1.0.0 | |||
There was a problem hiding this comment.
| l10n_countries: ^1.0.0 | |
| l10n_countries: 1.0.0 |
| @@ -28,6 +29,7 @@ class CountrySelector extends StatelessWidget { | |||
| this.inkWellBorderRadius, | |||
| this.loadingHeight = 48.0, | |||
| this.autoValidate = true, | |||
| super.key, | |||
There was a problem hiding this comment.
| super.key, |
There was a problem hiding this comment.
Thank you @pavanlashkari!
Your code goes in the right direction but there are still points to address; please read my comments.
packages/smooth_app/pubspec.yaml
Outdated
| @@ -101,6 +100,8 @@ dependencies: | |||
|
|
|||
|
|
|||
| openfoodfacts: 3.20.0 | |||
| l10n_countries: 1.0.0 | |||
| iso_countries: ^2.2.0 | |||
There was a problem hiding this comment.
The point of this PR is to get rid of iso_countries, isn't it?
packages/smooth_app/pubspec.yaml
Outdated
| @@ -101,6 +100,8 @@ dependencies: | |||
|
|
|||
|
|
|||
| openfoodfacts: 3.20.0 | |||
| l10n_countries: 1.0.0 | |||
There was a problem hiding this comment.
Please put it where iso_countries was.
Here is not a good idea at all, between two versions of openfoodfacts-dart (public and local).
| const CountrySelector( | ||
| {required this.forceCurrencyChange, | ||
| this.textStyle, | ||
| this.padding, | ||
| this.icon, | ||
| this.inkWellBorderRadius, | ||
| this.loadingHeight = 48.0, | ||
| this.autoValidate = true}); |
There was a problem hiding this comment.
| const CountrySelector( | |
| {required this.forceCurrencyChange, | |
| this.textStyle, | |
| this.padding, | |
| this.icon, | |
| this.inkWellBorderRadius, | |
| this.loadingHeight = 48.0, | |
| this.autoValidate = true}); | |
| const CountrySelector({ | |
| required this.forceCurrencyChange, | |
| this.textStyle, | |
| this.padding, | |
| this.icon, | |
| this.inkWellBorderRadius, | |
| this.loadingHeight = 48.0, | |
| this.autoValidate = true, | |
| }); |
Please don't change the format as:
- it forces me to review a code that hasn't been changed
- it's typically the way we format here, with a last comma
| @@ -122,8 +124,7 @@ class _CountrySelectorButton extends StatelessWidget { | |||
| SizedBox( | |||
| width: IconTheme.of(context).size! + LARGE_SPACE, | |||
| child: AutoSizeText( | |||
| EmojiHelper.getEmojiByCountryCode( | |||
| country.countryCode)!, | |||
| EmojiHelper.getEmojiByCountryCode(country.offTag)!, | |||
There was a problem hiding this comment.
May not work for UK, to be checked.
| final OpenFoodFactsCountry country, | ||
| ) async { | ||
| final UserPreferences userPreferences = context.read<UserPreferences>(); | ||
| final OpenFoodFactsCountry? offCountry = | ||
| OpenFoodFactsCountry.fromOffTag(country.countryCode); | ||
| OpenFoodFactsCountry.fromOffTag(country.offTag); |
There was a problem hiding this comment.
This makes no sense anymore: now we directly get the correct country, we don't have to tap dance around offTag.
| _suggestionsNotifier.value = const SimpleInputSuggestionsNoSuggestion(); | ||
| return; | ||
| } | ||
| _countries = OpenFoodFactsCountry.values; |
There was a problem hiding this comment.
| _countries = OpenFoodFactsCountry.values; |
| @@ -289,7 +290,8 @@ class _CountrySelectorScreen extends StatelessWidget { | |||
| Expanded( | |||
| flex: 7, | |||
| child: TextHighlighter( | |||
| text: country.name, | |||
| text: country | |||
| .getLocalizedName(AppLocalizations.of(context).localeName), | |||
There was a problem hiding this comment.
Please use ProductQuery.getLanguage() instead of localeName.
| OpenFoodFactsCountry? userCountry, | ||
| OpenFoodFactsCountry? selectedCountry, | ||
| String? filter, | ||
| BuildContext context) { |
There was a problem hiding this comment.
| BuildContext context) { | |
| ) { |
There was a problem hiding this comment.
Please remove context as we don't need it.
| country == userCountry || | ||
| country == selectedCountry || | ||
| country.name.toLowerCase().contains( | ||
| country | ||
| .getLocalizedName(AppLocalizations.of(context).localeName) |
There was a problem hiding this comment.
Please use ProductQuery.getLanguage() instead of localeName.
|
|
||
| return finalCountriesList; | ||
| } | ||
| static List<OpenFoodFactsCountry> _sanitizeAndSortCountries(String? locale) { |
There was a problem hiding this comment.
Please keep the same methods and method names.
You should create another extension to |
|
Thank you so much for patiently reviewing my code and guiding me through the changes. |
There was a problem hiding this comment.
Thank you @pavanlashkari!
We're close to a very good code!
Please have a look at all my comments.
| String get iso2Code => | ||
| offTag.toUpperCase() == 'UK' ? 'GB' : offTag.toUpperCase(); |
There was a problem hiding this comment.
| String get iso2Code => | |
| offTag.toUpperCase() == 'UK' ? 'GB' : offTag.toUpperCase(); | |
| String get iso2Code => | |
| this == OpenFoodFactsCountry.UNITED_KINGDOM ? 'GB' : offTag.toUpperCase(); |
Generally speaking it's not a brillant idea to copy code.
| @@ -431,14 +431,7 @@ class _ProductQueryPageState extends State<ProductQueryPage> | |||
| return null; | |||
| } | |||
| final String locale = Localizations.localeOf(context).languageCode; | |||
There was a problem hiding this comment.
Please use ProductQuery.getLanguage().offTag instead.
There was a problem hiding this comment.
And it's not a Future anymore.
| OpenFoodFactsCountry? userCountry, | ||
| OpenFoodFactsCountry? selectedCountry, | ||
| String? filter, | ||
| BuildContext context) { |
There was a problem hiding this comment.
Please remove context as we don't need it.
| _suggestionsNotifier.value = SimpleInputSuggestionsLoaded( | ||
| suggestions: <String>[country.name], | ||
| suggestions: country.getLocalizedName(locale) != null | ||
| ? <String>[country.getLocalizedName(locale)!] | ||
| : <String>[], | ||
| ); |
There was a problem hiding this comment.
Generally speaking it's not a brillant idea to copy code.
Please compute country.getLocalizedName(locale) only once.
|
|
||
| if (country == null || _terms.contains(country.name) == true) { | ||
| if (country == null || _terms.contains(country.getLocalizedName(locale))) { |
There was a problem hiding this comment.
Generally speaking it's not a brillant idea to copy code.
Please compute country.getLocalizedName(locale) only once.
| import 'package:smooth_app/pages/preferences/country_selector/tmp_country_iso3.dart'; | ||
|
|
||
| extension OpenFoodFactsCountryNameExtension on OpenFoodFactsCountry { | ||
| String? getLocalizedName(String locale) { |
There was a problem hiding this comment.
| String? getLocalizedName(String locale) { | |
| String? getLocalizedName(OpenFoodFactsLanguage language) { |
| } | ||
|
|
||
| String getEnglishName() => | ||
| getLocalizedName('en') ?? toString().split('.').last; |
There was a problem hiding this comment.
| getLocalizedName('en') ?? toString().split('.').last; | |
| getLocalizedName(OpenFoodFactsLanguage.ENGLISH) ?? toString().split('.').last; |
There was a problem hiding this comment.
This file isn't such a good idea after all.
Please delete this file and edit emoji_helper.dart instead: there we don't have to tap dance with gb GB uk UK, we can just use iso2code.
| return (a.getLocalizedName('en') ?? '') | ||
| .compareTo(b.getLocalizedName('en') ?? ''); |
There was a problem hiding this comment.
Please NEVER use magic values like 'en'.
OpenFoodFactsLanguage.ENGLISH would be more appropriate.
| (localizedCountries, userCountryCode), | ||
| ); | ||
|
|
||
| Future<List<OpenFoodFactsCountry>> onLoadValues() async { |
There was a problem hiding this comment.
Please keep the same methods with the same names here. I'm a bit lost in that code and I'm afraid you may skip interesting parts while trying to simplify the code.
There was a problem hiding this comment.
Thank you @pavanlashkari, I think we're very very close to the end!
Beyond my comments, there are 2 points that bother me:
- when we store the country code in the preferences, do we store
offTagoriso2code? Not specific to your code btw, it might have been an issue before. I think we should make it explicit in the code anyway, something like "we storeoffTagin the preferences, and the rest (e.g. iso2code) is for the display only" - it could be that using a specific class for storing countries and their translations wasn't such a bad idea, for performance reasons - here we frequently translate again and again, as opposed to one single call that translates everything. We may even get rid of the
compute, that looks like overkill, if the performances below are good enough
| _suggestionsNotifier.value = const SimpleInputSuggestionsLoaded( | ||
| suggestions: <String>[], | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| _suggestionsNotifier.value = SimpleInputSuggestionsLoaded( | ||
| suggestions: <String>[country.name], | ||
| suggestions: localizedName != null ? <String>[localizedName] : <String>[], |
There was a problem hiding this comment.
| suggestions: localizedName != null ? <String>[localizedName] : <String>[], | |
| suggestions: <String>[localizedName], |
| : _userCountry = OpenFoodFactsCountry.fromOffTag( | ||
| userPreferences.userCountryCode ?? '') ?? | ||
| OpenFoodFactsCountry.FRANCE; |
There was a problem hiding this comment.
| : _userCountry = OpenFoodFactsCountry.fromOffTag( | |
| userPreferences.userCountryCode ?? '') ?? | |
| OpenFoodFactsCountry.FRANCE; | |
| : _userCountry = OpenFoodFactsCountry.fromOffTag( | |
| userPreferences.userCountryCode) ?? | |
| OpenFoodFactsCountry.FRANCE; |
| final OpenFoodFactsCountry? country = _countries.firstWhereOrNull( | ||
| (OpenFoodFactsCountry country) => | ||
| country.iso2Code == _userCountry.iso2Code, | ||
| ); |
There was a problem hiding this comment.
| final OpenFoodFactsCountry? country = _countries.firstWhereOrNull( | |
| (OpenFoodFactsCountry country) => | |
| country.iso2Code == _userCountry.iso2Code, | |
| ); |
| final OpenFoodFactsLanguage locale = getLanguage(); | ||
| final String? localizedName = country?.getLocalizedName(locale); |
There was a problem hiding this comment.
| final OpenFoodFactsLanguage locale = getLanguage(); | |
| final String? localizedName = country?.getLocalizedName(locale); | |
| final String? localizedName = _userCountry.getLocalizedName(getLanguage()); |
|
|
||
| if (country == null || _terms.contains(country.name) == true) { | ||
| if (country == null || _terms.contains(localizedName)) { |
There was a problem hiding this comment.
| if (country == null || _terms.contains(localizedName)) { | |
| if (localizedName == null || _terms.contains(localizedName)) { |
| import 'package:smooth_app/pages/preferences/country_selector/tmp_country_iso3.dart'; | ||
|
|
||
| extension OpenFoodFactsCountryNameExtension on OpenFoodFactsCountry { | ||
| String? getLocalizedName(OpenFoodFactsLanguage locale) { |
There was a problem hiding this comment.
| String? getLocalizedName(OpenFoodFactsLanguage locale) { | |
| String? getLocalizedName(OpenFoodFactsLanguage language) { |
| EmojiHelper.getEmojiByCountryCode(country.iso2Code) ?? | ||
| '', |
There was a problem hiding this comment.
| EmojiHelper.getEmojiByCountryCode(country.iso2Code) ?? | |
| '', | |
| EmojiHelper.getCountryEmoji(country) ?? '', |
| @@ -273,14 +277,14 @@ class _CountrySelectorScreen extends StatelessWidget { | |||
| Expanded( | |||
| flex: 1, | |||
| child: Text( | |||
| EmojiHelper.getEmojiByCountryCode(country.countryCode) ?? '', | |||
| EmojiHelper.getEmojiByCountryCode(country.iso2Code) ?? '', | |||
There was a problem hiding this comment.
| EmojiHelper.getEmojiByCountryCode(country.iso2Code) ?? '', | |
| EmojiHelper.getCountryEmoji(country) ?? '', |
| ?.toLowerCase() | ||
| .contains(filter.toLowerCase()) ?? | ||
| false) || | ||
| country.offTag.toLowerCase().contains( |
There was a problem hiding this comment.
offTag or iso2code? Or both?
| return OpenFoodFactsLanguage.values.firstWhere( | ||
| (lang) => lang.code == code, | ||
| orElse: () => OpenFoodFactsLanguage.ENGLISH, | ||
| ); |
There was a problem hiding this comment.
| return OpenFoodFactsLanguage.values.firstWhere( | |
| (lang) => lang.code == code, | |
| orElse: () => OpenFoodFactsLanguage.ENGLISH, | |
| ); | |
| return OpenFoodFactsLanguage.fromOffTag(code); |
|
Hey @monsieurtanuki, Also just pushed the latest changes based on your feedback — let me know if I missed anything! |
|
Thank you for your work @pavanlashkari! Honestly, I think we'd be much better off with a new
With that I think we'll cover all the needs. |
|
Hello @monsieurtanuki, Just a small note: I’ve intentionally continued using country.offTag instead of preferenceCode, in places where we’re already working directly with OpenFoodFactsCountry. This helps avoid unnecessary indirection and keeps the code simpler. Of course, if you’d prefer consistency with LocalizedCountry.preferenceCode, I’ll be happy to update it accordingly. |
There was a problem hiding this comment.
Hi @pavanlashkari, and thank you for your code!
I'm not sure anymore about where to put the "English fallback name", but first I really need you to
- switch both "country selector" files to use "LocalizedCountry" instead of "OpenFoodFactsCountry"
- explicitly refactor method
_sanitizeCountriesListincountry_select_provider.dart. You may have skipped relevant steps when trying to code quicker.
/// Sanitizes the country list, but without reordering it.
/// * by removing countries that are not in [OpenFoodFactsCountry]
/// * and providing a fallback English name for countries that are in
/// [OpenFoodFactsCountry] but not in [localizedCountries].
static List<Country> _sanitizeCountriesList(Please have a look at my other comments too.
| bool selected, | ||
| String filter, | ||
| ) { | ||
| final LocalizedCountry? lc = localizedCountries |
There was a problem hiding this comment.
Generally speaking, don't name variables with those short names.
We can afford more letters, can't we?
I assumed "lc" was "languageCode"...
|
|
||
| final String? localized = localizedMap[iso3]; | ||
|
|
||
| String fallbackEnglish = |
There was a problem hiding this comment.
Misunderstanding: the fallback in English should be the fallback, in English, when the English localization isn't found.
You're supposed to call mapper.localize in English too.
|
|
||
| String get preferenceCode => country.offTag; | ||
|
|
||
| static List<LocalizedCountry> getLocalizedCountries() { |
There was a problem hiding this comment.
Things would be much simpler if you returned a Map<OpenFoodFactsCountry, LocalizedCountry>, wouldn't they?
| return localized; | ||
| } | ||
|
|
||
| final String fallbackEnglish = |
There was a problem hiding this comment.
Please don't duplicate code. Make the getFallbackEnglishName a method used in both methods.
There was a problem hiding this comment.
In that file it really make no sense using LocalizedCountry.
Switch back to OpenFoodFactCountry and offTag.
| const CountriesHelper._(); | ||
| Future<void> onSaveItem(OpenFoodFactsCountry country) => | ||
| preferences.setUserCountryCode( | ||
| country.offTag); // Save offTag (not iso2code) in preferences |
There was a problem hiding this comment.
With your new code we slightly change the value (from ISO2CODE to offTag, including "GB" / "uk") but that shouldn't matter that much as OpenFoodFactsCountry.fromOffTag is quite resilient.
Eventually remove the comment: let's pretend we always used offTag.
| static void _reorderCountries( | ||
| List<Country> countries, | ||
| List<OpenFoodFactsCountry> countries, |
There was a problem hiding this comment.
If we reorder LocalizedCountrys instead it's much clearer, I think.
| (localizedCountries, userCountryCode), | ||
| ); | ||
|
|
||
| Future<List<OpenFoodFactsCountry>> onLoadValues() async { |
pavanlashkari
force-pushed
the
refactor/l10n-countries-migration
branch
from
3a6cf43 to
73d1bd5
Compare
|
Hey! 👋 I just updated the commit author from my work account (@pavaniroid) to my personal account (@pavanlashkari) since this is my personal fork and my work account might be deactivated in the future. There are no changes in the actual code — just amended the commit author info and force-pushed. Thanks for the review! 😊 |
What
iso_countriestol10n_countriesiso3ToCountrymap to handle ISO 3166-1 alpha-3 code toOpenFoodFactsCountrymappingOpenFoodFactsCountryNameExtensionfor English and localized country name retrievalCountriesHelperto useCountriesLocaleMapperfor locale-based namingiso_countriesdependenciesFixes bug
iso_countriesplugin must be replaced #6523