dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.11k stars 1.17k forks source link

[API Proposal]: FluentWindows Theme Switch in WPF #8932

Open dipeshmsft opened 8 months ago

dipeshmsft commented 8 months ago

Background and motivation

With the introduction of FluentWindows ( Win11 ) theme, we want to allow developers to be able to enable disable and switch themes in their WPF applications. Irrespective of if we provide an API in .NET 9, I believe that we should provide a way for developers to choose only Light, Dark theme or respond to system theme changes.

Part of effort - https://github.com/dotnet/wpf/discussions/8655

This is inspired and a minified version of the #8759

Before going through the alternatives, I would like to bring to the point, since we are so close to the code completion deadline for .NET Preview 7, that if we choose to implement the API, we keep it experimental, so that we have room for further discussion in future.

API Proposal

Current Proposal


    public class Application
    {
        [Experimental]
        public string Theme { get; set; }
    }

    public class Window
    {
        [Experimental]
        public string Theme { get; set; }
    }

The accepted values for the theme properties can be - None, System, Light and Dark

As mentioned above, these APIs will be Experimental until we have further discussions and have a better way to handle theme ( default ) styles in .NET 10.

Behavior of the APIs

  1. When Window.Theme = System / Light / Dark it will take precdence over Application.Theme and we will load the Fluent styles for window. In case, of System Window will respond to System theme changes.
  2. When Application.Theme = System / Light / Dark and Window.Theme = None then window will follow the application.
  3. In case none of the above properties are set, we will get Aero2 by default.
  4. Accent color changes will be received by every window irrespective of the Theme property values. If Theme value is None, then Accent colors don't come into play.
  5. Default values for both Theme properties is None.

Alternative Designs

No response

Risks

No response

cc: @dotnet/dotnet-wpf-triage @pomianowski

pomianowski commented 8 months ago

Will this property automatically affect all windows for which current one is Parent? I think that changing e.g. MainWindow should affect all additional windows for this below

batzen commented 8 months ago

The default should come from Application.Current, if there is one.

I don't think getting it from the parent window is a good idea.

dipeshmsft commented 8 months ago

@pomianowski @batzen I have updated the proposal. The new DP was meant to be at the application level.

batzen commented 8 months ago

@dipeshmsft Only having it at the application level won't be enough for all use-cases, i think.

dipeshmsft commented 8 months ago

@batzen, apart from the application one place where we should have this, I guess is at the window level. Do you have some other use cases for this, that we can consider ?

batzen commented 8 months ago

I think Application and Window should be enough.

dipeshmsft commented 8 months ago

@batzen, what is the behavior that you expect when a developer sets the Theme property on only one window in the application ?

batzen commented 8 months ago

@dipeshmsft That only that one specific window, and thus it's content, changes it's theme.

miloush commented 8 months ago

@dipeshmsft Only having it at the application level won't be enough for all use-cases, i think.

@batzen can you please elaborate on which use cases are you expecting for having different theme per window in one app? I don't think I have seen an app that offers that. It would also mess up the precedence of resources I think...

MichaeIDietrich commented 8 months ago

A prominent example is Visual Studio. Visual Studio supports dark and light themes for quite some time now, but the options window still ignores it, most probably due to its complexity.

In general I can see where it can be useful to disable dark theme (force light theme) for some windows where it just will take some time to fix all the issues to correctly support dark theming. Or when you embed 3rd party views where you are not in control whether theming is correctly implemented.

Then it may be better to force a light theme for a specific window rather than mixing dark and light themes.

batzen commented 8 months ago

@MichaeIDietrich That's exactly what i would have said. And we must not forget that WPF has to compete with MahApps.Metro, MaterialDesignInXAML, WPFUI, Fluent.Ribbon, ControlzEx etc. all of those allow changing the theme even further down at the FrameworkElement level.

miloush commented 8 months ago

Visual Studio case is more about "themed" window vs "non-themed" window. They probably just did not bother investing in theming the legacy Settings window, focusing on the new settings tab experience instead (which is themed). Same with some of the dialogs that were "forgotten" unthemed but newer ones are themed. Note that if you turn on one of the high contrasty themes, they will apply to the settings too. This is not a very convincing example.

WPF has to compete

It does not. People can still keep using 3rd party libraries to fill in functionality that the framework does not provide.

I am not against having window/element level theming in principle if it was a low-hanging fruit, despite me not seeing a good use case for it yet, but it is not. You would have to design and change the whole DP precedence system to squeeze per window/element themes in. The team is already too busy with delivering this feature and I would rather if we not increase the release bar further. Let's get application-wide theme switching out and if there is enough calls for having this controlled more granularly, we can do add this later.

As for the proposed API itself, I would prefer if this was a string property where you can put any of the themes available, such as "Classic" or "Royale", which would require no changes for future themes (or possibly when custom ones can be shipped with apps).

MichaeIDietrich commented 8 months ago

Visual Studio case is more about "themed" window vs "non-themed" window.

Not quite sure whether I would agree on the "themed" vs "non-themed" argument. From the WPF point of view every visual representation is a theme, so there isn't really something like "non-themed". That's why I would differentiate between light and dark theming, mixing two different light themes (e.g. Aero and Aero2) is visually less an issue than mixing dark and light themes.

They probably just did not bother investing in theming the legacy Settings window, focusing on the new settings tab experience instead (which is themed). Same with some of the dialogs that were "forgotten" unthemed but newer ones are themed.

I'm pretty sure that the idea of moving the settings to a new tab experience is by far not as old as the Settings window not supporting a different theme. So they had their reasons to not support it, we can only guess at that point.

Note that if you turn on one of the high contrasty themes, they will apply to the settings too. This is not a very convincing example.

Good point indeed. I'm pretty surprised it doesn't look as scrambled as I would have expected it.

Still, my point stays that we need to keep in mind real world applications that consists of more than 3 windows and have developed over years with hard coded colors at several places and stuff that cannot be fixed within two days.

I just see applications like from the company I work at that has support for 3rd party plugins bringing their own UI. Such plugins won't support dark theme on day one. And now there will be only the option to disable dark theme for the whole application or to let the user live with the result.

if it was a low-hanging fruit

I agree with that. It's more work to do if we want to support this. And I'm also on your side that even with the current state I'm not 100% confident that Win11 themes will make it to the next .NET release seeing the progress so far.

As for the proposed API itself, I would prefer if this was a string property where you can put any of the themes available, such as "Classic" or "Royale", which would require no changes for future themes (or possibly when custom ones can be shipped with apps).

Same for me, I would also prefer to make all the themes just identifiable by a key and not implement any specific logic for Win11 into WPF. That way custom themes could also be implemented.

But this doesn't mean the theme to use couldn't be also resolved on window level, since resources are already resolved that way by walking up the hierarchy, this could be also done for themes.

This all being said, focus should be on bringing Win11 themes with the next .NET version and if such feature wishes would delay the release, then it can be, of course, argued to give those a lower priority.

batzen commented 8 months ago

@miloush Another argument for having it at least at the window level: What should i do if i don't have an application object? That's often the case for unit tests and for things like office addins.

WPF has to compete

It does not. People can still keep using 3rd party libraries to fill in functionality that the framework does not provide.

In regards to dark/light flexibility it has to compete. If it does not have to compete with those, or at least delivering the building blocks, i hardly see any reason copy/pasting the code from WPFUI to be WPF code base. This time WPF slept for a very long time and people got used to use third party libraries to fill those gaps. If we now want to fill parts of those gaps it has to meet the expectations people have from those third party libraries. It's also quite strange to me that we haven't heard from larger companies building WPF solutions here. Do they even know what's currently planned here? What's the expectation for future development? If it does not have to compete i can't rely on the then WPF provided dark/light setting in my libraries/apps and will have to keep all the code that manages that. Dark/Light should be totally unrelated to the concrete theme being loaded, but just the color values being provided for a theme.

@dipeshmsft So i guess what we really need is not None, System, FluentWindowsLight and FluentWindowsDark but System, Dark, Light as the enum values and an additional DP that reflects the current real value for those. Without that additional DP we can't trigger on anything that's system because we don't know if it's dark or light. We also need a way to load additional RDs for Dark/Light in the app/library itself as apps/libraries might have custom dark/light color values. The theming system we came up with in ControlzEx (see https://github.com/ControlzEx/ControlzEx/blob/develop/src/ControlzEx/Theming/ThemeManager.cs) does exactly that. It's far from perfect, but at least ensures/delivers that flexibility.

@miloush

You would have to design and change the whole DP precedence system to squeeze per window/element themes in. The team is already too busy with delivering this feature and I would rather if we not increase the release bar further. Let's get application-wide theme switching out and if there is enough calls for having this controlled more granularly, we can do add this later.

I am unable to see any work being done in a direction that implements a theming system different from regular resource dictionaries, so i totally don't get that point. If there is work being done in that direction could you point me to it? There are bugs/quirks in WPF that should be fixed along the way to fully unblock RD based theming though For example https://github.com/dotnet/wpf/issues/8860

Also https://github.com/dotnet/wpf/pull/5610 should be merged if all the DynamicResource usages increase in the system provided theme which currently enable dark/light switching.

miloush commented 8 months ago

What should i do if i don't have an application object?

OK that is an argument I can understand.

So i guess what we really need is not None, System, FluentWindowsLight and FluentWindowsDark but System, Dark, Light.

I can work with that. So you would have one property for the theme selection (Classic/Royale/Aero2/Fluent etc.) that would pick up control templates and one property for System/Dark/Light that would say substitute a set of colors/brushes used as dynamic resources (technically this wouldn't need to be enums either). The colors/brushes set would do nothing for themes that don't use them, but app could still look up the values in resources. As for the extra DP, this could be a value in SystemParameters or somewhere. There already seems to be UxThemeName and UxThemeColor.

The alternative is kind of what Visual Studio is doing (and possibly what @dipeshmsft was originally proposing), you have one property with a set of "themes" that include dark, system and anything in-between.

image

I think I am starting to like the separation of colors and templates more.

It is my impression that the intention is that the Fluent theme would be in all respects equivalent to the existing themes such as Aero2, the RD method is just used for testing it. We cannot leave it as RD because that messes up with the precedence of resources. An opt-in to Fluent is needed, and providing a property that allows users to pick a specific theme seems to be the best option to me and an enhancement on its own.

As a possibly slightly off-topic, how does ThemeDictionaryExtension fit into this? I don't think I have used that one personally.

dipeshmsft commented 5 months ago

@batzen @MichaeIDietrich @pomianowski

@miloush, Although I like the idea of having separate properties for theme name and color, but this will open up a new layer for styling to the developers. With the current system in place, any WPF developer can use explicit and implicit styles, however providing theme name would open a new layer ThemeStyle for the developers. So, I am not very convinced if we should do that.

I like the idea of having ThemeColor property as string, as other themes in past have different names ( Luna - Metallic, Cobalt, etc. ).

I am unable to see any work being done in a direction that implements a theming system different from regular resource dictionaries, so I totally don't get that point. If there is work being done in that direction could you point me to it?

@batzen, as miloush mentioned earlier the current method was a testing method and we want to make it similar to other themes. I have been doing experiments with ThemeStyle and it doesn't work well with DynamicResource's . When we want to do changes in ThemeStyle, the current infrastructure invalidates all the resource and loads them again, and if I haven't missed anything there is no way as of now to load two theme style resource dictionaries at once. So, if we want to enable different theme for window and application ( which we definitely want to do ), it will need some changes in StyleHelper, SystemResources and related classes.

One thing that I can do is, quickly try creating PresentationFramework.Fluent.Light and Dark combined resource dictionaries and move accent colour resources to SystemResources as then we can allow DynamicResource, but rest of the resources will have to be made static. However this stops developers from using the brush and color resources defined in Fluent theme to create new styles for custom controls. What are your views on this ?

miloush commented 5 months ago

I have looked into this a bit and I would support a property on FrameworkElement. We currently have ThemeDictionaryExtension that can be used for ResourceDictionary.Source. If I understand correctly, this can already be used today to switch theme assemblies on individual elements, e.g.

<Button>
    <Button.Resources>
        <ResourceDictionary Source="{ThemeDictionary PresentationFramework}" />
    </Button.Resources
</Button>

The only catch is that you cannot change the theme name and color, it will always try to load Component/Themes/ThemeName.ThemeColor.baml resource from the assembly (where in most cases ThemeName=Aero2 and ThemeColor=NormalColor).

  1. The first thing we could do is to extend the ThemeDictionaryExtension to support theme name and color properties. This should allow people to load Fluent (or other) themes as proper themes themselves using the existing infrastructure.

  2. We could then add a property, let's say ThemeDictionary on FrameworkElement. This would be a syntactic sugar to the above. So basically

<Button ThemeDictionary="{ThemeDictionary Name=Fluent, Color=Dark}" />

would be equivalent to

<Button>
    <Button.Resources>
        <ResourceDictionary Source="{ThemeDictionary Name=Fluent, Color=Dark}" />
    </Button.Resources>
</Button>

(or merged in if the Button already has resource dictionary set). This would allow per-element theme without Application object existing. I was skeptical about per-element themability because I thought it would require significant work. However, it seems everything should already be in place with the help of ThemeDictionaryExtension.

  1. Finally, we could add application-wide way to change the theme name and color. There is SystemParameters.UxThemeName and SystemParameters.UxThemeColor which drive the theme selection described above. One option is to make them settable, however, they would no longer reflect the system settings, which might be undesirable (and setting them would not change the Ux theme, which is also confusing). SystemParameters has a lot of useful infrastructure so it might be beneficial to put the properties on SystemParameters, like ThemeName or UxThemeNameOverride or similar, even though they would not be system per se. The other option could be static properties on Application EDIT:, although it might be desirable to set them at the same time using a method. We could also read these from app.config.
dipeshmsft commented 5 months ago

@miloush, the idea of extending ThemeDictionaryExtension seems reasonable and above all it sits well with the existing infrastructure, but to do this right now, we would need to make changes in infrastructure like - loading multiple theme dictionaries for PresententationFramework to start with. It was also the intention of the original developers, for ThemeDictionary to react to theme changes and we would have to sever that and take care of coercion in case of high contrast. This will take some time.

However, IMHO I don't think we should make SystemParameters settable. They are meant to reflect the actual values from the system and by making them settable we will deviate from the actual behavior.

terrajobst commented 5 months ago

I assume the property is meant to cause a particular resource dictionary to be merged in?

If so, what is the extensibility story? Would a developer set the property to None and manage their resource dictionaries themselves, just like they do right now?

Alternatively, you could have a design where the theme is a string and the user can register additionally dictionaries under a given name. This would allow custom themes to benefit from the same theme selection logic if you support both Application and Window scopes.

Strings aren't great for discovery, but we have recently started using what we call string-based enums which are just struct wrappers around a string:

namespace System.Windows;

public partial class Application 
{
    public static readonly DependencyProperty ThemeProperty;
    public ApplicationTheme Theme { get; set; }
    public void RegisterTheme(ApplicationTheme theme, ResourceDictionary resources);
}

public partial class Window
{
    public static readonly DependencyProperty ThemeProperty;
    public ApplicationTheme Theme { get; set; }
}

public readonly struct ApplicationTheme : IEquatable<ApplicationTheme>
{
    public static ApplicationTheme None { get; } = new(nameof(None));
    public static ApplicationTheme System { get; } = new(nameof(System));
    public static ApplicationTheme FluentWindowsLight { get; } = new(nameof(FluentWindowsLight));
    public static ApplicationTheme FluentWindowsDark { get; } = new(nameof(FluentWindowsDark));

    public ApplicationTheme(string value);
    public string Value { get; }

    public static bool operator==(ApplicationTheme left, ApplicationTheme right);
    public static bool operator!=(ApplicationTheme left, ApplicationTheme right);
}

This allows developers to define their own themes:

public static class ImmosThemes
{
    public static ApplicationTheme ImmoDark { get; } = new(nameof(ImmoDark));
    public static ApplicationTheme ImmoLight { get; } = new(nameof(ImmoDark));
}
...
ResourceDictionary immoDarkResources = ...
ResourceDictionary immoLightResources = ...
Application.Current.RegisterTheme(ImmosThemes.ImmoDark, immoDarkResources);
Application.Current.RegisterTheme(ImmosThemes.ImmoLight, immoLightResources);
Application.Current.Theme = ImmosThemes.ImmoDark;

We could also decide that having a strongly typed theme is overkill and just purely go with strings:

namespace System.Windows;

public partial class Application 
{
    public static readonly DependencyProperty ThemeProperty;
    public string Theme { get; set; }
    public KeyedCollection<string, ResourceDictionary> Themes { get; }
}

public partial class Window
{
    public static readonly DependencyProperty ThemeProperty;
    public string Theme { get; set; }
}

Usage:

...
ResourceDictionary immoDarkResources = ...
ResourceDictionary immoLightResources = ...
Application.Current.Themes.Add("ImmoDark", immoDarkResources);
Application.Current.RegisterTheme("ImmoLight", immoLightResources);
Application.Current.Theme = "ImmoDark";
dipeshmsft commented 5 months ago

@terrajobst

I assume the property is meant to cause a particular resource dictionary to be merged in?

Yes, although for now this will be to merge the resource dictionary, however the current way is not how system themes are loaded in WPF. I have started a discussion on another thread (https://github.com/dotnet/wpf/issues/9283), whether we should provide a way to enable Fluent as a system theme.

If so, what is the extensibility story? Would a developer set the property to None and manage their resource dictionaries themselves, just like they do right now?

None, here means that the framework will use the current default theme ( i.e. Aero2 ) for the application, and none of the Fluent resources will be loaded in Application resources. Regarding extensibility, historically system ( default ) theme has been linked to the OS theme and developers have managed the customization themselves.

However, I guess the string-based enum is suitable here, because even earlier we have themes have been structured as "ThemeName.ThemeColor" and the ThemeColor part keeps on varying from one theme to another.

miloush commented 5 months ago

Thanks @terrajobst we do have a few "string-based enums" in WPF already.

However, before we spend more cycles on this, perhaps @dipeshmsft can update the API proposal to what the current thinking is, it has been a while since this got originally posted.

dipeshmsft commented 5 months ago

@terrajobst @miloush

I have updated the proposal inculcating my thoughts and the inputs from the community. However, I want to reiterate that, I would like to keep any API introduced now as 'Experimental'

miloush commented 5 months ago

Thanks @dipeshmsft. Option 1 feels overly convoluted for an experimental thing to be replaced in the future. For that option I would just add a simple plain string property (which I am guessing is what you marked as alternative approach).

robert-abeo commented 5 months ago

Without reading the full discussion, I don't think the names/keys are correct.

1) You should provide System (default), Aero, Aero2, Fluent2Light, Fluent2Dark, etc. with the current convention. Older themes should all be included too IMO. 2) Not sure how you plan for high-contrast support. 3) Naming needs to be updated "FluentWindows" should just be "Fluent" as re-named elsewhere. 4) If we are making this official I still think we should name it "Fluent2". If Microsoft releases a "Fluent3" you are going to have an issue with naming and off-by-ones otherwise.

I like alternative 1 supporting both Application-level and Window-level theme properties. But why stop there? I wonder if it should be a control-level, inherited property.

dipeshmsft commented 4 months ago

After doing implementation and testing out both approaches, I have come to the following conclusion for the shape of API for .NET 9


    public class Application
    {
        [Experimental]
        public string Theme { get; set; }
    }

    public class Window
    {
        [Experimental]
        public string Theme { get; set; }
    }

The accepted values for the theme properties can be - None, System, Light and Dark

As mentioned above, these APIs will be Experimental until we have further discussions and have a better way to handle theme ( default ) styles in .NET 10.

Behavior of the APIs

  1. When Window.Theme = System / Light / Dark it will take precdence over Application.Theme and we will load the Fluent styles for window. In case, of System Window will respond to System theme changes.
  2. When Application.Theme = System / Light / Dark and Window.Theme = None then window will follow the application.
  3. In case none of the above properties are set, we will get Aero2 by default.
  4. Accent color changes will be received by every window irrespective of the Theme property values. If Theme value is None, then Accent colors don't come into play.
  5. Default values for both Theme properties is None.

PS: We want to get the experimental API implementation by Preview 7 code complete. ( 7/19 )

cc @miloush @MichaeIDietrich @robert-abeo @batzen @lindexi @ThomasGoulet73 @pomianowski

miloush commented 4 months ago

These are properties, right? i.e. public string Theme { get; set; }

"Default" sounds like a better default value than "None" because as you said the default is Aero2. However, since this is experimental it doesn't matter much.

Do these properties cause the theme to be loaded into a merged dictionary or as a proper theme?

dipeshmsft commented 4 months ago

Changed the above to properties. I haven't reached to the implementation of how Applicaiton.Theme will behave with default theme behavior. But for the default scenario, only Application.Theme will play the role in deciding the behavior of theme styles. Window.Theme will still load the themes in merged dictionary.

terrajobst commented 4 months ago

Should this be marked as api-ready-for-review then? If so, please update the issue description to reflect the current proposal.

batzen commented 4 months ago

IMHO System/Dark/Light are too vague. What happens when we get Windows 12 or Fluent3 or whatever comes next?

dipeshmsft commented 4 months ago

@batzen, as mentioned this is experimental and we will rethink this for .NET 10.

lindexi commented 4 months ago

How about add the Themes static class to avoid hard coding string?

public static class Themes
{
    public readonly static string None = "None";
    public readonly static string System = "System";
    public readonly static string Dark = "Dark";
    public readonly static string Light = "Light";
}
miloush commented 4 months ago

I don't want to add any unnecessary API for experimental purposes, especially since we know we don't actually want to do this this way in production. The aim here is to get more testing coverage, and class like this won't help in XAML anyway.

dipeshmsft commented 4 months ago

I am going ahead and marking this API as ready-for-review now

terrajobst commented 4 months ago

@miloush

I don't want to add any unnecessary API for experimental purposes, especially since we know we don't actually want to do this this way in production. The aim here is to get more testing coverage, and class like this won't help in XAML anyway.

I think that largely defeats the purpose of an experimental API. I'd say: design your experimental API as if that's your final API.

You typically use an experimental API if you that don't have a lot of runway to incorporate custome feedback, or if you know of complex interactions between your feature and the ecosystem, and thus want to ship it such that enough people use it in production or production-like scenarios to unveil any design limitations.

Yes, that means you expect that your design needs to make some changes, but you generally don't know where or what they might be. Even with an experimental API, you still want to minimize breaking changes. Therefore, I wouldn't ship something as experimental if I already know that's not the design I want -- especially because that also means you don't get feedback on the thing you actually want to build.

That said, I think it makes sense to scope the size of an experimental API such that it's a good V1. You generally want to include affordances that help with productivity / address the common cases but also sufficient coverage for advanced scenarios as that's usually where the rubber hits the road and feedback is the most useful.

Experimental APIs shouldn't be toys or prototypes. They should be well-designed and viable V1 products that need more validation.

miloush commented 4 months ago

@terrajobst agree with you completely, however that is simply not the case here. It is now apparent that this feature will need to roll out at least over two major versions. The team has spent all resources on creating the content and barely any discussions started on how the content should be made available to developers.

There is generally two ways how to do this: one is merging in a dictionary at app/window level, and the other one is turning it into a "proper" theme the same way existing themes have been done. The former is what has been done so far for testing the content and can still be done manually by developers in XAML. It is, however, not a single liner. The latter will need more work, design and API changes to get done, certainly not doable for .NET 9.

To be honest I thought this API proposal would enable to start testing the latter, while the former would remain available the same way it has been so far. This seems to not be the case though and the API proposal is now basically a much shorter syntax for the former (+ automatically swapping between dark and light themes).

It is indeed a prototype at best. If that is not acceptable for an experimental API, then I would be pressured to push back on this API proposal. Do you have any alternative mechanisms for prototypes?

terrajobst commented 4 months ago

Apologies if my commentary came across as harsh; I didn't mean to criticize the work that is being done here.

It just seemed to me that you were suggesting to keep the size of the experimental API surface as small as possible, maybe because it somehow feels risky. My main point was that I wouldn't approach it like this but rather treat it like any other feature work (compelling, good UX, etc) and treat the experimental-ness merely as an insurance policy that allows you to tweak the API based on customer feedback.

But that's an ideal. Sometimes ideal isn't workable and our job is "to pick the least bad option".

If you think your design is only at a prototype level, you might want a solution that allows you to iterate more quickly. That's where shipping a NuGet package might be helpful. We've done this with some of the generic math APIs in .NET 6, which then got added in-box with .NET 7. Sometimes shipping an OOB isn't easy or even feasible because you need to modify types that have to be in-box; that's partially why we came up with the [Experimental] attribute solution.

I'm not involved closely enough with your design and constraints to answer what approach makes the most sense. However, it seems we're all in agreement that all things being equal, having customers be able to use this new feature in .NET 9 is preferrable over another solution that defers it to .NET 10. So in that sense I'm very supportive of any experimental API shape we can ship in .NET 9.

Hope this makes sense.

miloush commented 4 months ago

you need to modify types that have to be in-box; that's partially why we came up with the [Experimental] attribute solution

That is indeed the case here. Sounds like experimental is the best we can do at the moment.

bartonjs commented 4 months ago

Video

namespace System.Windows
{
    public class Application
    {
        [Experimental]
        public ThemeMode ThemeMode { get; set; }
    }

    public class Window
    {
        [Experimental]
        public ThemeMode ThemeMode { get; set; }
    }

    [Experimental]
    public readonly struct ThemeMode : IEquatable<ThemeMode>
    {
        public static ThemeMode None => new ThemeMode();
        public static ThemeMode System => new ThemeMode("System");
        public static ThemeMode Light => new ThemeMode("Light");
        public static ThemeMode Dark => new ThemeMode("Dark");

        public string Value => _value ?? "None";

        public ThemeMode(string value) => _value = value;

        // + IEquatable members
    }
}
robert-abeo commented 4 months ago

Since this is experimental I realize you are avoiding most feedback; however, for the future:

  1. You are conflating two different ideas here: Theme (Aero2, Fluent, etc.) with Theme "variants" (light, dark, etc.) to use a term from Avalonia. Every XAML framework handles variants differently:
    • WPF has no mechanism. Meaning FluentLight, FluentDark, etc. will all have to be considered separate themes
    • Avalonia has Theme and ThemeVariant
    • UWP/WinUI has RequestedTheme -- but the resource dictionaries themselves can handle light/dark variations by setting the key on the resource dictionary.
  2. This isn't well thought out in terms of None/System/Default. None shouldn't exist. Default should mean match the system. That means both Aero2 on pre-Windows 10 vs Fluent on Windows11 but it also means matching light/dark in this case because WPF has no concept to manage the variants.
  3. As stated, you should absolutely not have magic strings here
  4. You haven't defined high-contrast, etc.
miloush commented 4 months ago

I have to agree the strongly-typed string was an unfortunate decision. The value of having a loose string was especially to support the experimental nature of the API and avoid having to approve further API changes when any of the strings or their set needs to change, for example to include more information than just a theme name. The name ThemeMode is not helping either.

While WPF has cases of enumerated strings as static members, this way of strongly typing is unprecedented in the framework. As also noted above, it doesn't bring any value for the testing, since it would be used mostly in XAML without any autocompletion support. The type will need to eventually go away and if the aim is to minimize future breaking changes, removing a public type and members feels more disruptive than just changing a property type, although formally that is probably equivalent.

Thanks for approving the API though, it would be good to make it easier for more people to try out the theming!

(@robert-abeo WPF has ThemeColor)

robert-abeo commented 4 months ago

@miloush

(@robert-abeo WPF has ThemeColor)

Please elaborate as I'm not familiar with ThemeColor

terrajobst commented 4 months ago

(FYI, I'm not a WPF expert but was part of the API review group, so my comments are a reflection of how we understood the feature.)

@robert-abeo

  1. Not sure you watched the review, but this was discussed. The conclusion was that there is inherent overlap between what is already there and what is being added as both are sometimes referred to as "theme". We landed on calling it ThemeMode, which is similar to what you call it ("theme variant"). That said, I think a big part of why we think shipping something here is better than nothing is precisely to figure out how the new built-in style support for dark mode will interplay with the ecosystem. This includes both terminology as well as interop (i.e. when one is hosted in the other). That also applies to the WinForm/WPF interop.

  2. None usually means "the user hasn't made a decision". For regular enums, this is usually equivalent to the value 0. In C# terms that's default(ThemeMode), which means "the .NET runtime decides for you" while System would mean "use the OS configuration". For instance, .NET 9 might decide to use Light while .NET 10 might use System. That said, I think it's not unreasonable to collapse them. Personally, I wouldn't call it Default and rather go with System because it makes it clear what default means.

  3. My understanding was that there is a desire for strings to support an extension point where a user/component vendor can supply their own themes. WinForms decided to scope out custom theming entirely; it's basically just Light/Dark/System.

  4. High contrast was discussed and my understanding is that it's treated as system-wide override.

@miloush

The value of having a loose string was especially to support the experimental nature of the API and avoid having to approve further API changes when any of the strings or their set needs to change, for example to include more information than just a theme name.

I think using loose strings to avoid review time isn't a good reason. Rather, we should think about the desired UX and ship that. I'd rather cut corners in internal processes than compromise the UX. However, if we do strings then the value of this type is to allow an open-ended set while also making well-known names more discoverable. This pattern is used in the .NET platform in various places and has proven to be an effective UX.

As also noted above, it doesn't bring any value for the testing, since it would be used mostly in XAML without any autocompletion support.

That is a very fair point and one that wasn't considered in the review. If the primary use is indeed from XAML, then this design has no benefits. My assumption was that this would be part of the app's start up code i.e in code-behind.

The type will need to eventually go away and if the aim is to minimize future breaking changes.

If you already know that you don't want this type because you don't think you want to use strings but some other to-be-designed type, then one can argue that this design doesn't help much. However, it also doesn't hurt much because you're basically saying "I believe this feature will work completely differently, I just don't know what it will look like". IOW, you're saying "I expect all users of this features having to completely change what code I expect them to write".

Removing a public type and members feels more disruptive than just changing a property type, although formally that is probably equivalent.

In general yes, removing types is more disruptive than adding/removing members. However, in your case the entire feature (type and member) is opt-in. The only people that can use it are the ones that suppressed the error "this type is marked as experimental, suppress me if you want to use it anyway". So the break only impacts people that have agreed to be broken.

You still want to minimize breaking changes for early adopters but it sounds to me like any design we cook up right now is guaranteed to change anyway, so we might as well choose one that we think is the easiest to use.

Whether or not this approved design will be easier to use is a different question; we didn't consider usage from XAML. If this is the primary use and having this type shape makes it harder, then I think it's reasonable to revert back to the original plain string design.

pomianowski commented 4 months ago

I know this may be wrong BUT leaving aside what is currently implemented there and what maybe we should be added, it seems to me that from the point of view of the UX of a new developer the division into Theme(Kind/Mode) and ThemeVariant seems optimal and future-proof.

enum ThemeMode
{
  System,
  Light,
  Dark
}
default(ThemeMode) = ThemeMode.System

We can consider that we have an Application Theme that is simply light or dark (High contrast is more difficult for me because it can simply be a variant of light or dark), there are no other combinations. And its default value can be System. With ThemeMode.System from Window's point of view means Application, and from Application's point of view means operating system.

By default, the new Application (if it exists) could simply follows the system theme. Likewise, the Window could simply follows the application (or os if it is a dialog).

Variant can be some enum or const that simply suggests the name of the theme file to load.

Maybe something like:

public class ThemeVariant
{
  public const string HighContrastWhite = "HCWhite";
  // or
  public const string HighContrast = "HC";
  // or
  public const string Desert = "HCWhite";
}
namespace System.Windows
{
    public class Application
    {
        [Experimental]
        public ThemeMode Theme { get; set; }

        [Experimental]
        public string ThemeVariant { get; set; }

        // or composite like public sealed record ThemeContext(ThemeMode Mode, ThemeVariant Variant);
        [Experimental]
        public ThemeContext Theme { get; set; }
    }

    public class Window
    {
        [Experimental]
        public ThemeMode Theme { get; set; }

        [Experimental]
        public string ThemeVariant { get; set; }
    }
}

From ThemeContext, we create a decision matrix to load files / select resources / update resources

Mode (Light/Dark) Variant (Name) Result
Light HC HighContrastWhite.xaml
Dark HC HighContrastDark.xaml
Light Default DefaultLight.xaml
Dark Default DefaultDark.xaml
<Button ThemeDictionary="{ThemeDictionary Mode=Dark, Variant=HighContrast}" />
<Button ThemeDictionary="{ThemeDictionary Mode=Light, Variant=Default}" />
From the Windows 11 system themes there's also a table Windows Theme Mode (Light/Dark) Variant (Name)
Light Light Default
Dark Dark Default
Dusk Light HC
Nightsky Dark HC
Sunrise Light CustomPrimary
Captured Motion Dark CustomPrimary
robert-abeo commented 4 months ago

@terrajobst Thanks for some background

We landed on calling it ThemeMode, which is similar to what you call it ("theme variant")

No haven't seen the video and probably won't have time. I'm glad you discussed the nuance here. I would just try to use existing conventions in WinUI as much as possible in the future.

None usually means "the user hasn't made a decision". For regular enums, this is usually equivalent to the value 0. In C# terms that's default(ThemeMode), which means "the .NET runtime decides for you" while System would mean "use the OS configuration". For instance, .NET 9 might decide to use Light while .NET 10 might use System. That said, I think it's not unreasonable to collapse them. Personally, I wouldn't call it Default and rather go with System because it makes it clear what default means.

API context is VERY important here. When you set Theme="None" you are saying "Don't set a theme". That implies you are inserting your own theme resource dictionaries somewhere in the tree I suppose. What this actually is doing is "Default" so my naming is correct. If you used WinUI terminology RequestedTheme=None then I would agree with you.

My understanding was that there is a desire for strings to support an extension point where a user/component vendor can supply their own themes. WinForms decided to scope out custom theming entirely; it's basically just Light/Dark/System.

That does make some sense. Nobody has solved this problem in any XAML framework as far as I know. You just have to manually insert the resource dictionaries.

High contrast was discussed and my understanding is that it's treated as system-wide override.

Ok, that is how it's handled most places anyway so as long as it's supported automatically behind the scenes it makes sense to me.

miloush commented 4 months ago

the division into Theme(Kind/Mode) and ThemeVariant seems optimal and future-proof

More people arrived to the same conclusion, which is probably why WPF originally distinguishes "theme name" and "theme color" (which is also why adding another "theme X" is adding to the confusion). For example, the Luna theme has 3 variants, NormalColor, Homestead, Metallic. More recent themes (AeroLite/Aero2 etc.) have only one color variant, NormalColor. One option is to make the Fluent theme fit into this pattern with color variants of Dark, Light and System. The existing themes typically have high contrast look built-in using triggers, which might or might not provide enough flexibility for the modern concept of high-contrast themes. I can see HC variants being treated as another color variants.

The WPF already seems to have a way to load not only a different theme, but theme from any 3rd party assembly, possibly per-element, using ThemeDictionaryExtension. It accepts assembly name and theme name, but currently does not support specifying the theme color, so I think that scoping out custom theming would therefore be an unnecessary step back for WPF.

Ultimately, we might want a way for developers to specify 1) an assembly 2) theme name 3) theme color, while keeping the XAML syntax to a minimum. Adding 3 new properties to everything is my least favorite option, especially since not all combinations of values will be valid. At the moment I think it will either have to be a custom type carrying a combination of these values, or, quite WPF-naturally and as the existing infrastructure uses, an Uri, possibly utilizing a markup extension as mentioned couple of times above.

However, it also doesn't hurt much because you're basically saying "I believe this feature will work completely differently, I just don't know what it will look like". IOW, you're saying "I expect all users of this features having to completely change what code I expect them to write".

That very accurately describes my understanding of the current state. Not only that, but I am realizing that changing a property type might be a worse outcome if we will have another year of people documenting and blogging about how to use the current API (it's easier to figure out that the required type/property does not exist than that it subtly changed).

My worry also was that having more complex experimental API is an investment in everyone's time and there will be more reluctance to "fix" it later, while I very strongly believe it needs to be fixed. Knowing that the API review team is happy to engage with the WPF team and open to the future changes is much appreciated.

With the approved API being experimental and learning the expectations from both sides I don't have objections going ahead as approved. Thanks @terrajobst for your time and expertise!

dipeshmsft commented 4 months ago

@terrajobst, for setting ThemeMode from XAML we want to add a ThemeModeConverter which allows to set the ThemeMode properties as attributes of Application and Window

namespace System.Windows;

[Experimental]
public class ThemeModeConverter : TypeConverter
{
    public ThemeModeConverter();
    public override bool CanConvertFrom(ITypeDescriptorContext typeDescriptorContext, Type sourceType);
    public override bool CanConvertTo(ITypeDescriptorContext typeDescriptorContext, Type destinationType);
    public override object ConvertFrom(ITypeDescriptorContext typeDescriptorContext, CultureInfo cultureInfo, object source);
    public override object ConvertTo(ITypeDescriptorContext typeDescriptorContext, CultureInfo cultureInfo, object value, Type destinationType);
}
terrajobst commented 4 months ago

I assume this would go into the System.Windows namespace. This makes sense to me.

@dotnet/fxdc unless anyone objects I'll consider this change request approved as well.

terrajobst commented 4 months ago

Consider it approved.