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

Respect UserAppTheme in RequestedTheme #5071

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Mar 5, 2022

Description of Change

The #4964 PR made some changes and inadvertently wiped out support for user setting the theme:

https://github.com/dotnet/maui/pull/4964/files?show-deleted-files=true&show-viewed-files=true&file-filters%5B%5D=#diff-e95ac81676df57c3de0226dc120b0e527fbd52bc7fc506ddfde7cccf34a8f489L188

Issues Fixed

Fix #5066

@mattleibow mattleibow self-assigned this Mar 5, 2022
public void ThemeChanged()
void IApplication.ThemeChanged()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just an implementation detail and causes conflicts and ambiguity for user code trying to attach the the very similarly named event.

Comment on lines -171 to +174
public AppTheme RequestedTheme => AppInfo.RequestedTheme;
public AppTheme RequestedTheme => UserAppTheme != AppTheme.Unspecified ? UserAppTheme : PlatformAppTheme;
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the bug!

@@ -167,8 +167,11 @@ public AppTheme UserAppTheme
TriggerThemeChangedActual(new AppThemeChangedEventArgs(value));
}
}

public AppTheme PlatformAppTheme => AppInfo.RequestedTheme;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a new property to get the OS theme so as not to break existing code in a subtle way. I know @PureWeen you suggested not doing this, but I just wanted to mention the potential breaks and if you thought it was worth it?

@mattleibow mattleibow added the legacy-area-a11y Relates to accessibility label Mar 5, 2022
@mattleibow mattleibow added this to the 6.0.300-preview.14 milestone Mar 5, 2022
@mattleibow mattleibow added the t/bug Something isn't working label Mar 5, 2022
Comment on lines -167 to +169
TriggerThemeChangedActual(new AppThemeChangedEventArgs(value));
TriggerThemeChangedActual();
Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually incorrect and my test detected it. If you set the user theme to unspecified, then the event would say it was unspecified - but this is not a valid state for the theme as it falls back to the OS theme.

var uiStyle = Platform.GetCurrentUIViewController()?.TraitCollection?.UserInterfaceStyle ??
UITraitCollection.CurrentTraitCollection.UserInterfaceStyle;
var traits =
MainThread.InvokeOnMainThread(() => Platform.GetCurrentUIViewController()?.TraitCollection) ??
Copy link
Member Author

Choose a reason for hiding this comment

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

This API needs to be accessed from the UI thread.

@mattleibow mattleibow merged commit 8f329d7 into release/6.0.2xx-preview14 Mar 7, 2022
@mattleibow mattleibow deleted the dev/fix-app-theme branch March 7, 2022 04:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@Eilon Eilon added the t/a11y Relates to accessibility label May 13, 2024
@samhouts samhouts added the fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! legacy-area-a11y Relates to accessibility t/a11y Relates to accessibility t/bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants