X11: Report `CursorMoved` when touch event occurs by murarth · Pull Request #1297 · rust-windowing/winit

murarth · 2019-11-30T20:40:51Z

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

murarth · 2019-12-06T21:26:27Z

@dhardy Can you confirm that this PR resolves the issue?

dhardy · 2019-12-07T10:59:37Z

Ah, sorry. Yes, it does, but it causes another more subtle bug because now all touch events fire CursorMoved events (potentially leaving my UI highlighting the wrong button after multiple touch events end).

The solution would be to only fire CursorMoved events for the "first" touch event (i.e. where no other touch id was active at the touch start time).

murarth · 2019-12-07T17:04:10Z

@dhardy I've implemented the fix you described. Does everything seem to be working as expected now?

dhardy · 2019-12-07T22:48:53Z

I tested the commit I made and it worked perfectly, but I just tested this version and it doesn't. The issue is that first_touch will set a new id as soon as the previous "first touch" ends; to match X11 behaviour only a new touch (i.e. Phase::Begin) should be able to become a "first touch".

I didn't think to test a third touch event started after the first ends (but while the second is still active). Both commits fail in this case. Instead, a touch should only be able to become a "first touch" if there are no other active touch events when it starts.

murarth · 2019-12-07T23:11:50Z

@dhardy Oh, what a silly mistake on my part. I'm glad you caught it. I've pushed another fix.

dhardy · 2019-12-07T23:13:14Z

Just tested, and it passes my obscure test, but not the much simpler test of touch and drag across buttons: the first button remains highlighted. I will investigate tomorrow. (Also, touch-cancel events seem to be causing issues but this may be my UI).

dhardy

Corrected version here (tested): https://github.com/dhardy/winit/commits/x11-touch-cursor-moved

I also took the liberty of removing that complicated data structure we don't actually need. Counting is enough in this case.

dhardy · 2019-12-08T09:30:20Z

src/platform_impl/linux/x11/event_processor.rs

 }
+
+fn is_first_touch(m: &mut HashSet<u64>, id: u64, phase: TouchPhase) -> bool {
+    let is_first = m.is_empty();


This logic is incorrect except on TouchPhase::Started.

Fixed again. Hopefully.

Also, I changed the code from using a HashSet to a simple count of concurrent running touch events. As long as winit doesn't receive a duplicate Ended event, it should work as well as the HashSet.

dhardy · 2019-12-09T09:27:57Z

Is there any reason you keep re-writing my patches? I already tested mine, now I have to test this one too.

murarth · 2019-12-09T19:13:37Z

@dhardy I didn't realize you were writing any patches. It seems we each arrived at the same solution independently.

dhardy · 2019-12-09T19:23:28Z

The first I created a pull-request against your fork, and the second I linked above.

murarth · 2019-12-09T19:48:09Z

Well, I'm sorry, but I didn't see either of those. I just saw the one comment on my code, so I wrote my own fix, which appears to have essentially the same logic.

If you've tested your version and can confirm that it resolves the issue, I think this PR is ready to be reviewed and merged.

dhardy

Anyway, the ~~above~~ below optimisation is irrelevant. I tested this code and it appears to work correctly, so this gets a 👍 and thanks for the effort!

(For what it's worth, the reason I didn't open a PR myself is that I was unsure if adding an X11-specific workaround was the correct thing to do.)

dhardy · 2019-12-10T09:38:18Z

src/platform_impl/linux/x11/event_processor.rs

+        }
+        TouchPhase::Cancelled | TouchPhase::Ended => {
+            if *first == Some(id) {
+                *first = None;


My understanding is that this is correct, but unnecessary: if num == 0 then first == Some(id) is never evaluated (except on TouchPhase::Ended for the last event, when the above ids are still valid). As soon as the next event starts, first is re-assigned. Thus in my version I didn't bother using an Option here.

murarth · 2019-12-10T20:06:51Z

@goddessfreya I believe this PR is now ready for review.

goddessfreya

lgtm if it works for @dhardy

X11: Report CursorMoved when touch event occurs

9b4f090

murarth mentioned this pull request Nov 30, 2019

On X11, touch events can move the cursor without CursorMoved event #1261

Closed

goddessfreya self-requested a review November 30, 2019 21:57

goddessfreya added DS - x11 Affects the X11 backend, or generally free Unix platforms C - waiting on maintainer A maintainer must review this code labels Dec 1, 2019

goddessfreya approved these changes Dec 1, 2019

View reviewed changes

Merge branch 'master' into x11-touch-cursor-moved

d95a020

Only trigger CursorMoved events for the first touch ID

3c2db88

murarth added 2 commits December 7, 2019 15:58

Fix testing for current touch events

0eb9d63

Merge branch 'master' into x11-touch-cursor-moved

947c646

dhardy reviewed Dec 8, 2019

View reviewed changes

goddessfreya self-requested a review December 8, 2019 17:16

Fix first touch logic

104785f

dhardy approved these changes Dec 10, 2019

View reviewed changes

goddessfreya approved these changes Dec 11, 2019

View reviewed changes

murarth merged commit 1f81e5c into rust-windowing:master Dec 12, 2019

murarth deleted the x11-touch-cursor-moved branch December 12, 2019 00:24

Conversation

murarth commented Nov 30, 2019

Uh oh!

murarth commented Dec 6, 2019

Uh oh!

dhardy commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murarth commented Dec 7, 2019

Uh oh!

dhardy commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murarth commented Dec 7, 2019

Uh oh!

dhardy commented Dec 7, 2019

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

dhardy Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murarth Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

murarth Dec 8, 2019

Choose a reason for hiding this comment

Uh oh!

dhardy commented Dec 9, 2019

Uh oh!

murarth commented Dec 9, 2019

Uh oh!

dhardy commented Dec 9, 2019

Uh oh!

murarth commented Dec 9, 2019

Uh oh!

dhardy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardy Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

murarth commented Dec 10, 2019

Uh oh!

goddessfreya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dhardy commented Dec 7, 2019 •

edited

Loading

dhardy commented Dec 7, 2019 •

edited

Loading

dhardy Dec 8, 2019 •

edited

Loading

dhardy left a comment •

edited

Loading