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

ThemeDictionary + ThemeVariantScope #8166

Merged
merged 14 commits into from
Jan 31, 2023
Merged

ThemeDictionary + ThemeVariantScope #8166

merged 14 commits into from
Jan 31, 2023

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented May 21, 2022

What does the pull request do?

Up to date API changes:
Avalonia.Base:

+public sealed record ThemeVariant
+{
+    public ThemeVariant(object key, ThemeVariant? inheritVariant);
+    public static ThemeVariant Default { get; }
+    public static ThemeVariant Light { get; }
+    public static ThemeVariant Dark { get; }
+    public object Key { get; }
+    public ThemeVariant? InheritVariant { get; }
+}
public interface IResourceDictionary
{
+    IDictionary<ThemeVariant, IResourceProvider> ThemeDictionaries { get; }
}
public interface IResourceNode
{
-    bool TryGetResource(object key, out object? value);
+    bool TryGetResource(object key, ThemeVariant? theme, out object? value);
}
public class StyledElement
{
+    public static readonly StyledProperty<ThemeVariant> ActualThemeVariantProperty;
+    public static readonly StyledProperty<ThemeVariant?> RequestedThemeVariantProperty;
+    public ThemeVariant ActualThemeVariant { get; }
}
+public interface IGlobalThemeVariantProvider : IResourceHost
+{
+    ThemeVariant ActualThemeVariant { get; }
+    event EventHandler? ActualThemeVariantChanged;
+}

Avalonia.Controls:

+public class ThemeVariantScope : StyledElement
+{
+    public ThemeVariant? RequestedThemeVariant { get; set; }
+}
public class Application : IGlobalThemeVariantProvider
{
+    public ThemeVariant? RequestedThemeVariant { get; set; }
+    public ThemeVariant ActualThemeVariant { get; }
+    event EventHandler? ActualThemeVariantChanged
}
public class TopLevel : StyledElement
{
+    public ThemeVariant? RequestedThemeVariant { get; set; }
}

Avalonia.Theme.xxx

public class FluentTheme
{
-    FluentThemeMode Mode
}
-public enum FluentThemeMode { Light, Dark }
public class SimpleTheme
{
-    SimpleThemeMode Mode
}
-public enum SimpleThemeMode { Light, Dark }

P.S. IGlobalThemeVariantProvider can be avoided if we directly subscribe on Application.PropertyChanged from the TopLevel. As property inheriting doesn't work from the Application to the Window normally. cc @grokys

How was the solution implemented (if it's not obvious)?

How to define a set of theme specific resources

In Avalonia, theme variant specific resources can be defined in the ResourceDictionary using the ThemeDictionaries dictionary. The ThemeDictionaries dictionary uses ThemeVariant as the key and a child ResourceDictionary as the value.

Typically, developers use Light or Dark as the key for the theme variants, which are recognized at compile time. Using Default as the key marks a specific theme dictionary as a fallback in case the theme variant or resource key is not found in other theme dictionaries.

In addition to the built-in values of Light, Dark, and Default, any object value can be used as a key (since it's wrapped in the ThemeVariant(object key) structure). {x:Static} markup extension can also be used here, if a developer wants to define multiple custom themes as static properties and reference them from the XAML code.

Example:

<ResourceDictionary>
    <ResourceDictionary.ThemeDictionaries>
        <ResourceDictionary x:Key='Default'>
            <SolidColorBrush x:Key='BackgroundBrush'>White</SolidColorBrush>
            <SolidColorBrush x:Key='ForegroundBrush'>Black</SolidColorBrush>
        </ResourceDictionary>
        <ResourceDictionary x:Key='Dark'>
            <SolidColorBrush x:Key='BackgroundBrush'>Black</SolidColorBrush>
            <SolidColorBrush x:Key='ForegroundBrush'>White</SolidColorBrush>
        </ResourceDictionary>
    </ResourceDictionary.ThemeDictionaries>
</ResourceDictionary>

Note, in this PR for the Simple and Fluent theme I defined light-variant resources as Default. It's an exact opposite of UWP definitions, which define dark-variant resources as a Default.

Why? It's theme specific and we historically used light-variant as a default. Not to mention, most of modern OSes are light by default now. Custom third-party themes can still define it how they decide.

Defining custom ThemeVariant:

Custom Theme Variant can be defined as:

public static class Themes
{
    public static ThemeVariant Pink { get; } = new ThemeVariant(nameof(Pink))
    {
        InheritVariant = ThemeVariant.Light
    };
}

Then, when a resource lookup is performed, the theme dictionary defined with the Pink key is selected. If the resource is not found in that dictionary, the Light variant is used as a fallback.
It is important for developers to always set InheritVariant, as otherwise Avalonia will not be able to convert the application-level variant to the system-level variant (which is always either Light or Dark) in order to change the window frame colors.
However, when the system theme variant is used (i.e., no requested variant is set), it will always use the "Dark" or "Light" default variants. In the future, there may be a need for an API to convert system-Light to a custom application-Pink variant, for example.

The theme variant lookup in this PR works as follows:

Each control has two properties: RequestedThemeVariant and ActualThemeVariant. RequestedThemeVariant is only accessible for Application, TopLevel (including Window), and ThemeVariantScrope controls. Potentially it can be added to the Page control to improve mobile support. We need this restriction because Avalonia has inheritable properties, while UWP does not. This would cause problems with font and background colors inherited from the parent which has different theme variant. While ThemeVariantScrope override most common inheritable properties.

When Application.RequestedThemeVariant is not set, the application will inherit the system theme variant. The same goes for Window and its child controls tree, which will inherit the ActualThemeVariant from the application. If RequestedThemeVariant is set on any supported control, the subtree will have an overridden ActualThemeVariant.

Setting RequestedThemeVariant to Default or null will reset actual theme and value will be inherited from the parent (or from the system in case of Application.RequestedThemeVariant).

It's worth noting that there is a design flaw in that a user can still go one level lower and call control.SetValue(RequestedThemeVariant) or control.SetValue(ActualThemeVariant) on any control, which will still work but will not override inherited controls automatically. Ideally, ActualThemeVariant should be a fully read-only property, but it also needs to be inheritable, which is not currently possible to combine in Avalonia.

The resource lookup in this PR works as follows:

The ThemeVariant is picked up from the StyledControl.ActualThemeVariant, regardless of whether the extension was used on the control or attached with a style. Each ResourceDictionary looks up the resource in the following priority:

  1. Checks if the resource is defined directly in the ResourceDictionary.
  2. If a ThemeVariant is provided:
    2.1. Uses the theme dictionary with the variant key, if defined.
    2.2. Otherwise, uses ThemeVariant.InheritVariant recursively, if defined.
  3. If resource wasn't found, uses the theme dictionary with the Default key, if defined.
  4. Checks if the resource is defined in any merged resources.

Steps 2 and 3 are new and may add overhead to existing applications, but only if the ResourceDictionary has at least one theme dictionary set.

In the case of StaticResource, when an extension is used from the ResourceDictionary, it won't use any theme variant during the lookup process. Additionally, it is not able to understand if the extension was used from a specific theme dictionary, as our markup extension does not have this context information. However, it will still work if both resources are defined in the same dictionary or have been merged into a single dictionary. This limitation is similar to that of UWP, which also merges dictionaries.

Checklist

Breaking changes

Yes, SimpleThemeMode and FluentThemeMode removed. IResourceNode interface was changed.

c.mp4

@maxkatz6

This comment was marked as outdated.

@maxkatz6 maxkatz6 force-pushed the themeresource branch 3 times, most recently from a96fb54 to cc446d2 Compare May 22, 2022 06:53
@grokys

This comment was marked as outdated.

@grokys
Copy link
Member

grokys commented May 26, 2022

I've not fully reviewed this PR, but so far I only have two comments:

  • As mentioned in the issue, the term "theme" is overloaded. Maybe something like "theme variant" might be clearer?
  • I'm a little concerned about dynamic resources now having to make two subscriptions for each resource

I realise that the second point was done so that {DynamicResource} could be used, with no separate {ThemeResource}, but I wonder if this may hamper optimization in future? However given the benchmarks above, maybe it's not much of an issue.

@maxkatz6
Copy link
Member Author

Internally we agreed on ThemeVariant property name as for this moment. Might be changed depending on ControlTheme PR status. We want to avoid naming conflicts.
ThemeControl should also be renamed to define only a scope for theme variant (dark/light...), and not the whole theme (fluent/default).
"ThemeDictionary" probably still makes sense as it is now.

@maxkatz6 maxkatz6 added this to the 11.0 milestone Jun 19, 2022
@maxkatz6
Copy link
Member Author

maxkatz6 commented Aug 5, 2022

We now have Control.Theme property on each control.
So, we need some other naming options:

  • Control.ThemeVariant (of ThemeVariant type) - after previous discussion this one was prefered
  • Control.ThemeMode (of ThemeMode type) - sounds natural as for me
  • Control.ThemeScheme (of ThemeScheme type)

Other frameworks:

  • UWP - ElementTheme
  • GTK - GTK_THEME=theme:variant (i.e. GTK_THEME=Adwaita:dark, similarly how we plan it in Avalonia)
  • Qt - it doesn't support it natively as it seems, but developer can provide custom QtPallete
  • UIKit/AppKit (apple) - UIUserInterfaceStyle (seems like community uses term theme there a lot)
  • SwiftUI (apple) - colorTheme
  • Flutter - Brightness (in a context of Theme Brightness), that's unusual one
  • Electron - Theme (nativeTheme)
  • HTML5 - color-scheme (prefers-color-scheme)
  • Xamarin - OSAppTheme

Bottom line, it feels like no matter what we choose, there is no "correct" term for dark/light mode or theme or style or brightness or variant or [put your option here].

@grokys
Copy link
Member

grokys commented Aug 5, 2022

Yeah I think I still prefer Control.ThemeVariant. colorTheme from SwiftUI is a possibility, but given that it can control more than just colors I still prefer ThemeVariant. At least there's some precedent from GTK :)

@workgroupengineering
Copy link
Contributor

AppThemeColor?

@maxkatz6

This comment was marked as outdated.

@timunie
Copy link
Contributor

timunie commented Aug 6, 2022

AppThemeColor?

For me that would conflict with AccentColor for newbies. Just my two cents.

@maxkatz6 maxkatz6 force-pushed the themeresource branch 4 times, most recently from c25b6dc to dd2f821 Compare August 13, 2022 07:12
@maxkatz6 maxkatz6 changed the title ThemeDictionary + ThemeControl ThemeDictionary + ThemeVariantScope Aug 13, 2022
@maxkatz6 maxkatz6 marked this pull request as ready for review August 15, 2022 02:59
@maxkatz6 maxkatz6 requested a review from a team August 15, 2022 03:00
@maxkatz6

This comment was marked as outdated.

@timunie

This comment was marked as outdated.

@DrWenz

This comment was marked as outdated.

@maxkatz6

This comment was marked as outdated.

@DrWenz

This comment was marked as outdated.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028798-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member Author

@amwx thanks, done.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028801-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@amwx
Copy link
Contributor

amwx commented Jan 19, 2023

@maxkatz6 Found one issue so far trying to integrate this with FluentAvalonia. Light and Dark switching works perfectly, but it will not work with other themes (HighContrast specifically). The resource lookups are coming back with transparent. What seems to be happening is (example):
Brush: ApplicationPageBackgroundThemeBrush
Maps to: SystemColorWindowColor

When the DeferredItem factory runs for the Brush, the lookup for SystemColorWindowColor is done with null ThemeVariant which ends up looking in the Default dictionary (I've followed you so its Light), where that resource isn't defined so it ends up returning transparent. I think the Markup extensions are going to need context/theme awareness so the lookup is done correctly.

Interestingly, when Dark (for example) mode is used, when that Brush initializes in ResourceDictionary.TryGetValue(), when the item factory runs, the very next call to TryGetValue is on the same dark theme resource dictionary. When using the HighContrast theme, the next call to TryGetValue after the factory isn't the theme dictionary, but the main ResourceDictionary on FluentAvaloniaTheme (which is of type Styles). Not really sure what's causing that difference.

Note: that I just kind of threw this PR into FA so it's possible I'm doing something that's messing with it. The only special thing I do is in FATheme I force search the Application ResourceDictionary first for overriding purposes. But I took that out to test and the same behavior occurs. I'll test some more tomorrow

Otherwise, I think the API itself is fine. IMO I'm not a huge fan of the "Default" variant (I don't like it in WinUI either) as it is ambiguous as to its meaning, especially if the meaning can be changed between Avalonia & third parties. I haven't looked closely enough to know if the ThemeVariant.Default member has a purpose, but at least in the ThemeDictionaries IMO I think we should use actual theme names and just implicitly map "Default" to "Light"

@timunie
Copy link
Contributor

timunie commented Jan 19, 2023

maybe FallbackTheme is a better naming than Default? At least for me it would be easier to understand what is meant by this, a fallback to search if the other lookup was not successful.

@maxkatz6
Copy link
Member Author

The problem is, "ThemeVariant.Default" is used in two different ways depending on the context.
RequestedTheme="Default" will reset variant on the current control. Basically, sets it to "Unset".
ResourceDictionary x:Key="Default" will work as a fallback resource.

@SKProCH
Copy link
Contributor

SKProCH commented Jan 19, 2023

@maxkatz6 in Material.Avalonia we have BaseThemeMode (Light and Dark actually) and also Primary and Secondary colors (which may be any color). Do I understand correctly that this change only covers BaseThemeMode in my case?

@maxkatz6
Copy link
Member Author

@SKProCH yes, only dark/light mode, I don't think there is any meaningful way to abstract app accent colors across the app. As different themes might have one or many accent colors. Like material 3 on android which has three accent color.

@amwx
Copy link
Contributor

amwx commented Jan 22, 2023

@maxkatz6 Expanding on the issue I noted, take this repro into the Sandbox project:

MainWindow:

<Window xmlns="https://github.com/avaloniaui"
        xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'
        x:Class="Sandbox.MainWindow"
        Background="#202020">

  <Border Width="300" Height="300"
          Background="{DynamicResource UniqueColorBrush}" />
  
</Window>

MainWindow.cs

public ThemeVariant CustomTheme = new ThemeVariant("Custom")
{
   InheritVariant = ThemeVariant.Light
};

public MainWindow()
{
    this.InitializeComponent();
    this.AttachDevTools();

    // A method to switch themes
    KeyDown += (s, e) =>
    {
        switch (e.Key)
        {
            case Avalonia.Input.Key.L:
                Application.Current.RequestedThemeVariant = ThemeVariant.Light;
                break;

            case Avalonia.Input.Key.D:
                Application.Current.RequestedThemeVariant = ThemeVariant.Dark;
                break;

            case Avalonia.Input.Key.Space:
                Application.Current.RequestedThemeVariant = CustomTheme;
                break;
        }
    };
}

Now, take this ResourceDictionary and put it in the Window's Resources:

<ResourceDictionary>
  <ResourceDictionary.ThemeDictionaries>
    <ResourceDictionary x:Key="Default">
      <Color x:Key="UniqueColor">Red</Color>

      <SolidColorBrush x:Key="UniqueColorBrush" Color="{StaticResource UniqueColor}" />
    </ResourceDictionary>
    <ResourceDictionary x:Key="Dark">
      <Color x:Key="UniqueColor">Blue</Color>

      <SolidColorBrush x:Key="UniqueColorBrush" Color="{StaticResource UniqueColor}" />
    </ResourceDictionary>
    <ResourceDictionary x:Key="Custom">
      <Color x:Key="UniqueColor">Purple</Color>

      <SolidColorBrush x:Key="UniqueColorBrush" Color="{StaticResource UniqueColor}" />
    </ResourceDictionary>
  </ResourceDictionary.ThemeDictionaries>
</ResourceDictionary>

So far this should all work as expected.

Now make the following changes:

  • Remove the resources from the light/dark theme dictionaries, but leave the Color and Brush in the Custom dictionary
  • Change the binding on the brush color to a DynamicResource.

Switch to the Custom theme, then to either light or dark, then back to custom and the Border will be transparent. If you return the light/dark resources (keeping the custom theme as a DynamicResource) it will work. Interestingly, only the colors need to be kept in all dictionaries for the lookup to succeed, removing just the brushes from light/dark have no effect.


Now, remove the ResourceDictionary from the window. Copy the original (from above) to the Application resources.

Change the binding on custom theme's brush to DynamicResource (but leave the others active). The lookup will fail and fall back to Light theme (Red) when you switch to the Custom theme (this worked in the Window's resources, but fails at the App level). Thus, not having the resources present in light/dark would lead to the lookup failing, and this is basically the same issue/result with the HighContrast theme I already noted.

It seems to be an issue with DynamicResource

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0029177-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member Author

@amwx I investigated this issue and found surprising bug with our bindings support. See #10110

Copy link
Member

@kekekeks kekekeks left a comment

Choose a reason for hiding this comment

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

LGTM except for the compiler thingy

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0029334-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@0x90d
Copy link
Contributor

0x90d commented Feb 13, 2023

Hi,

I just upgraded to Preview 5 and figured out the Fluent Mode property has been removed. It seems like I must set the mode in code using:

Application.Current.RequestedThemeVariant = Avalonia.Styling.ThemeVariant.Dark;

That seems to work. However I am also using DataGrid and the code above has no impact. The DataGrid is still themed in light mode. Can someone please quickly tell me what I am missing? List of breaking changes doesn't give me a clue

@maxkatz6
Copy link
Member Author

@0x90d it's a bug, see #9410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants