WIP: [android] Implement Light, Dark, and System modes on android(Auto) #8309

rtsisyk · 2024-05-27T15:20:34Z

SRSAS commented

2024-05-27 15:20:34 +00:00

(Migrated from github.com)

closes #749

Changed from the existing night modes (on, off, auto), to "appearence" modes:
- Light;
- Dark;
- System;

System mode follows whatever mode the user's device is in. This implementation works for both normal android and AndroidAuto.

Main changes:
- added necessary strings to XML files;
- changed the position and option names of the "map style preferences";
- implemented system mode, and some helper methods on ThemeSwitcher.java and ThemeUtils.java;
- added uiMode changes detection to all activities;
- removed "auto" mode implementation;

Co-authored-by: Francisco Nael Salgado francisco.nael.salgado@tecnico.ulisboa.pt

closes #749 Changed from the existing night modes (on, off, auto), to "appearence" modes: - Light; - Dark; - System; System mode follows whatever mode the user's device is in. This implementation works for both normal android and AndroidAuto. Main changes: - added necessary strings to XML files; - changed the position and option names of the "map style preferences"; - implemented system mode, and some helper methods on ThemeSwitcher.java and ThemeUtils.java; - added uiMode changes detection to all activities; - removed "auto" mode implementation; Co-authored-by: Francisco Nael Salgado <francisco.nael.salgado@tecnico.ulisboa.pt>

🎉 2

biodranik (Migrated from github.com) reviewed 2024-05-27 20:59:49 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! It would be better to merge this PR first, and implement "Follow the sun" behavior separately, if needed.

android/app/src/main/AndroidManifest.xml

biodranik (Migrated from github.com) commented

2024-05-27 20:52:52 +00:00

Is there a way to automatically add it to any activity without explicitly specifying everywhere?

android/app/src/main/java/app/organicmaps/SplashActivity.java

					
				@ -103,6 +104,13 @@ public class SplashActivity extends AppCompatActivity

				    mPermissionRequest = null;

biodranik (Migrated from github.com) commented

2024-05-27 20:53:08 +00:00

  public void onConfigurationChanged(@NonNull Configuration newConfig)
  {

```suggestion public void onConfigurationChanged(@NonNull Configuration newConfig) { ```

android/app/src/main/java/app/organicmaps/base/BaseMwmFragmentActivity.java

					
				@ -128,6 +129,14 @@ public abstract class BaseMwmFragmentActivity extends AppCompatActivity

				    }

biodranik (Migrated from github.com) commented

2024-05-27 20:53:28 +00:00

  public void onConfigurationChanged(@NonNull Configuration newConfig)
  {

```suggestion public void onConfigurationChanged(@NonNull Configuration newConfig) { ```

android/app/src/main/java/app/organicmaps/car/util/ThemeUtils.java

					
				@ -88,13 +87,13 @@ public final class ThemeUtils

				  @NonNull

biodranik (Migrated from github.com) commented

2024-05-27 20:54:02 +00:00

One-liners don't need braces.

    if (themeMode.equals(followSystemTheme))
      return ThemeMode.FOLLOW_SYSTEM;

One-liners don't need braces. ```suggestion if (themeMode.equals(followSystemTheme)) return ThemeMode.FOLLOW_SYSTEM; ```

android/app/src/main/java/app/organicmaps/util/ThemeSwitcher.java

					
				@ -87,3 +45,2 @@

				    UiThread.cancelDelayedTasks(mAutoThemeChecker);

				    setThemeAndMapStyle(theme);

				    final String themeToApply = ThemeUtils.getAndroidTheme(mContext);

biodranik (Migrated from github.com) commented

2024-05-27 20:54:30 +00:00

?

biodranik (Migrated from github.com) commented

2024-05-27 20:54:35 +00:00

?

biodranik (Migrated from github.com) commented

2024-05-27 20:55:12 +00:00

Marking constant variables as final often helps the reader.

biodranik (Migrated from github.com) commented

2024-05-27 20:55:59 +00:00

Please follow existing code style.

  private void setAndroidTheme(@NonNull String theme)
  {
    if (ThemeUtils.isFollowSystemTheme(mContext, theme))
      AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM);
    else if (ThemeUtils.isNightTheme(mContext, theme))
      AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_YES);
    else if (ThemeUtils.isDefaultTheme(mContext, theme))
      AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_NO);

Please follow existing code style. ```suggestion private void setAndroidTheme(@NonNull String theme) { if (ThemeUtils.isFollowSystemTheme(mContext, theme)) AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM); else if (ThemeUtils.isNightTheme(mContext, theme)) AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_YES); else if (ThemeUtils.isDefaultTheme(mContext, theme)) AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_NO); ```

biodranik (Migrated from github.com) commented

2024-05-27 20:56:10 +00:00

  private int getStyle(@NonNull String theme)
  {

```suggestion private int getStyle(@NonNull String theme) { ```

biodranik (Migrated from github.com) commented

2024-05-27 20:56:18 +00:00

    if (ThemeUtils.isNightTheme(mContext, theme))
    {

```suggestion if (ThemeUtils.isNightTheme(mContext, theme)) { ```

biodranik (Migrated from github.com) commented

2024-05-27 20:56:31 +00:00

    }
    else
    {

```suggestion } else { ```

android/app/src/main/java/app/organicmaps/util/ThemeUtils.java

					
				@ -45,6 +46,25 @@ public final class ThemeUtils

				    return VALUE_BUFFER.resourceId;

biodranik (Migrated from github.com) commented

2024-05-27 20:56:49 +00:00

Please fix braces in other places.

android/app/src/main/res/xml/prefs_main.xml

biodranik (Migrated from github.com) commented

2024-05-27 20:58:05 +00:00

Wrong indentation?

SRSAS (Migrated from github.com) reviewed 2024-05-28 00:41:49 +00:00

android/app/src/main/AndroidManifest.xml

SRSAS (Migrated from github.com) commented

2024-05-28 00:41:49 +00:00

The XML attributes are only inherited by activities that actualy inherit (in java) other activities. So we were able to reduce the number of 'android:configChanges="uiMode"' by adding the BaseMwmFragmentActivity to the manifest and giving it that attribute.
However, any activity that specifies its configChanges, has to have "uiMode" set explicitly

The XML attributes are only inherited by activities that actualy inherit (in java) other activities. So we were able to reduce the number of 'android:configChanges="uiMode"' by adding the BaseMwmFragmentActivity to the manifest and giving it that attribute. However, any activity that specifies its configChanges, has to have "uiMode" set explicitly

Jean-BaptisteC (Migrated from github.com) reviewed 2024-05-28 04:36:33 +00:00

data/strings/strings.txt

Jean-BaptisteC (Migrated from github.com) commented

2024-05-28 04:36:33 +00:00

    fr = Clair

```suggestion fr = Clair ```

rtsisyk reviewed 2024-06-09 12:34:07 +00:00

android/app/src/main/java/app/organicmaps/util/ThemeSwitcher.java

					
				@ -87,3 +45,2 @@

				    UiThread.cancelDelayedTasks(mAutoThemeChecker);

				    setThemeAndMapStyle(theme);

				    final String themeToApply = ThemeUtils.getAndroidTheme(mContext);

rtsisyk commented

2024-06-09 12:34:07 +00:00

We really need to add a formatter. A lot of time and effort is spent on these nit comments.

Jean-BaptisteC (Migrated from github.com) requested changes 2024-06-26 17:00:34 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

❌ Pixel 6 - Android 14
How system value works on device using Android 8 or Android 5 (In this version native settings to update theme device doesn't exist) ?
Change Android theme settings is not seen by the app when users is in OSM profile fragment and about fragment

❌ Pixel 6 - Android 14 How system value works on device using Android 8 or Android 5 (In this version native settings to update theme device doesn't exist) ? Change Android theme settings is not seen by the app when users is in OSM profile fragment and about fragment

Jean-BaptisteC (Migrated from github.com) approved these changes 2024-08-04 16:57:09 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

✅ Pixel 6 - Android 14
Can you rebase your branch and removes changes about harfbuzz?
And replace in french translations claire with clair.
PTAL @biodranik

✅ Pixel 6 - Android 14 Can you rebase your branch and removes changes about harfbuzz? And replace in french translations claire with clair. PTAL @biodranik

This repo is archived. You cannot comment on pull requests.

No reviewers

Jean-BaptisteC