Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

Improved icon rendering quality and added support for dark mode icons

Open TomEdwardsEnscape opened this issue 1 year ago • 2 comments

This PR expands and corrects Avalonia's window icon support, particularly for Microsoft Windows.

Win32 icon sizes

On MS Windows, a window has three icons: Big, Small, and Small2. Small2 is requested by other applications, and appears to have been added to support the Taskbar displaying the window's small icon with the platform theme variant (see the "Dark mode" section below).

Previously, Avalonia only set the Big icon, and did not refresh that icon when the window's DPI or theme variant changed. With this PR it provides values for all three icons and updates them when appropriate. This provides higher quality icon rendering:

Comparison image

This PR also adds handling of the WM_ICON message. This message is sent by other applications when they request the local window's icon for their own use, and ought to have special handling because the message can request a DPI different from the window's own. This can happen when the window and taskbar are on different monitors with different DPIs, for example.

To make it easier to produce a window icon with multiple sizes, Avalonia.Win32.DualWindowIcon has been added. This class is constructed with a "big" and a "small" stream, both of which should contain bitmap data. These streams can be any supported bitmap format, including ICO. Providing two ICO streams can be useful because the big/small resolution ranges overlap: a small icon at 200% DPI is 32px, the same as a big icon at 100% DPI.

Lastly, window icons are now set synchronously. Previously they were set with PostMessage, which only enqueues the change. By using SendMessage instead, we avoid a race condition which caused the window sometimes to be briefly drawn without an icon.

Avalonia icon display

Avalonia can also render icons itself, thanks to Skia's support for loading the .ico format. Previously only the largest image within the icon was ever used, resulting in nasty downscaling artefacts. This has been resolved by creating a new bitmap type, Avalonia.Skia.IconBitmap. When rendered, this class uses the existing Skia API to select the icon image with the closest resolution to its on-screen pixel size. This improves image quality by greatly reducing the degree of upscaling or downscaling required.

Before and after:

av_ico

Dark mode

Window icons are displayed by the operating system. On MS Windows, platform UI (e.g. the taskbar) can be in dark mode while a window is in light mode, leading to both light and dark icons being visible at the same time. To support this Avalonia.Controls.ThemeVariantIcons has been added, which is a specialised WindowIcon that defines a light and a dark icon. The appropriate icon will be used for each OS display context, regardless of Avalonia's active theme. ThemeVariantIcons can be constructed in XAML with a markup extension.

Support for platform dark mode has been extended to Windows 10. Previously, while dark mode was supported within the Avalonia window, its title bar would only become dark on Windows 11 or later.

To support dark mode icons within Avalonia, bitmaps (of all supported formats) can now be constructed in XAML with BitmapResourceExtension. By inserting a bitmap resource with the same key into each dictionary within ResourceDictionary.ThemeDictionaries, and then referencing that resource key with DynamicResource, the value of Image.Source can be made dependent on the active theme variant. This technique is demonstrated in ControlCatalog.

Mac OS

Mac doesn't have window icons, but it does have tray icons. There is a complication: starting with OSX 10.16, the system menu changes between light and dark mode according to the brightness of the user's desktop background, regardless of whether the user has selected light or dark mode in System Preferences. Avalonia needs to update each active tray icon when the desktop background causes the system menu to change theme, but this PR does not include this functionality.

Checklist

  • [x] Added unit tests (if possible)?
  • [x] Added XML documentation to any related classes?
  • [ ] Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #11569 Fixes #7784

TomEdwardsEnscape avatar Jul 08 '23 13:07 TomEdwardsEnscape

You can test this PR using the following package version. 11.0.999-cibuild0037550-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

avaloniaui-team avatar Jul 08 '23 13:07 avaloniaui-team

You can test this PR using the following package version. 11.0.999-cibuild0037564-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

avaloniaui-team avatar Jul 09 '23 10:07 avaloniaui-team

What am I supposed to do about the "ValidateApiDif" CI errors?

TomEdwardsEnscape avatar Jul 15 '23 11:07 TomEdwardsEnscape

What am I supposed to do about the "ValidateApiDif" CI errors?

I guess this is the reason and describes what to do: https://github.com/AvaloniaUI/Avalonia/pull/12072

Mrxx99 avatar Jul 15 '23 15:07 Mrxx99

I think this PR should be split into two parts (at least), so we can merge some parts easier:

  1. Skia backend changes with better support of Ico files (I would change IconBitmap to IcoBitmap, as it can be confusing, but not too important)
  2. All of the new APIs types including platform backends support

While I don't see problems with Skia changes, as it is simple and useful. I am not sure about the second.

BitmapResourceExtension

It's something that was possible before in one way or another, but definitely needed to simplify in the future. Unfortunately, the current BitmapResource design is detached from the way how compiler transforms string to bitmap, and has a problem of being sync. While I know we never had support for async bitmap files (including HTTP requests), it basically was a reason why this extension wasn't added before in the form as it is in this PR. We need proper support for bitmaps and fonts loaded from different sources. Not necessarily a markup extension, but might be one as well.

DefaultPlatformSettings.ThemeVariant

Duplicates API, as we have this information in GetColorValues().ThemeVariant

DualWindowIcon

From what I see, this API only exists in Win32 project, which is something we try to avoid, as it forces every application to reference a specific backend, if they want to support this feature even as optional. Also, API with two options "bit" and "small" doesn't feel as extendable. As was discussed in #9354, UWP had scale options and a manifest configuration on top of it, but they generally have quite a different approach from this PR for tailored assets. I feel like it's something that should be discussed well and compared with other frameworks.

ThemeVariantIcons

Same issue as above about API, but while we do support custom theme variants we probably don't need to extend more limited PlatformThemeVariant enum for a while. But still, what if the user has high contrast enabled? UWP could have a structure like:

\Assets\Images\contrast-standard\theme-dark
	\scale-100\logo.png
	\scale-200\logo.png
\Assets\Images\contrast-standard\theme-light
	\scale-100\logo.png
	\scale-200\logo.png
\Assets\Images\contrast-high
	\scale-100\logo.png
	\scale-200\logo.png

And by using it from XAML Source="/Assets/Images/logo.png" it would automatically pick up the best option depending on scale, contrast, and theme variant. By any means, I don't think that UWP approach is the best, as it adds invisible magic to the file paths without any typing and validation, and requires IDE support as well. It still is a more flexible approach though. And I would love to know how it's done in other frameworks like flutter or qt or web.

P.S. we do plan on supporting high-contrast variants in the future, and FluentAvalonia library already does by using GetColorValues() API (more third-party themes can technically support it sooner then we do as well).

maxkatz6 avatar Jul 17 '23 03:07 maxkatz6

What am I supposed to do about the "ValidateApiDif" CI errors?

Locally run "nuke" command. It will create packages and update validation tool suppressions, which then should be committed. It's an assertion that all API/ABI breaking changes are documented in these suppression files.

maxkatz6 avatar Jul 17 '23 03:07 maxkatz6

BitmapResourceExtension

It's something that was possible before in one way or another, but definitely needed to simplify in the future. Unfortunately, the current BitmapResource design is detached from the way how compiler transforms string to bitmap, and has a problem of being sync. While I know we never had support for async bitmap files (including HTTP requests), it basically was a reason why this extension wasn't added before in the form as it is in this PR. We need proper support for bitmaps and fonts loaded from different sources. Not necessarily a markup extension, but might be one as well.

Loading resources synchronously and asynchronously are distinct tasks. I wouldn't try to make one system which supports both.

  • Async loading means that at least one frame is going to be rendered before the resource is loaded. This can lead to annoying flickering when all the images in a window load very quickly. It's better to load synchronously in such cases.
  • Downloading resources from remote locations can require extra configuration such as user authentication. This isn't something that can reasonably be embedded into XAML.
  • Downloading remote resources can in any case reasonably be expected to fail sometimes.

In our WPF solution we always download remote images with viewmodel tasks, even though WPF supports asynchronous image loading. Their system just doesn't provide enough flexibility to meet our needs for authentication and error handling. But for local/embedded image loading, XAML resources (or direct assignment, obv.) are perfect.

Can you expand on the way that the "compiler transforms string to bitmap"? This sounds like something which should be supported properly.

(Irrespective of the above, this type is not central to the PR and could be removed.)

DefaultPlatformSettings.ThemeVariant

Duplicates API, as we have this information in GetColorValues().ThemeVariant

This is a different value. GetColorValues().ThemeVariant is the theme variant of the window, whereas the new properties that this PR adds represent the theme variant of the platform. These can be different.

DualWindowIcon

From what I see, this API only exists in Win32 project, which is something we try to avoid, as it forces every application to reference a specific backend,

Understandable. I don't mind deleting this type, it's a fairly obscure use case. The idea can be brought back after #11384.

"Big" and "small" icons are Win32-specific concepts, they don't translate to any other platforms.

ThemeVariantWindowIcon

But still, what if the user has high contrast enabled?

I view this as something which can be handled via the normal theme resource system, which provides a global solution for all resource types.

Window icons are more complicated solely because the window and platform can have different theme variants at the same time. But as far as I'm aware, high contrast is a global on/off setting on every platform. So if you want to provide high contrast icons, you would simply have a separate ThemeVariantWindowIcon object that is used when the system is in high-contrast mode.

TomEdwardsEnscape avatar Jul 20 '23 08:07 TomEdwardsEnscape

What am I supposed to do about the "ValidateApiDif" CI errors?

Locally run "nuke" command. It will create packages and update validation tool suppressions, which then should be committed. It's an assertion that all API/ABI breaking changes are documented in these suppression files.

I don't have a "nuke" command. Would executing the product of _build.csproj be the same thing? This produces an error because I don't have the web workload installed. I'm using Avalonia.Desktop.slnf to avoid adding this bulk to my system.

Is there a way to run nuke on just specific projects?

TomEdwardsEnscape avatar Jul 20 '23 08:07 TomEdwardsEnscape

Closing this PR temporarily due to inactivity. Please ping me if this needs to be reopened.

jmacato avatar Nov 17 '23 07:11 jmacato

What am I supposed to do about the "ValidateApiDif" CI errors?

Locally run "nuke" command. It will create packages and update validation tool suppressions, which then should be committed. It's an assertion that all API/ABI breaking changes are documented in these suppression files.

I don't have a "nuke" command. Would executing the product of _build.csproj be the same thing? This produces an error because I don't have the web workload installed. I'm using Avalonia.Desktop.slnf to avoid adding this bulk to my system.

Is there a way to run nuke on just specific projects?

You can dotnet run the _build project

Gillibald avatar Nov 20 '23 14:11 Gillibald

@TomEdwardsEnscape Do you have time to resolve current merge conflicts so we can move this further?

Gillibald avatar Nov 20 '23 14:11 Gillibald

I'll be able to look at it after Wednesday. Some massaging will be required due to #13431, things should fall back into place though.

TomEdwardsEnscape avatar Nov 20 '23 15:11 TomEdwardsEnscape

@TomEdwardsEnscape as I mentioned before, can we have this PR split into smaller pieces? Skia backend changes can be merged without any issues, as it doesn't introduce any new public APIs, but improves icons rendering.

maxkatz6 avatar Nov 21 '23 03:11 maxkatz6

Upd: @TomEdwardsEnscape ok, ignore my last message.

After a discussion with @kekekeks, there shouldn't be rendering backend-specific scaling algorithm. Specifically nothing like IconBitmap with custom drawing. Instead, it should be handled on the Composition Renderer layer, and there should be a concept of a mipmap bitmap...

maxkatz6 avatar Nov 21 '23 07:11 maxkatz6

I can remove that feature, it's not business critical for us.

TomEdwardsEnscape avatar Nov 21 '23 08:11 TomEdwardsEnscape

Related? https://github.com/AvaloniaUI/Avalonia/issues/7077

Takoooooo avatar Nov 21 '23 10:11 Takoooooo

@TomEdwardsEnscape what exactly is critical then? If we isolate a specific feature, we can discuss an API for it outside of this PR.

maxkatz6 avatar Nov 21 '23 23:11 maxkatz6

Everything under the "Win32 icon sizes" header except the DualWindowIcon type, and everything under "Dark mode" except the last paragraph (dark mode within Avalonia).

TomEdwardsEnscape avatar Nov 22 '23 08:11 TomEdwardsEnscape

@TomEdwardsEnscape please ping me/us if this PR is ready for review again :pray:

jmacato avatar Nov 29 '23 11:11 jmacato

It's ready.

TomEdwardsEnscape avatar Nov 29 '23 12:11 TomEdwardsEnscape

@TomEdwardsEnscape let me summarize what you need from this PR:

  1. A way to refresh system icons on system theme changed - including window icon and tray menu icon. You implemented this part by introducing new IPlatformSettings event+property, and handling it inside of each IWindowIconImpl per each platform. And by providing new ThemedWindowIcon class.
  2. Handle system events to provide icons of fitting scaling. Specifically on WM_DISPLAYCHANGE and WM_GETICON
  3. There is also BitmapResourceExtension

After discussion, we believe that the proper solution of the first problem would require introducing BitmapSource WPF or BitmapSource UWP API with tailoring built in. Though, we didn't agree if we need the last one, but BitmapSource infra with caching is planned for a while now. As you might have noticed current infrastructure with IPlatformIconLoader doesn't work anymore for extended scenarios.

Instead, we can add SystemThemeVariant information to the PlatformSettings (as I mentioned above), and developers can handle this event to replace the whole WindowIcon on their application side. Possibly adding caching.

Now, handling WM_DISPLAYCHANGE and WM_GETICON is harder in the user application. And I personally believe we can have it in any way if it doesn't change public API surface. It looks in this PR it is only implemented for Windows and for ico files (without using big+small streams, which was Nikita's concern before).

I also don't see reasons to have BitmapResourceExtension in this PR, as it can be simply implemented in the application code. Until we have BitmapSource.

maxkatz6 avatar Nov 30 '23 10:11 maxkatz6

In other words, would it be sufficient to limit this PR to two major changes?

  1. IPlatformSettings with PlatformColorValues.SystemThemeVariant - single public API change.
  2. Handling WM_DISPLAYCHANGE and WM_GETICON on Windows backend when we use ico. Basically most of current Avalonia.Win32 changes, but cleaning up some unused code from previous revisions.

maxkatz6 avatar Nov 30 '23 10:11 maxkatz6

IThemeVariantWindowIconImpl and ThemedWindowIcon are necessary for this feature to work. The icon can be light in the window title bar and dark on the taskbar at the same time, which is the default state in Windows 10. But a window can only have one icon. That one object needs to be able to provide both icon variants.

image

The interface could be internal, but the class must be public.

TomEdwardsEnscape avatar Dec 04 '23 12:12 TomEdwardsEnscape