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

support pinch, double tap and rotation gestures on iOS #3130

Merged
merged 32 commits into from
Jan 16, 2024

Conversation

mockersf
Copy link
Contributor

@mockersf mockersf commented Oct 4, 2023

  • Tested on all platforms changed
  • 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

Support existing macOS gestures on iOS

  • renamed TouchpadMagnify to PinchGesture, SmartMagnify to DoubleTapGesture and TouchpadRotate to RotationGesture. Those were supported only on macOS, when adding iOS support it feels better to have more generic names
  • gesture recognition is not active by default on iOS, and is enabled gesture by gesture by the user. this is because enabling gesture recognition may block some touch events while the OS is trying to recognise a gesture, and may not always be what the user want. it's also possible to disable gesture recognition

DoubleTapGesture is a discrete gesture (one event once detection is finished) that can be configured. PinchGesture and RotationGesture are continuous gestures (events for each incremental update) with data.

@mockersf mockersf requested a review from madsmtm as a code owner October 4, 2023 23:17
@mockersf mockersf marked this pull request as draft October 4, 2023 23:17
@alice-i-cecile
Copy link

I really like the renames: those are much clearer, especially in the context of things like games. Gestures being off-by-default makes sense, but that will need to be aggressively documented.

The continuous / discrete distinction is useful; could that be included in module-level docs somewhere?

@mockersf mockersf marked this pull request as ready for review January 2, 2024 16:14
@mockersf mockersf changed the title support pinch and double tap gestures on iOS support pinch, double tap and rotation gestures on iOS Jan 2, 2024
@daxpedda daxpedda added this to the Version 0.30.0 milestone Jan 3, 2024
src/event.rs Show resolved Hide resolved
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks @mockersf, and apologies for not getting to this sooner! I don't know what you could've done better to push me, the continous updating of the branch worked pretty well to keep it on my radar, I've just been a bit hesitant to review iOS additions (since I'm less familiar with those than macOS).

src/event.rs Outdated Show resolved Hide resolved
src/event.rs Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
examples/touchpad_gestures.rs Show resolved Hide resolved
src/platform/ios.rs Show resolved Hide resolved
src/platform_impl/ios/uikit/gesture_recognizer.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member

Maybe we should unify the naming and the meaning of the gestures an probably accumulate them under the same enum? Also, touch screen gestures are different to touchpad ones, usually, but not that much.

For example on Wayland I have swipe, hold, and pinch gestures https://wayland.app/protocols/pointer-gestures-unstable-v1

@kchibisov
Copy link
Member

I'm also not sure how we should handle the touchpad gestures, since it could be weird to have them on ios, but not on desktop platforms, since touchpad gesture recognition is usually done based on the raw events (unlike touchpad gestures) in the clients, thus winit should be able to map them, but I understand that you generally want to run iOS builtin gesture recognition stuff. Maybe winit should handle touchpad gestures with the help of some lib on linux, but it's a question for the future.

Also, I'd suggest to straight rename events without adding aliases, etc, and probably rename to follow the terminology used by wayland since it's more generic and you already did that for pinch.

What do you also think about having the device type attached along these events, so you can identified whether it was touchpad or touchscreen or you generally don't have such information on ios/macOS available?

@madsmtm
Copy link
Member

madsmtm commented Jan 15, 2024

What do you also think about having the device type attached along these events, so you can identified whether it was touchpad or touchscreen or you generally don't have such information on ios/macOS available?

The DeviceId is passed, although for some reason that contains UIScreen on iOS? That seems wrong... Edit: #3402

In general, I think macOS/iOS tries to hide away which device caused a specific event, at least I can't seem to find any info in the docs on how you'd determine it.

Co-authored-by: Mads Marquart <mads@marquart.dk>
@madsmtm
Copy link
Member

madsmtm commented Jan 15, 2024

I took a closer look, I think it could be supported on macOS with https://developer.apple.com/documentation/appkit/nsevent/1530014-deviceid?language=objc, but there is no such thing in UIKit.

@kchibisov
Copy link
Member

DeviceId is quite useless on its own, I was thinking about

enum GestureSource {
    TouchPad,
    TouchScreen,
}

But generally, I'm not sure that these things differ much...

@mockersf
Copy link
Contributor Author

mockersf commented Jan 15, 2024

Also, I'd suggest to straight rename events without adding aliases, etc, and probably rename to follow the terminology used by wayland since it's more generic and you already did that for pinch.

Wayland doesn't seem to have equivalent for rotation and double tap, so that's it. It has hold and swipe, which are called long press and swipe on iOS, I think "long press" is clearer than "hold" but both work for me.

edit: hold is also called long press on Android: https://developer.android.com/reference/android/view/GestureDetector.OnGestureListener#onLongPress(android.view.MotionEvent)

@kchibisov
Copy link
Member

I've pointed wayland gestures in the context of existing macOS gestures since you've renamed a bunch. These are for touchpad, not the touchscreen, touchscreen gestures are client side, so it's irrelevant.

src/platform_impl/ios/uikit/gesture_recognizer.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/view.rs Outdated Show resolved Hide resolved
src/platform_impl/ios/uikit/gesture_recognizer.rs Outdated Show resolved Hide resolved
@madsmtm
Copy link
Member

madsmtm commented Jan 16, 2024

Tested this on my iPad, and fixed a few bugs in the process. The absolute scale for the pinch gesture doesn't match between macOS and iOS, but I'm not sure how to make them, so let's punt on that for now.

@kchibisov
Copy link
Member

The absolute scale for the pinch gesture doesn't match between macOS and iOS,

Could you elaborate on what you mean here? Can't we translate into physical sizes of some sort or it's just something else like values are just different and you can't really translate anything?

@madsmtm madsmtm merged commit 6b29253 into rust-windowing:master Jan 16, 2024
51 checks passed
@madsmtm
Copy link
Member

madsmtm commented Jan 16, 2024

Could you elaborate on what you mean here? Can't we translate into physical sizes of some sort or it's just something else like values are just different and you can't really translate anything?

Oops, merged before I saw your message.

I'm not entirely sure, I guess it's kinda like the MouseScrollDelta event, there isn't really one scale that "makes sense", it depends on what you're doing.

@kchibisov
Copy link
Member

Delta stuff depends, since there's one discrete and there's one pixel delta, if it's pixel it's scaled, if it's discrete it's not scaled and passed as is, but we have separate types to encode such situation.

@mockersf
Copy link
Contributor Author

thanks for testing on your device and fixing those bugs!

@madsmtm
Copy link
Member

madsmtm commented Jan 16, 2024

Delta stuff depends, since there's one discrete and there's one pixel delta, if it's pixel it's scaled, if it's discrete it's not scaled and passed as is, but we have separate types to encode such situation.

I guess the most correct thing would be to have an enum like enum Pinch { OnScreen(f32), TouchPad(f32) }, with the OnScreen value corresponding to pixels on the screen, and TouchPad being arbitrary values; but eh, people are probably gonna have to do something platform-specific here anyhow.

@mockersf when you do get to supporting this in Bevy, feel free to reach out if the API is inconvenient, or you find a way to represent the data correctly, then we can change it in Winit.

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

Successfully merging this pull request may close these issues.

6 participants