Remove window subclassing

[maroider]

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

As discussed in #1891, using window subclasses exclusively to handle window messages has some unexpected edge cases, such as denying us the ability to react to WM_NCCREATE and WM_CREATE.

Moving away from subclasses may potentially fix alacritty/alacritty#4069. @desmap, if you could check if PR fixes your issue, then that would be awesome.

I'm not entirely happy with the way I solved this as the window creation code assumes that it's ok to create WindowData (SubclassInput) after the window has been created, which is awkward when we also need to pass a pointer to said structure to the window procedure through CreateWindowExW. I recall seeing that some of this logic was inside the window procedure on a previous trip through the repo's git history, so I'm going to dig through the repo's git history to see if it's ok to move the aforementioned code back where I believe it used to be.

[msiglreith]

Haven't looked to deeply into it but would be keeping the subclassing also an option? Inside the newly added window callback, which handles WM_NCREATE and friends, it would set the subclass instead of doing it after the creation step.

[maroider]

Haven't looked to deeply into it but would be keeping the subclassing also an option?

It could be. I'll admit the current approach of removing all sublcassing is a bit of a nuclear one, particularly when it would introduce merge conflicts with other in-flight PRs.

Inside the newly added window callback, which handles WM_NCREATE and friends, it would set the subclass instead of doing it after the creation step.

I think that should be possible, but that would require moving larger portions of code into the window callback, whose invariants are unknown to me. I think the current approach would also have a more significant impact on git blame. If you'd prefer, I can try this approach on another branch and open a separate PR.

[amrbashir]

Thanks for taking time to fix this issue.

There is a couple of bugs that I found:

1. CloseRequested event isn't fired until some interaction with the window like resizing it. Changing the event loop control flow to Poll didn't help.
2. I still can't receive WM_NCCALCSIZE upon window creation because userdata at line 760 will be None and it will need to be handled twice on Line 778 inside public_window_callback and on line 806 inside public_window_callback_inner, I could extract the logic into a function and call it twice in these two spots but it is not ideal and I also need the window_flags inside WM_NCCALCSIZE at window creation.
3. Transparency with a titlebar is broken and I am not sure why, just remove this line to reproduce it and try resizing the window.
   

   winit/examples/transparent.rs

   Line 13 in 657b4fd

   .with_decorations(false)

[maroider]

This was a fun puzzle, but I can't say I'm happy with how the error path in init turned out. public_window_callback could probably also use some more comments.

1. CloseRequested event isn't fired until some interaction with the window like resizing it. Changing the event loop control flow to Poll didn't help.

This should be fixed now. Turns out that not calling DefWindowProcW can have weird results.

2. I still can't receive WM_NCCALCSIZE upon window creation because userdata at line 760 will be None and it will need to be handled twice on Line 778 inside public_window_callback and on line 806 inside public_window_callback_inner, I could extract the logic into a function and call it twice in these two spots but it is not ideal and I also need the window_flags inside WM_NCCALCSIZE at window creation.

This should also be fixed now. You should now be able to stick anything inside public_window_callback_inner and pretend that none of the nasty initialization logic exists.

3. Transparency with a titlebar is broken and I am not sure why, just remove this line to reproduce it and try resizing the window.

I couldn't reproduce this at all. How exactly was it broken?

[amrbashir]

1,2 are solved, thank you.

As for 3, the trasnparent issue, I don't remember if I tested master the last time but right now seems like this PR and master both fail when transparent is used with decorations, I guess it is out of the scope of this PR.

here is a gif to demonstrate though

[maroider]

As for 3, the trasnparent issue, I don't remember if I tested master the last time but right now seems like this PR and master both fail when transparent is used with decorations, I guess it is out of the scope of this PR.

here is a gif to demonstrate though

Thanks for the demonstration, that lets me conclude that we're both seeing the same behaviour. I'll put this down as a non-issue, though, as it shouldn't be a problem for applications that draw anything at all (unlike winit's examples, which don't draw anything at all).

[amrbashir]

well, it still happens even when I attached a webview2 control(might not be the best thing to use for demonstration) to it, minimizing the window and restoring it seems to fix the issue but as mentioned earlier that's out of this PR scope because the same issue exists on the master branch, so we can discuss it in another issue later.

[maroider]

but as mentioned earlier that's out of this PR scope because the same issue exists on the master branch, so we can discuss it in another issue later

You may want to file a new issue in that case, as there doesn't seem to be any existing issues describing this problem.

---

With my latest changes, I believe I have "distilled" the changeset to something vaguely reasonable.
An approach based on moving the point of "subclassing" into the WM_NCCREATE handler would still require some form of the InitData trick I've employed, but should otherwise produce a smaller diff.

If you'd still like me to do so, @msiglreith, I can go ahead and explore the "move the subclassing point" approach now that I've (mostly) scratched this itch.

[msiglreith]

Thanks! A subclassing variant is not necessary I'm fine with either 👍

[maroider]

I'm fine with either 👍

OK. The PR should be ready to be merged now.