[Core] Switching theme via theme service works only once

MartinZikmund commented 8 months ago

Current behavior

I tried this code and it seems it only works once (e.g. starting with dark theme, first it switches to light, but on the second click the IsDark property stays true and it no longer does anything.

private void Button_Click(object sender, RoutedEventArgs e)
{
    var themeService = this.GetThemeService();
    if (themeService.IsDark)
    {
        themeService.SetThemeAsync(AppTheme.Light);
    }
    else
    {
        themeService.SetThemeAsync(AppTheme.Dark);
    }
}

Expected behavior

How to reproduce it (as minimally and precisely as possible)

Environment

Nuget Package (s):

Package Version(s):

Affected platform(s):

[ ] iOS
[ ] macOS (AppKit)
[ ] Mac Catalyst
[ ] Android
[ ] WebAssembly
[ ] Windows
[ ] Skia (WPF)
[ ] Skia (GTK on Linux/macOS/Windows)
[ ] Skia (Linux Framebuffer)
[ ] Build tasks

Visual Studio:

[ ] 2019 (version: )
[ ] 2022 (version: )
[ ] Visual Studio Code (version: )
[ ] Rider Windows (version: )
[ ] Rider macOS (version: )

Relevant plugins:

[ ] Resharper (version: )

Anything else we need to know?

assassin316 commented 5 months ago

@MartinZikmund

Is there a workaround I can apply in the meantime?

MartinZikmund commented 5 months ago

@assassin316 The problem in this case was incorrect usage - specifically if the GetThemeService is only called when switching the theme, it will first start initializing asynchronously and reset the theme that was set when the app was running previously. Then the if executes right after and switch the theme again, so in result what can happen it jumps back to the original theme - e.g. dark -> light -> dark. The workaround is to call this.GetThemeService earlier, for example during app launch.

@nickrandolph I am not sure what to do to make this more intuitive. The key thing is to make sure the theme service is always initialized earlier than it is used - e.g. during app startup. It feels that rather than doing the initialization in async fire-and-forget fashion, it would be more correct to have an explicit Initialize method for example. In addition, I am not sure why the theme service needs to be async - it seems that it is mainly to handle the case where it is called from non-UI thread, which sounds to me as an unlikely scenario, which should also be taken care of by the caller, not by the service

nickrandolph commented 4 months ago

I've been trying to reproduce this issue and theme switching seems to be working fine in a new application created with the Blank preset + Extensions. The one issue I did see is if I switched themes and then restarted the application, it wouldn't pick up the theme - toggling would then not work first time I clicked but there after it did. The fix for this is to initialize the theme service in app.xaml.cs immediately before activating the window eg

var theme = MainWindow.GetThemeService();

// Ensure the current window is active
MainWindow.Activate();

I'm going to close this issue but if we can get more context/repro, we can reopen the issue for further investigation

unoplatform / uno.extensions