Fix pointer position#185850
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request updates the Windows platform implementation to convert pointer event coordinates from screen to client space for WM_POINTERUPDATE, WM_POINTERUP, and WM_POINTERLEAVE messages. Feedback indicates that the fix is incomplete because it does not address WM_POINTERDOWN or other relevant pointer events. Additionally, the changes lack automated regression tests, which are required by the Flutter style guide for engine modifications.
| case WM_POINTERUPDATE: | ||
| case WM_POINTERUP: | ||
| case WM_POINTERLEAVE: { |
There was a problem hiding this comment.
The fix appears to be incomplete as it does not address WM_POINTERDOWN. Like the other WM_POINTER messages, WM_POINTERDOWN also provides screen coordinates in lParam and requires conversion to client coordinates. Please ensure all relevant WM_POINTER messages (including WM_POINTERDOWN and potentially WM_POINTERENTER) are updated to use client coordinates.
There was a problem hiding this comment.
Not sure what the ai meant, this has all cases covered:
case WM_POINTERDOWN:
case WM_POINTERUPDATE:
case WM_POINTERUP:
case WM_POINTERLEAVE: {
There was a problem hiding this comment.
This seems like a hallucination. Marking as closed
|
@mattkae would you be able to review this as well? We'll want to cherry pick this |
Oh wow. Sadly, I don't have any tips here. Good hardware can cut down the build time dramatically, but I know that can be expensive 😞 |
loic-sharma
left a comment
There was a problem hiding this comment.
Thanks for the very fast turnaround, it is very appreciated!
|
The Google testing failures look unrelated to this change: it's a failure on setting IDE breakpoints on Linux. I've restarted Google testing. If that test fails again, we might need to rebase this PR ontop of the latest master commit. |
|
@CodeDoctorDE Could you rebase off latest master? |
After adding ScreenToClient for WM_POINTER*, the old unit tests were still injecting client coordinates in lParam. Real WM_POINTER messages provide screen coordinates, so the tests needed to convert their injected points first.
10156a6 to
592c5d0
Compare
|
Thanks :) |
|
autosubmit label was removed for flutter/flutter/185850, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
Closes #184954.
This pull requests fixes a regression in the POINTER input event handling that was introduced with the stylus support. The regression caused the pointer position to be global instead of relative to the window, which broke touch and stylus input handling in many applications.
Thanks to @dkmcgowan for suggesting a fix. I have updated my flutter input demo to also see the pointer position.
I tested it with the latest flutter master branch which shows the incorrect pointer position, and with my patch which shows the correct pointer position.
This fix should also be backported to the beta branch to ensure that it is included in the next stable release, as it affects a wide range of applications that rely on pointer input events.
For testing this pr, i used my flutter input demo project which can be found here: https://github.com/CodeDoctorDE/flutter-input-demo/tree/ad61c7d6f5011594a90cbb2b9840904144f877b5
PS: I run
et buildto build the engine and it took over 1 1/2 hours to complete. I'm not sure if this is expected or if there is a more efficient way to build the engine to test changes. Any tips on how to speed up the build process would be appreciated.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.