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

Cache the dynamic AppTheme value in Controls (and not Essentials) #11200

Merged
merged 18 commits into from Mar 14, 2023

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Nov 8, 2022

Description of Change

Previously, the RequestedAppTheme and RequestedLayoutDiection values were cached on first access and this was incorrect as they can change at runtime - both by the OS and the user.

This PR makes a few changes:

  • Don't cache the theme and layout in Essentials as there is no way to update the cache
  • Don't cache the layout anywhere because this is only accessed once when the window is [re]created
  • Cache the theme in the Controls Application where the events can actually handle.

Issues Fixed

Fixes #8236 also related with #11005
Fixes (partially) #11299
Fixes #8606

This issue was inadvertently introduced in #7996 when all the Essentials fields were cached, when a few were dynamic.

Tasks

  • Code
    • Remove incorrect caching from Essentials
    • Add caching to Controls
  • Testing
    • Manual tests
      • Android
      • iOS
      • Mac Catalyst
      • Windows
    • Unit tests
    • Device tests (not sure if this is possible as it requires a manual interaction)

Redth
Redth previously requested changes Nov 22, 2022
src/Essentials/src/AppInfo/AppInfo.android.cs Outdated Show resolved Hide resolved
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

What is the LOLs per second impact of this change?

You can try this app: https://github.com/jonathanpeppers/lols/tree/net7.0

src/Essentials/src/AppInfo/AppInfo.android.cs Outdated Show resolved Hide resolved
@UkeHa
Copy link

UkeHa commented Feb 23, 2023

what's the ETA of this issue being resolved?

The RequestedThemeChanged-Event is flimsy at best in both Android and iOS. With the trigger reacting only once in a blue moon and the app turning completely blank if you change the theme from the settings menu in iOS and try to switch back to the running app.

@mattleibow mattleibow changed the title [Android] Fix RequestedThemeChanged event [Android] Don't cache the dynamic AppTheme and LayoutDirection values Mar 7, 2023
@mattleibow mattleibow changed the title [Android] Don't cache the dynamic AppTheme and LayoutDirection values Only cache the dynamic AppTheme and LayoutDirection values in Controls Mar 8, 2023
@mattleibow mattleibow dismissed Redth’s stale review March 8, 2023 00:45

A bunch of changes so the code is fairly different.

@mattleibow mattleibow self-assigned this Mar 8, 2023
@mattleibow mattleibow added the area-theme Themes, theming label Mar 8, 2023
src/Essentials/src/AppInfo/AppInfo.android.cs Show resolved Hide resolved
src/Controls/src/Core/Application.cs Outdated Show resolved Hide resolved
Comment on lines 564 to 565
FlowController.EffectiveFlowDirection = mauiContext.GetFlowDirection().ToEffectiveFlowDirection(true);
var flowDirection = Application?.PlatformLayoutDirection.ToFlowDirection() ?? FlowDirection.LeftToRight;
FlowController.EffectiveFlowDirection = flowDirection.ToEffectiveFlowDirection(true);
Copy link
Member

Choose a reason for hiding this comment

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

Is this only called once per new Window created?

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Reverting the layout direction changes as this is a larger thing and affect locale. Android supports configuration changes on Application and Activity, and each have different effects and requirements.

I still reverted the lazy layout direction in the AppInfo as this just fires once when the handler changes - which should basically be once. This code also means that the Activity will need to restart if the locale changes. And this is done automatically by the OS because we don't have the ConfigChanges.Locale on the [Activity].

src/Controls/src/Core/Application.cs Outdated Show resolved Hide resolved
Comment on lines 564 to 565
FlowController.EffectiveFlowDirection = mauiContext.GetFlowDirection().ToEffectiveFlowDirection(true);
var flowDirection = Application?.PlatformLayoutDirection.ToFlowDirection() ?? FlowDirection.LeftToRight;
FlowController.EffectiveFlowDirection = flowDirection.ToEffectiveFlowDirection(true);

This comment was marked as outdated.

src/Essentials/src/AppInfo/AppInfo.android.cs Show resolved Hide resolved
@mattleibow
Copy link
Member

@jonathanpeppers OK, so I removed the changes for the LayoutDirection because this is not essential for performance and is only ready once per window/activity.

I saw that we would have to trigger these changes from the Application OnConfigurationChnaged level OR add the ChangesConfig.Locale to the [Activity] attribute. Both cases are not so great. If we update the theme/layout direction from application level, the UI may not be even on screen (I got events when the app was backgrounded). This can cause unexpected timing errors. If we move this to the activity, then we now need to add the new config flag and this will break all the code that assumes that a new locale means a refreshed window - the single place where we do that.

Comment on lines 112 to 115
void IApplication.ThemeChanged()
{
if (UserAppTheme != AppTheme.Unspecified)
return;

TriggerThemeChangedActual();
PlatformAppTheme = AppInfo.RequestedTheme;
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of being smart here, we just cache the new value from the OS and let the application decide what it wants to do. It is the same class, but now the property responds in a normal way to raise an event if the value has changed.

Comment on lines -19 to -20
static readonly Lazy<AppTheme> _requestedTheme = new Lazy<AppTheme>(GetRequestedTheme);
static readonly Lazy<LayoutDirection> _layoutDirection = new Lazy<LayoutDirection>(GetLayoutDirection);
Copy link
Member

Choose a reason for hiding this comment

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

We can't cache these as things in the Application.Context.Resources.Configuration are all dynamic - changeable by both the use and the OS internal processing.

@mattleibow mattleibow enabled auto-merge (squash) March 13, 2023 17:09
@mattleibow mattleibow enabled auto-merge (squash) March 13, 2023 17:09
@mattleibow mattleibow merged commit f14de87 into main Mar 14, 2023
@mattleibow mattleibow deleted the fix-8236 branch March 14, 2023 01:20
@Redth Redth added backport/suggested The PR author or issue review has suggested that the change should be backported. backport/approved After some discussion or review, this PR or change was approved to be backported. labels Mar 15, 2023
@Redth
Copy link
Member

Redth commented Mar 15, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/4427242243

@github-actions
Copy link
Contributor

@Redth backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix RequestedThemeChanged on Android
Using index info to reconstruct a base tree...
M	src/Essentials/src/AppInfo/AppInfo.android.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Essentials/src/AppInfo/AppInfo.android.cs
CONFLICT (content): Merge conflict in src/Essentials/src/AppInfo/AppInfo.android.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix RequestedThemeChanged on Android
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@Redth an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

mattleibow pushed a commit that referenced this pull request Mar 15, 2023
…1200)

Previously, the RequestedAppTheme and RequestedLayoutDiection values were cached on first access and this was incorrect as they can change at runtime - both by the OS and the user.

This PR makes a few changes:

Don't cache the theme and layout in Essentials as there is no way to update the cache
Don't cache the layout anywhere because this is only accessed once when the window is [re]created
Cache the theme in the Controls Application where the events can actually handle.

---------

Co-authored-by: Rui Marinho <me@ruimarinho.net>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
# Conflicts:
#	src/Essentials/src/AppInfo/AppInfo.android.cs
PureWeen pushed a commit that referenced this pull request Mar 16, 2023
…1200) (#13963)

Previously, the RequestedAppTheme and RequestedLayoutDiection values were cached on first access and this was incorrect as they can change at runtime - both by the OS and the user.

This PR makes a few changes:

Don't cache the theme and layout in Essentials as there is no way to update the cache
Don't cache the layout anywhere because this is only accessed once when the window is [re]created
Cache the theme in the Controls Application where the events can actually handle.

---------

Co-authored-by: Rui Marinho <me@ruimarinho.net>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
# Conflicts:
#	src/Essentials/src/AppInfo/AppInfo.android.cs

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Mar 20, 2023
…1200)

Previously, the RequestedAppTheme and RequestedLayoutDiection values were cached on first access and this was incorrect as they can change at runtime - both by the OS and the user.

This PR makes a few changes:

Don't cache the theme and layout in Essentials as there is no way to update the cache
Don't cache the layout anywhere because this is only accessed once when the window is [re]created
Cache the theme in the Controls Application where the events can actually handle.

---------

Co-authored-by: Rui Marinho <me@ruimarinho.net>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
# Conflicts:
#	src/Essentials/src/AppInfo/AppInfo.android.cs
mattleibow pushed a commit that referenced this pull request Mar 20, 2023
…1200) (#14072)

Previously, the RequestedAppTheme and RequestedLayoutDiection values were cached on first access and this was incorrect as they can change at runtime - both by the OS and the user.

This PR makes a few changes:

Don't cache the theme and layout in Essentials as there is no way to update the cache
Don't cache the layout anywhere because this is only accessed once when the window is [re]created
Cache the theme in the Controls Application where the events can actually handle.

---------

Co-authored-by: Rui Marinho <me@ruimarinho.net>
Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
# Conflicts:
#	src/Essentials/src/AppInfo/AppInfo.android.cs

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
@StepKie
Copy link

StepKie commented May 30, 2023

Not sure why but I am on the latest Visual Studio (17.6.2) Maui version, and I still see #8236 - it will only fire once (i.e. if I register in App constructor, RequestThemeChanged will fire exactly the first time the user manually changes the system theme. Subsequent changes do not raise the event (ie. breakpoint is not hit)

This is NOT FIXED.

public App()
{
       RequestedThemeChanged += (s, a) => ConfigureTheme(a.RequestedTheme);
      // ... more initialization logic
}

private void ConfigureTheme(AppTheme requestedTheme)
{
       // On Android: Breakpoint here will only hit on first toggle between Light and Dark Mode
       //ConfigChanges.UiMode is set in MainActivity.cs according to MS documentation
       Log.Debug($"Requested {requestedTheme} theme.");
       UserAppTheme = requestedTheme;
}

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-theme Themes, theming area-xaml XAML, CSS, Triggers, Behaviors backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. platform/android 🤖 t/bug Something isn't working
Projects
None yet
9 participants