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 DeviceEventFilter on Windows #465

Merged
merged 3 commits into from
Aug 17, 2022
Merged

Add DeviceEventFilter on Windows #465

merged 3 commits into from
Aug 17, 2022

Conversation

wusyong
Copy link
Member

@wusyong wusyong commented Jul 6, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@wusyong wusyong requested a review from a team as a code owner July 6, 2022 06:51
Comment on lines -2054 to -2105
if msg != WM_PAINT {
RedrawWindow(window, ptr::null(), HRGN::default(), RDW_INTERNALPAINT);
}
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason for removing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Calling RedrawWindow will trigger intense WM_PAINT signals which is one of the reason for webview2 delay.
I moved them to each match arm when we really need to handle something.

Copy link
Member

Choose a reason for hiding this comment

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

this is not the same behavior as before and I am afraid it might bring drawbacks and issues for integrations like egui that depend on drawing events. I don't understand drawing events of the event_loop pretty well but I think we should be careful with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This signal is more used for sending clear events. In its WM_PAINT arm, it checks if there are events still need to handle.
Other windows should also have their own WM_PAINT arm to handle drawing event.
I'll open an issue on winit to ask about if this is safe.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

Copy link
Member

Choose a reason for hiding this comment

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

now that the event filter is set for device events properly in 5ac18b4, do we still need this change? should I revert it

@amrbashir
Copy link
Member

winit just merged the device event filter for windows, rust-windowing/winit#2409

@nothingismagick
Copy link
Sponsor Member

nothingismagick commented Aug 11, 2022

Not sure that merely qualifying on win10 (winit's PR) is good enough, ngl:
image

@amrbashir
Copy link
Member

That's the just the PR author testing, it should work fine for >=win7, I checked the MSDN docs for the used apis and they are all supported for win xp and newer (except one flag which was added in win vista)

@wusyong
Copy link
Member Author

wusyong commented Aug 12, 2022

winit just merged the device event filter for windows, rust-windowing/winit#2409

I'm a little busy on mobile support at the moment. I can resume this work once it's done. Or anyone is interested can take it too.

@amrbashir
Copy link
Member

I can take this

Co-authored-by: ajtribick <ajtribick@googlemail.com>
@wusyong
Copy link
Member Author

wusyong commented Aug 17, 2022

@amrbashir I rebased the PR to resolve merge conflict. Might need your approve since this is my PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants