api: move primary from FingerId to Pointer events #3947

kchibisov · 2024-10-11T17:59:48Z

Whether the pointer event is primary or not generally matters for the
context where all input is done by the same event, so users can
ignore non-primary events since they are likely from users clicking
something else with their other fingers.

Having it only on a FingerId made it useless, since it's usually used
to avoid multi-touch, but if you start mapping on touch event you
already can track things like that yourself.

Fixes #3943.

--

@daxpedda I left the web, since it's starting to get messy with those callbacks, so you might want to refactor things to your liking. I could still plumb those bools though.
That's also the reason PR doesn't really build.

@madsmtm I did only true for ios, which is likely fine, but I guess we could track based on some Option<Touch>? I decided not to, since ios uses pointer casts for its id, etc.

daxpedda

@daxpedda I left the web, since it's starting to get messy with those callbacks, so you might want to refactor things to your liking. I could still plumb those bools though.

Seems you already did.

@madsmtm I did only true for ios, which is likely fine, but I guess we could track based on some Option<Touch>? I decided not to, since ios uses pointer casts for its id, etc.

Pointer cast for ID is the proper way to do this on iOS according to my research I did last time already.
I don't believe its fine to merge it with just true.
Will look into it shortly.

MacOS, Web and Windows LGTM.

src/event.rs

src/platform_impl/apple/uikit/view.rs

src/platform_impl/android/mod.rs

kchibisov · 2024-10-13T18:28:25Z

Pointer cast for ID is the proper way to do this on iOS according to my research I did last time already.
I don't believe its fine to merge it with just true.

true is more correct in general, since false will potentially prevent you from processing valid input.

In general, my main issue is not that to e.g. track fingers like you did, but which styles + finger should result in primary tracking stuff. For example, all ios input is done the same way via touch, as I said, so e.g. pencil/pen/whatever is also going here, and given that ID is a pointer cast, it's not really a touch in some cases, and according to the primary in the web docs, it seems like for pen/whatever it should be true.

daxpedda · 2024-10-13T18:37:34Z

In general, my main issue is not that to e.g. track fingers like you did, but which styles + finger should result in primary tracking stuff. For example, all ios input is done the same way via touch, as I said, so e.g. pencil/pen/whatever is also going here, and given that ID is a pointer cast, it's not really a touch in some cases, and according to the primary in the web docs, it seems like for pen/whatever it should be true.

Yes, good point, we should only apply false for touch input.
I'm gonna go ahead and do just that for Android and iOS.

kchibisov · 2024-10-13T18:44:00Z

I'm gonna go ahead and do just that for Android and iOS.

That already works for Android though the way I've implemented If I read the docs correctly, it's not though for ios here, but its docs are not that clear what things actually are. That's the reason I check those up/down events on android since it tells that some of them are primary and the rest is not primary.

daxpedda

Only reviewed Android, iOS, MacOS, Web and Windows.

I believe before we merge we should let @madsmtm approve and test iOS for us.

kchibisov · 2024-10-13T19:06:24Z

src/platform_impl/apple/uikit/view.rs

+                    let primary = if let UITouchType::Pencil = touch_type {
+                        true
+                    } else {
+                        ivars.fingers.set(ivars.fingers.get() + 1);


Do we actually need all of this set logic? I think just checking the finger_id == self.primary_finger and only updating it, like it's on Android will work the same?

If the primary finger is lifted while other fingers are pressed, it should still be possible to recognize the same finger again when its pressed. By that logic, we should only reset the primary finger ID when all fingers are lifted.

This isn't an issue on Android because ACTION_DOWN and ACTION_UP only get triggered on the primary finger. So which finger exactly is the primary finger, even if it was lifted during a multi-touch interaction, is already tracked for us by the OS.

I agree with @daxpedda's reasoning here. I can imagine a case like this occurring in multitasking mode, where a finger is moved from one application to another and then back again - or maybe if the user lifted the finger so briefly that the OS is able to identify the finger as being the same one.

It'd be nice to have this discussed a bit in a comment in the source code though.

MarijnS95

Android looks good!

MarijnS95 · 2024-10-14T08:05:52Z

src/changelog/unreleased.md

+  - Add `primary` to every `PointerEvent` as a way to identify discard non-primary pointers in a
+    multi-touch interaction.
  - Add `position` to every `PointerEvent`.


Any specific reason why these are duplicated between ### Changed here and ### Added above?

Yeah, its because we didn't really add a new event per se, but we changed the name of an existing event.
But the pointer event overhaul is a bit too big to describe with just "what changed", so the idea was to add what changed on a syntactic level for easy migration in the ### Changed section and add an actual description of the new API in the ### Added one.

I'm happy to take any suggestions!

MarijnS95 · 2024-10-14T08:09:47Z

src/platform_impl/android/mod.rs

+                        MotionAction::Down | MotionAction::PointerDown => {
+                            let primary = action == MotionAction::Down;
+                            if primary {
+                                self.primary_pointer = finger_id.0;


I wonder if we could technically have a debug_assert_eq!(self.primary_pointer, FingerId::dummy()) here, but might be a bit too scary.

I do that plenty of times to check API sanity ... so I'm in favor actually.
Let me know if you approve and I'm happy to add it.

src/platform_impl/android/mod.rs

kchibisov · 2024-10-15T14:49:23Z

I've pushed a separate commit with merging all the FingerId under the same thing and squashed what was present.

madsmtm · 2024-10-25T12:30:55Z

src/event.rs

+        ///
+        /// A pointer is considered primary when it's a mouse, the first finger in a multi-touch
+        /// interaction, or an unknown pointer source.
+        primary: bool,


Would it make sense to link to https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/isPrimary as a reference for this?

I don't think, since it's present on a lot of platforms. At least one could follow up if there will be questions, but linking in docs to external resources when we can explain ourselves is a bit to much.

Something called "primary" is present on many platforms, but we implement the semantics of the web, which is why I thought it useful.

I don't think there's a web semantic, web also forwards platform primary information forward.

I can add, but I think it's pretty clear right now, since it states basically the same as the link, without confusing parts.

src/platform_impl/apple/uikit/view.rs

madsmtm · 2024-10-25T12:35:59Z

src/platform_impl/apple/uikit/view.rs

+                    let primary = if let UITouchType::Pencil = touch_type {
+                        true
+                    } else {
+                        ivars.fingers.set(ivars.fingers.get() + 1);


I agree with @daxpedda's reasoning here. I can imagine a case like this occurring in multitasking mode, where a finger is moved from one application to another and then back again - or maybe if the user lifted the finger so briefly that the OS is able to identify the finger as being the same one.

It'd be nice to have this discussed a bit in a comment in the source code though.

madsmtm · 2024-10-25T12:44:25Z

src/platform_impl/apple/uikit/view.rs

@@ -513,14 +518,35 @@ impl WinitView {
                )
            };
            let window_id = window.id();
-            let finger_id = RootFingerId(FingerId(touch_id));
+            let finger_id = FingerId::from_raw(touch_id);


I think this may be wrong, but have opened #3970 to track there instead.

kchibisov · 2024-10-29T19:39:39Z

Rebased/changed the type to FingerId on ios/and also made Wayland backend to follow ios behavior.

Whether the pointer event is primary or not generally matters for the context where all input is done by the same event, so users can _ignore_ non-primary events since they are likely from users clicking something else with their other fingers. Having it only on a FingerId made it useless, since it's usually used to avoid multi-touch, but if you start mapping on touch event you already can track things like that yourself. Fixes #3943. Co-authored-by: daxpedda <[email protected]>

The same as for `WindowId`.

kchibisov requested review from jackpot51, notgull, daxpedda, madsmtm and MarijnS95 as code owners October 11, 2024 17:59

kchibisov force-pushed the kchibisov/pointer-event-primary branch 3 times, most recently from 1a9cfd7 to d7e8cfe Compare October 12, 2024 13:48

daxpedda requested changes Oct 12, 2024

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

daxpedda reviewed Oct 12, 2024

View reviewed changes

src/platform_impl/apple/uikit/view.rs Outdated Show resolved Hide resolved

daxpedda reviewed Oct 12, 2024

View reviewed changes

src/platform_impl/android/mod.rs Show resolved Hide resolved

kchibisov requested a review from daxpedda October 13, 2024 18:31

daxpedda approved these changes Oct 13, 2024

View reviewed changes

kchibisov commented Oct 13, 2024

View reviewed changes

notgull approved these changes Oct 13, 2024

View reviewed changes

MarijnS95 approved these changes Oct 14, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/pointer-event-primary branch 3 times, most recently from ea1e4b3 to 45ffa9f Compare October 15, 2024 14:48

kchibisov requested a review from daxpedda October 15, 2024 14:48

daxpedda approved these changes Oct 15, 2024

View reviewed changes

madsmtm mentioned this pull request Oct 25, 2024

iOS: FingerId may not be stable #3970

Open

madsmtm approved these changes Oct 25, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/pointer-event-primary branch from 45ffa9f to f34ead6 Compare October 29, 2024 19:36

kchibisov force-pushed the kchibisov/pointer-event-primary branch from f34ead6 to 753c166 Compare October 29, 2024 19:40

kchibisov and others added 2 commits November 2, 2024 17:04

chore: remove platform FingerId

8788675

The same as for `WindowId`.

kchibisov force-pushed the kchibisov/pointer-event-primary branch from 3ec95b2 to 8788675 Compare November 2, 2024 14:05

kchibisov merged commit edfb4b0 into master Nov 2, 2024
58 checks passed

kchibisov deleted the kchibisov/pointer-event-primary branch November 2, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: move primary from FingerId to Pointer events #3947

api: move primary from FingerId to Pointer events #3947

kchibisov commented Oct 11, 2024

daxpedda left a comment

kchibisov commented Oct 13, 2024

daxpedda commented Oct 13, 2024 •

edited

Loading

kchibisov commented Oct 13, 2024 •

edited

Loading

daxpedda left a comment

kchibisov Oct 13, 2024

daxpedda Oct 13, 2024

madsmtm Oct 25, 2024

MarijnS95 left a comment

MarijnS95 Oct 14, 2024

daxpedda Oct 15, 2024

MarijnS95 Oct 14, 2024

daxpedda Oct 15, 2024

kchibisov commented Oct 15, 2024

madsmtm Oct 25, 2024

kchibisov Oct 29, 2024

madsmtm Oct 29, 2024

kchibisov Oct 29, 2024

madsmtm Oct 25, 2024

madsmtm Oct 25, 2024 •

edited

Loading

kchibisov commented Oct 29, 2024

api: move primary from FingerId to Pointer events #3947

api: move primary from FingerId to Pointer events #3947

Conversation

kchibisov commented Oct 11, 2024

daxpedda left a comment

Choose a reason for hiding this comment

kchibisov commented Oct 13, 2024

daxpedda commented Oct 13, 2024 • edited Loading

kchibisov commented Oct 13, 2024 • edited Loading

daxpedda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

kchibisov commented Oct 29, 2024

daxpedda commented Oct 13, 2024 •

edited

Loading

kchibisov commented Oct 13, 2024 •

edited

Loading

madsmtm Oct 25, 2024 •

edited

Loading