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

Improve updates of Win32 window's WindowStyles #12752

Merged
merged 15 commits into from
Sep 26, 2023

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Sep 1, 2023

What does the pull request do?

Improves updates of window styles, reducing the number of calls to SetWindowLong needed to apply an update to a window.
This change computes the required window styles from the window properties, ensuring only the styles we requested persists.
To allow user to set custom styles, 2 new apis are provided;
Win32SpecificOptions.SetWindowStylesCallback : called after the window styles are computed.
Win32SpecificOptions.SetWndProcHookCallback: called as part of the window message handling.

What is the current behavior?

Window styles are updated from the current window styles. Then the relevant styles are toggled and window is updated. Since the styles aren't validated with the window properties, and only when the properties change will their styles be toggled, the window styles can have other unexpected styles set by external applications, and window behavior may differ from normal.

What is the updated/expected behavior with this PR?

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

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@emmauss emmauss requested a review from kekekeks September 1, 2023 12:44
@avaloniaui-team
Copy link
Contributor

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

@MrJul
Copy link
Member

MrJul commented Sep 1, 2023

the window styles can have other unexpected styles set by external applications

What about other styles set by the application itself? There are very valid usages of SetWindowLong with GWL_EXSTYLE (WS_EX_TRANSPARENT or WS_EX_NOACTIVATE come to mind), which will be broken by such a change (or even prevent porting existing apps).

I've definitely seen such usages being suggested as workarounds in some Avalonia issues/discussions, which is fine imo, as Avalonia isn't expected to handle every platform-specific property.

@emmauss emmauss changed the title Inprove updates of Win32 window's WindowStyles Improve updates of Win32 window's WindowStyles Sep 1, 2023
@emmauss
Copy link
Contributor Author

emmauss commented Sep 4, 2023

the window styles can have other unexpected styles set by external applications

What about other styles set by the application itself? There are very valid usages of SetWindowLong with GWL_EXSTYLE (WS_EX_TRANSPARENT or WS_EX_NOACTIVATE come to mind), which will be broken by such a change (or even prevent porting existing apps).

I've definitely seen such usages being suggested as workarounds in some Avalonia issues/discussions, which is fine imo, as Avalonia isn't expected to handle every platform-specific property.

Thanks for your feedback. Win32SpecificOptions.SetWindowStylesCallback has been added to allow setting custom style and exStyle flags

@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from 9eb971e to 286dc6d Compare September 4, 2023 12:11
@MrJul
Copy link
Member

MrJul commented Sep 4, 2023

Thanks for your feedback. Win32SpecificOptions.SetWindowStylesCallback has been added to allow setting custom style and exStyle flags

Thank you!

Any reason why Win32SpecificOptions isn't in Avalonia.Win32? (It's so specific I don't think it belongs in the general platform.)
Moving it to Win32 will also simplify the code by removing the extra interface and directly check for Win32.WindowImpl.

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch 3 times, most recently from a343cdb to 5fc0bc5 Compare September 4, 2023 14:37
@emmauss
Copy link
Contributor Author

emmauss commented Sep 4, 2023

Thanks for your feedback. Win32SpecificOptions.SetWindowStylesCallback has been added to allow setting custom style and exStyle flags

Thank you!

Any reason why Win32SpecificOptions isn't in Avalonia.Win32? (It's so specific I don't think it belongs in the general platform.) Moving it to Win32 will also simplify the code by removing the extra interface and directly check for Win32.WindowImpl.

Having it in controls would allow using the api in the shared ui projects, especially in the multiplatform templates.

@avaloniaui-team
Copy link
Contributor

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

@Gillibald
Copy link
Contributor

Thanks for your feedback. Win32SpecificOptions.SetWindowStylesCallback has been added to allow setting custom style and exStyle flags

Thank you!
Any reason why Win32SpecificOptions isn't in Avalonia.Win32? (It's so specific I don't think it belongs in the general platform.) Moving it to Win32 will also simplify the code by removing the extra interface and directly check for Win32.WindowImpl.

Having it in controls would allow using the api in the shared ui projects, especially in the multiplatform templates.

This is platform-specific code. So I don't see why we wanted to add that to some x-plat template.

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from c31f662 to bd95c0e Compare September 4, 2023 15:54
@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from bd95c0e to 869b5c5 Compare September 12, 2023 15:57
@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from 869b5c5 to 4a5498e Compare September 13, 2023 12:21
@avaloniaui-team
Copy link
Contributor

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

@grokys
Copy link
Member

grokys commented Sep 14, 2023

This is platform-specific code. So I don't see why we wanted to add that to some x-plat template.

I've not been involved in this feature, so I may be wrong but this is my layman's understanding:

Win32SpecificOptions is defined in Avalonia.Controls to support cross-platform code like this:

if (OperatingSystem.IsWindows())
    Win32SpecificOptions.SetWindowStylesCallback(foo);

When running on non-win32 this code will be stripped, and no reference to Avalonia.Win32 or Avalonia.Desktop is necessary. However if Win32SpecificOptions were defined in Avalonia.Win32 then either:

  • All platforms would need a reference to Avalonia.Win32/Avalonia.Desktop whether they actually need to or not
  • You'd need to conditionally compile and conditionally reference Avalonia.Desktop

@kekekeks @emmauss please correct me if I'm wrong.

@kekekeks
Copy link
Member

kekekeks commented Sep 14, 2023

Even if it won't be stripped (e. g. you define it from XAML), the code in Win32SpecificOptions is so small it won't really make a difference.
The problem with having it in Avalonia.Win32 is not just trimming, but bringing the entire backend package when targeting mobile/wasm. It has various build problems.

@avaloniaui-team
Copy link
Contributor

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

@MrJul
Copy link
Member

MrJul commented Sep 14, 2023

I'd argue that if an user needs such a low level platform specific hook, they're either targeting a single platform, or already have conditional compilation/references in place.

Here, the API is simple: it's only two delegates with primitive arguments only. For the sake of the argument, imagine we need some heavy Win32-specific structs instead: personally, I'd find it really weird to have them in the base platform.

[PrivateApi]
public interface IWin32OptionsTopLevelImpl : ITopLevelImpl
{
public delegate (uint style, uint exStyle) CustomWindowStylesCallback(uint style, uint exStyle);
Copy link
Member

Choose a reason for hiding this comment

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

Those delegates are still accessible despite the PrivateApi but I don't know whether it's the expected behavior or not. All PrivateApi members and nested types normally become internal in the ref assemblies, but delegates aren't affected currently. @kekekeks

Copy link
Member

Choose a reason for hiding this comment

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

Callback delegate should be declared in Win32SpecificOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.


namespace Avalonia.Controls.Platform
{
[Unstable]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure Unstable is the right attribute here: both SetXxxCallback methods will be marked as obsolete, effectively discouraging or preventing their usage.

Also, the class should probably be made static to prevent inheritance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Class is now static, and Unstable attribute removed

@kekekeks
Copy link
Member

they're either targeting a single platform, or already have conditional compilation/references in place.

That's not the case for 3rd-party controls that are providing a custom window class. Since our desktop platforms are built for net6.0 it's not possible to conditionally reference Avalonia.Win32 from a 3rd party nuget package.

@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from cf636b7 to 787b5ce Compare September 20, 2023 11:04
@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from 7fd8a09 to 5ead24a Compare September 26, 2023 12:06
@avaloniaui-team
Copy link
Contributor

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

@emmauss emmauss force-pushed the win32_window_styles_cleanup branch from 5ead24a to 0705446 Compare September 26, 2023 12:57
@avaloniaui-team
Copy link
Contributor

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

@maxkatz6 maxkatz6 added this pull request to the merge queue Sep 26, 2023
Merged via the queue into master with commit d171353 Sep 26, 2023
@maxkatz6 maxkatz6 deleted the win32_window_styles_cleanup branch September 26, 2023 19:14
@billhenn
Copy link
Contributor

billhenn commented Dec 6, 2023

Hello, I've noticed that in recent master builds, a no-chrome Window opened in Windows 11 is no longer the size indicated by the Window.Width/Height properties, but it is a decent amount larger. This is for windows with Window.ExtendClientAreaChromeHints set to NoChrome and Window.ExtendClientAreaToDecorationsHint set to true in the window's constructor code-behind.

Previous to this PR, a <Window Width="1024" Height="768"> window with the ExtendClientAreaChromeHints and ExtendClientAreaToDecorationsHint set as above properties would open at 1024x768 size. Now after the PR, it opens at 1040x807 size on my system.

A window with those properties set is only showing a single pixel border all around. My guess is the result SHOULD be that the window opens at 1026x770 (1024x768 plus a pixel on each side) instead of 1040x807 since there is no chrome and Avalonia seems to try and get the window's content to the Window.Width/Height size. The size the no-chrome window now opens at is far too large.

Prior to the PR, the no-chrome window's overall size was the exact Window.Width/Height size, which was close at least to what it should be.

Can this PR be updated to get it working properly for no-chrome windows?

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

Successfully merging this pull request may close these issues.

10 participants