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

[Android & iOS] dialog theme change on changing UserAppTheme #24559

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Aug 31, 2024

Description of Change

Dialog theme didn't change on changing UserAppTheme. It works when changing the app theme via the system's settings thanks to this PR: #13318

Issues Fixed

Fixes #24558

Before After
Screen.Recording.2024-08-31.at.23.41.51.mov
Screen.Recording.2024-09-01.at.00.04.39.mov

@kubaflo kubaflo requested a review from a team as a code owner August 31, 2024 22:10
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 31, 2024
Copy link
Contributor

Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@kubaflo kubaflo added area-theme Themes, theming and removed community ✨ Community Contribution labels Aug 31, 2024
@kubaflo kubaflo requested a review from jsuarezruiz August 31, 2024 22:11
@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

LGTM! @jfversluis ?

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@@ -83,5 +86,9 @@ protected override PlatformView CreatePlatformElement() =>
/// <param name="application">The associated <see cref="IApplication"/> instance.</param>
/// <param name="args">The associated command arguments.</param>
public static partial void MapCloseWindow(ApplicationHandler handler, IApplication application, object? args);

#if ANDROID || IOS
Copy link
Contributor

@MartyIX MartyIX Sep 2, 2024

Choose a reason for hiding this comment

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

Is it just needed for IOS or even for Mac Catalyst? I'm not sure if IOS means IOS + MacCatalyst or not :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartyIX Glad you're here :) Could you maybe have a look at how Windows behaves?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this PR is about "changing app theme after an app is started", then WiUI 3 unfortunately does not support this nicely. Feel free to upvote this issue.

What can be done is: microsoft/microsoft-ui-xaml#4474 (comment) to modify elements that are in the visual tree (like this) and then add some custom mappings to make sure that elements that are not in the UI tree are switched to correct theme as they are inserted in the UI tree. I should create a sample for this because it's quite nuanced.

-> A lot of sadness here.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I spent a bit of time to put together this: #21042 (comment). It's "working solution" but it's not exactly nice because the API is limited.)

Copy link
Member

Choose a reason for hiding this comment

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

Does the theme not flow to the children? So it is not possible to set the theme on the window or the root content and get magic to flow?

I know in Maui itself, we use the dynamic resource system to push the theme to each child. But, I suppose if we had to we can maybe do the same on windows.

We already have a hack to toggle the theme on and off just to get dynamic colors to update. If there is a perf issue, we can push back and say the WinUI team has to either fix or provide some other way to set the theme like the rest of the development world.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattleibow WinUI behavior is explained very nicely here:

image

Proposal would allow to change theme on runtime (which would be the best really).

Does the theme not flow to the children? So it is not possible to set the theme on the window or the root content and get magic to flow?

No. I don't believe so.

If there is a perf issue, we can push back and say the WinUI team has to either fix or provide some other way to set the theme like the rest of the development world.

Well, if one is forced to switch theme one control at a time, then for a big app, it might take a long-ish time. However, it might still be acceptable because one does not change themes often. But yes, it seems that WinUI has all necessary plumbing in place and supporting app theme changes should be "relatively easy" for them to implement given that they support: User changes theme in Windows settings and app re-renders correctly.

Copy link
Member

Choose a reason for hiding this comment

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

OK, maybe for another PR we try to see if the theme flows, and if not, we see how long it really takes to update all the children's themes. Maybe it is super fast? I think that when we "refresh the theme resources" with our hack that "sets the theme to unspecified and then back" for things like button backgrounds, it is so fast that the UI did not have time to notice the change. I think this is because it is sort of setting a flag for the next UI cycle, so if we update all the controls, it may not actually trigger a direct refresh but wait for the next UI "cycle" or "frame".

@kubaflo kubaflo added the community ✨ Community Contribution label Sep 2, 2024
@rmarinho rmarinho merged commit 182c55a into dotnet:main Sep 4, 2024
97 checks passed
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Sep 5, 2024
@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Oct 1, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android & iOS] dialog theme doesn't change on changing UserAppTheme
6 participants