WindowManagerService does not apply current Theme to new Windows

[mvegaca]

1. Create a new project with a blank page, a settingsPage and MultiView feature
2. Add code to open a new window following this doc.
3. Start the app
4. Go to settings and change theme to Dark
5. Open a new window using the MultiView feature

Expected behaviour: The new window should show in dark theme.

Actual behaviour: The new Window should show in light theme.

We need to pass theme as Parameter and set in the new window frame.

public async Task<ViewLifetimeControl> TryShowAsStandaloneAsync(string windowTitle, Type pageType)
{
    var theme = (Window.Current.Content as FrameworkElement).RequestedTheme;
    ViewLifetimeControl viewControl = await CreateViewLifetimeControlAsync(windowTitle, pageType, theme);
    SecondaryViews.Add(viewControl);
    viewControl.StartViewInUse();
    var viewShown = await ApplicationViewSwitcher.TryShowAsStandaloneAsync(viewControl.Id, ViewSizePreference.Default, ApplicationView.GetForCurrentView().Id, ViewSizePreference.Default);
    viewControl.StopViewInUse();
    return viewControl;
}

private async Task<ViewLifetimeControl> CreateViewLifetimeControlAsync(string windowTitle, Type pageType, ElementTheme theme)
{
    ViewLifetimeControl viewControl = null;

    await CoreApplication.CreateNewView().Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
    {
        viewControl = ViewLifetimeControl.CreateForCurrentView();
        viewControl.Title = windowTitle;
        viewControl.StartViewInUse();
        var frame = new Frame() { RequestedTheme = theme };
        frame.Navigate(pageType, viewControl);
        Window.Current.Content = frame;
        Window.Current.Activate();
        ApplicationView.GetForCurrentView().Title = viewControl.Title;
    });

    return viewControl;
}

[crutkas]

shouldn't this grab the theme by the service, not be passed in?

[mvegaca]

I think this should be initialized when new Frame is created, even if there is no Settings page added.

[mvegaca]

I could retrieve it from theme service but if MultiView is added by right click we don't know if ThemeService is added or not.

[mrlacey]

I have three concerns about this issue.

Concern 1 - The repro is inadequate. Given the instructions in this issue, I can't recreate it. What is meant by "Create an app"? Surely there are some specifics needed in the app that must be documented. Does "Set theme to Dark" mean the OS setting? What if it was already? Are there some assumptions or pre-conditions that haven't been specified. Without adequate reproduction instructions, it's not possible to verify the issue on different machines or conditions. It's may not make it possible to comment on the appropriateness of any proposed solution. It's also not possible to verify that any fix actually addresses the issue as it can't be verified. People should not be left guessing at the specifics of reproduction steps as they may not match those of the OP. Even if different steps produce the same result this might be coincidental and the fix may not address what everyone recreates. The most common consequence of inadequate repro steps is that time is lost trying to work out what they are when the person who knows this doesn't take the time to document them.

Concern 2 - Jumping in with a code solution. The impression I get from viewing this issue is that an unexpected or unwanted behavior was observed, minimal details were recorded, and then a solution that involved adding more code was proposed without wider investigation or considering other options. Do not rush in with the first solution that comes to mind, especially if it means writing more code. The full extent of the issue should be explored and documented. Multiple possible solutions should be considered (and documented) with the merits and consequences of each assessed and the best overall solution chosen.

Concern 3 - This sounds like a bug with what already exists. Fixing what's there already is preferable to adding more code. My understanding of UWP is that a new window should automatically pick up the system theme if no theme is explicitly defined. If this is not happening it sounds like a platform bug. Based on the info in this issue it sounds as though the platform always uses the theme that was specified when the app was launched. There are potentially good reasons for why this might be the case but they should be verified by the platform team by first raising a bug for the UWP platform. If this is the case then a solution similar to the proposed may be the best solution. However, it looks as though the proposed solution would not then handle subsequent theme changes in the opened window. (I say "it looks as though" this is what will happen as I can't recreate it based on the repro currently provided.) Another possible expalanation of the desccribed behavior is that it's an issue with the code that is already being added to the solution. If this is the cause t shoudl be corrected, rather than other code added to counter the effects. It's important to identify the cause of the issue before a solution is proposed.

Also, the possibility of adding a dependency on the ThemeService to the MultiView feature seems to have been dismissed without thought or justification. Why is this?

[sibille]

I updated the repro steps. Regarding the proposed solution we had a similar issue with the dialogs (#1998) where we discussed various solutions and agreed on setting the theme based on the current window theme.

The ThemeService is not a independent template, but comes with the SettingsPage. MultiView should no have a dependency on the settings page, but we could think in building out the ThemeService in a separate Theme feature, that's consumed by SettingsPage, MultiView and Dialogs.

That would allow to take the theme from the ThemeService and also allow to notify Windows to change the theme if theme is changed once the window is opened.

[mrlacey]

The reason for not using the ThemeService was that there's no guarantee that it would be included (as a dependency of the SettingsPage) but the repro steps say that the Settings Page must be included to produce the effect. - Which is it?

The repro steps are still not complete. Step 1 also needs to include another page Between Steps 1 & 2 it's necessary to implement the code documented here.

This situation is different from the dialog issue. Dialogs are shown as part of the window. In this scenario, we have one app with multiple windows. By hardcoding the theme to use when opening other windows, it is then possible to change the desired app theme via the settings page in another window but not have secondary windows reflect the change. This doesn't happen with dialogs as dialogs are displayed within a window that can't have its theme changed while the dialog is displayed.

The ThemeService isn't a standalone dependency but there's no reason it couldn't be turned into one if it would address this problem without adding more code.

[sibille]

Updated the repro steps. @crutkas, if you agree I'll open an issue to turn the ThemeService in a standalone feature template, that is added as dependency of Settings, MultiView and FirstRunDialog to get the theme from the service when needed.

[crutkas]

I'm fine with that but i understand why Multi-view would but why would first run / new version dialog need it?

[sibille]

First run and new version dialog get the theme from current window theme, if there is a ThemeService, I think they should get it from the ThemeService.

[sibille]

Moving to 2.4 as this depends on the Theme Selection feature

[dgomezc]

Once we have the ThemeSelection feature, we can use it as a dependency of the MultiView feature and use it.

We can modify the CreateViewLifetimeControlAsyncmethod from WindowManagerService class and set RequestTheme to the frame using ThemeSelectorService:

private async Task<ViewLifetimeControl> CreateViewLifetimeControlAsync(string windowTitle, Type pageType)
{
    ViewLifetimeControl viewControl = null;

    await CoreApplication.CreateNewView().Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
    {
        viewControl = ViewLifetimeControl.CreateForCurrentView();
        viewControl.Title = windowTitle;
        viewControl.StartViewInUse();
        var frame = new Frame();
        frame.RequestedTheme = ThemeSelectorService.Theme;
        frame.Navigate(pageType, viewControl);
        Window.Current.Content = frame;
        Window.Current.Activate();
        ApplicationView.GetForCurrentView().Title = viewControl.Title;
    });

    return viewControl;
}

When we change the theme from the Settings page and have some views open, the views are not updated with the new selected theme. To solve this we can modify the SetRequestedTheme method of ThemeSelectorService to apply the selected theme in each view:

public async static Task SetRequestedTheme()
{
    foreach(var view in CoreApplication.Views)
    {
        await view.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
        {
            if (Window.Current.Content is FrameworkElement frameworkElement)
            {
                frameworkElement.RequestedTheme = Theme;
            }
        });
    }
}

[mvegaca]

Once ThemeSelection is a separated feature, I've created two different approaches to implement theme selection in multiview scenarios.

Common in both scenarios There is a ThemeSelection feature dependency on Multiview feature to create new windows using ThemeSelectorService.Theme

Changes on Service Option The ThemeSelectorService applies theme changes to all windows like David exposed above. Code here ChangesOnServiceOption.zip

Event Hadler Option ThemeSelector service will not change his SetRequestedTheme implementation. ThemeSelectorService contains a ThemeChanged EventHandler that informs theme changes to other parts of the application. All secondary views will be subscribed to this handler and will refresh his own Windows Requested Theme on the event handler method. Code here EventHandlerOption.zip.

I think that Changes on Service Option is better because the developer doesn't have to add more code to secondary views and the code works as expected.

I also think that Event Hadler Option is a less strong dependency between ThemeSelection and multiview and allows to reach a scenario with pages that apply theme changes and pages that not do it (I'm not sure if it makes sense on a real scenario). The worst part of this implementation is that it does not work out of the box.

I think once we have a separate ThemeSelection feature we should update MultiView feature with David approach.

[sibille]

Verified in dev-nightly: Templates version: 0.18.18241.1 Wizard version: 0.18.18241.1