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

Add ActiveEventLoopExtWindows::rwh_06_window_handle #4083

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Jan 17, 2025

  • Tested on all platforms changed
  • Added an entry to the changelog module 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

This can be helpful when I need to listen to a few native win32 messages by using SetWindowSubclass on the winit event loop window target HWND.

Alternative would be for me to create a hidden win32 and listen on messages on it but for simple use cases this is an overkill IMO.

@amrbashir amrbashir requested a review from notgull as a code owner January 17, 2025 01:48
@notgull
Copy link
Member

notgull commented Jan 17, 2025

If the intention is to be able to subclass the window target window, I think it would be better to expose that functionality directly. Maybe just with an "add_subclass" function, in addition to this code:

@kchibisov
Copy link
Member

I'm worried that the current code is more of an implementation detail rather than how it'll always be, from what I can see, since there were issues with this design wrt loop blocking during resize, so I'm not sure whether exposing it makes sense, since we could always change that and supporting API for what we don't need internally would be rather strange.

@amrbashir
Copy link
Contributor Author

If the intention is to be able to subclass the window target window, I think it would be better to expose that functionality directly. Maybe just with an "add_subclass" function, in addition to this code:

any reason to do this? I mean it will basically be the same, get HWND and call SetWindowSubclass. I'd argue that just exposing the HWND is more flexible, as it will allow me to use either windows-sys crate or the full featured windows crate.

@amrbashir
Copy link
Contributor Author

I'm worried that the current code is more of an implementation detail rather than how it'll always be, from what I can see, since there were issues with this design wrt loop blocking during resize, so I'm not sure whether exposing it makes sense, since we could always change that and supporting API for what we don't need internally would be rather strange.

Do you mean that winit maybe, in the future, decide to not depend on a hidden window for the event loop? if that does happen, It will probably include a breaking change and won't be a problem to remove this method as well.

@kchibisov
Copy link
Member

kchibisov commented Jan 17, 2025

I think getting a window handle out of event loop is rather confusing, since there's no window semantically. So juts an API that gets you HWND could be actually better in that case.

@amrbashir
Copy link
Contributor Author

Exposing the HWND directly will make it even a lot nore easier to consume. I was just a bit conservative and wanted to stay close to a similar method for display handle.

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

Successfully merging this pull request may close these issues.

3 participants