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

feat(mobile): exclude locales from overpass font #14158

Conversation

johnstef99
Copy link
Contributor

@johnstef99 johnstef99 commented Nov 15, 2024

Fixes #10436

As mentioned here this PR adds a list of languages that should be excluded from using Overpass as a font. These languages will use the default Flutter font based on the platform.

As of right now the only known to me language having a problem is Greek, feel free to exclude more if needed.

@alextran1502 alextran1502 changed the title feat(mobile) exclude locales from overpass font feat(mobile): exclude locales from overpass font Nov 15, 2024
navigatorObservers: () => [TabNavigationObserver(ref: ref)],
return ProviderScope(
overrides: [
localeProvider.overrideWithValue(context.locale),
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this overrides property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sets the localeProvider value for the rest subtree of widgets every time that context.locale gets updated. Do you have an alternative way of settings the initial value of localeProvider and then updating it?

@VelocityRa
Copy link
Contributor

VelocityRa commented Nov 15, 2024

As of right now the only known to me language having a problem is Greek, feel free to exclude more if needed.

Serbian Cyrillic has the issue too: #11216, could be added now.
Might also want to tweak the filtering to take the country code into account too - Serbian Cyrillic is specifically Locale('sr', 'Cyrl'), there's Serbian Latin too which presumably renders fine (I asked on the issue to make sure).

@johnstef99
Copy link
Contributor Author

As of right now the only known to me language having a problem is Greek, feel free to exclude more if needed.

Serbian Cyrillic has the issue too: #11216, could be added now. Might also want to tweak the filtering to take the country code into account too - Serbian Cyrillic is specifically Locale('sr', 'Cyrl'), there's Serbian Latin too which presumably renders fine (I asked on the issue to make sure).

Ok I will update it to filter with region/country subtag and exclude Locale('sr', 'Cyrl')

@alextran1502
Copy link
Contributor

The localProvider isn't used correctly here, as the value never updates. The theme is already initialized when the locale changes, so you don't need the provider.

You can achieve the same effect by removing the localProvider across the app then changing the getThemeData method signature to the following

image

@johnstef99
Copy link
Contributor Author

Have you tested that the localeProvider's value never changes? Because I can't reproduce it. When the EasyLocalization parent widget rebuilds the localeProvider's value gets updated.

Let me explain why I added this provider:

  1. In the MapThemeOveride widget we also need the locale to get the theme data. If conext.locale was used here, it would be difficult for the widget to be tested (and there are already written tests for this widget). The already written test will have to be refactored to have as parent widget the EasyLocalization.
  2. I have opened an other MR that also needs the localeProvider for the reason that some UI elements are cached by providers. That results on not refreshing the UI after the user changing language.

@johnstef99 johnstef99 force-pushed the feat/mobile-exclude-locales-from-overpass-font branch from d0118bc to de5f7b3 Compare November 17, 2024 23:19
@alextran1502 alextran1502 merged commit bcd17c2 into immich-app:main Nov 19, 2024
33 checks passed
yosit pushed a commit to yosit/immich that referenced this pull request Nov 21, 2024
* feat(mobile): create localeProvider

This provider can be used to refresh providers that provide UI elements
and get cached.

* feat(mobile): use default font for locales not supported by Overpass

* chore(mobile): fix test

* refactor(mobile): use Locale instead of String
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Greek font rendering incorrectly in Immich Mobile App
3 participants