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

Feature Gestures #30

Merged
merged 3 commits into from
Apr 11, 2024
Merged

Conversation

hactar
Copy link
Collaborator

@hactar hactar commented Mar 31, 2024

As we discussed in #18 I have added support for tapping map features. I see that you have added gestures already, so I tried to base my addition on your implementation, adding a GestureAction enum to support different kinds of closures for the MapView. I know you guys want to have gestures on DSL layer level too and I agree with that, but as said we need to support it on map level for layers that are not added via DSL, but are baked into the loaded style. For example, to make the countries in the demo preview tap-able, one can now do this:

#Preview("Tappable Countries") {
    MapView(styleURL: demoTilesURL)
        .onTapMapGesture(on: ["countries-fill"], onTapChanged: { _, features in
            print("Tapped on \(features.first)")
        })
        .ignoresSafeArea(.all)
}

So this solution is on MapView level, not on layer level, and can be used for both until we add layer level support later.

I've only done tap now, but adding longPress for this would now be trivial if you are happy with this PR.

This also fixes an issue where an added tap gesture is triggered even though a user double taps the map (which causes a zoom) by requiring that gesture to fail first before our custom gestures can trigger.

Copy link
Collaborator

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

I'm OK with the overall design in principle; thanks! I just need a bit more time to review one section.

Comment on lines +18 to +28
if numberOfTaps == 1 {
// If a user double taps to zoom via the built in gesture, a normal
// tap should not be triggered.
if let doubleTapRecognizer = mapView.gestureRecognizers?
.first(where: {
$0 is UITapGestureRecognizer && ($0 as! UITapGestureRecognizer).numberOfTapsRequired == 2
})
{
gestureRecognizer.require(toFail: doubleTapRecognizer)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to review this a bit further... Feels like there should be a slightly cleaner way...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is a very specific fix and I didn't go deeper to check if there's any other setups we would need to check for.

MapLibre comes with 8 built in gestures, which also have own "require to fail" dependencies:

(lldb) po mapView.gestureRecognizers
▿ Optional<Array<UIGestureRecognizer>>
  ▿ some : 8 elements
    - 0 : <UIPanGestureRecognizer: 0x1056231f0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handlePanGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail-for = {
        <UIPanGestureRecognizer: 0x105623910; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerDragGesture:, target=<MLNMapView 0x1078d6800>)>>
    }>
    - 1 : <UIPinchGestureRecognizer: 0x1056233a0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handlePinchGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail-for = {
        <UITapGestureRecognizer: 0x105623cc0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTouchesRequired = 2>
    }>
    - 2 : <UIRotationGestureRecognizer: 0x105623540; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleRotateGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail-for = {
        <UITapGestureRecognizer: 0x105623cc0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTouchesRequired = 2>
    }>
    - 3 : <UITapGestureRecognizer: 0x1056236a0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleDoubleTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTapsRequired = 2; must-fail-for = {
        <UITapGestureRecognizer: 0x1056243c0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleSingleTapGesture:, target=<MLNMapView 0x1078d6800>)>>,
        <UILongPressGestureRecognizer: 0x105623de0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleQuickZoomGesture:, target=<MLNMapView 0x1078d6800>)>>
    }>
    - 4 : <UIPanGestureRecognizer: 0x105623910; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerDragGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail = {
        <UIPanGestureRecognizer: 0x1056231f0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handlePanGesture:, target=<MLNMapView 0x1078d6800>)>>
    }; must-fail-for = {
        <UITapGestureRecognizer: 0x105623cc0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTouchesRequired = 2>
    }>
    - 5 : <UITapGestureRecognizer: 0x105623cc0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTouchesRequired = 2; must-fail = {
        <UIPinchGestureRecognizer: 0x1056233a0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handlePinchGesture:, target=<MLNMapView 0x1078d6800>)>>,
        <UIPanGestureRecognizer: 0x105623910; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleTwoFingerDragGesture:, target=<MLNMapView 0x1078d6800>)>>,
        <UIRotationGestureRecognizer: 0x105623540; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleRotateGesture:, target=<MLNMapView 0x1078d6800>)>>
    }>
    - 6 : <UILongPressGestureRecognizer: 0x105623de0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleQuickZoomGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail = {
        <UITapGestureRecognizer: 0x1056236a0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleDoubleTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTapsRequired = 2>
    }; must-fail-for = {
        <UITapGestureRecognizer: 0x1056243c0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleSingleTapGesture:, target=<MLNMapView 0x1078d6800>)>>
    }>
    - 7 : <UITapGestureRecognizer: 0x1056243c0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleSingleTapGesture:, target=<MLNMapView 0x1078d6800>)>; must-fail = {
        <UITapGestureRecognizer: 0x1056236a0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleDoubleTapGesture:, target=<MLNMapView 0x1078d6800>)>; numberOfTapsRequired = 2>,
        <UILongPressGestureRecognizer: 0x105623de0; state = Possible; view = <MLNMapView: 0x1078d6800>; target= <(action=handleQuickZoomGesture:, target=<MLNMapView 0x1078d6800>)>>
    }>

Since we don't want to expose the gesture recognizers or MapView to the user, I feel we have to help set up the gestures that a user adds to work with the built in gestures - just like SwiftUI where the motto is "convention over configuration" - the defaults should cover the most likely usage scenarios, ideally with a way of opting out of default behavior. So maybe we should add a further flag called "requireBuiltInGesturesToFail" which per default is true, but a user can set to false if they'd like their gestures to trigger regardless of the built in ones.

There are actually a total of 3 built in gesture recognizers that have numberOfTapsRequired = 2, so more code might be required, but in my testing, this code fixed the "user double tapped the map to zoom in, and yet the onMapGesture closure was run" problem, so I opted for the least invasive implementation for now.

Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

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

Gave this a play in a demo app. I think I'm fine merging. We can always polish and provide better methods to run symbol gestures, but it's already valuable as is.

@ianthetechie ianthetechie merged commit 6e0cf32 into maplibre:main Apr 11, 2024
2 checks passed
@hactar hactar mentioned this pull request May 14, 2024
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.

3 participants