-
Notifications
You must be signed in to change notification settings - Fork 127
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
Introduce Interactions API #748
base: main
Are you sure you want to change the base?
Conversation
android/src/main/kotlin/com/mapbox/maps/mapbox_maps/Extentions.kt
Outdated
Show resolved
Hide resolved
android/src/main/kotlin/com/mapbox/maps/mapbox_maps/Extentions.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small bits of review
android/src/main/kotlin/com/mapbox/maps/mapbox_maps/MapInterfaceController.kt
Outdated
Show resolved
Hide resolved
android/src/main/kotlin/com/mapbox/maps/mapbox_maps/Extentions.kt
Outdated
Show resolved
Hide resolved
2db19c7
to
0ae5762
Compare
62026a5
to
ff14167
Compare
let filterExpression = try? filter.flatMap { try $0.toExp() } | ||
let radius = arguments["radius"] as? CGFloat | ||
|
||
switch interactionType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this could be simplified somehow, but I can't think of exactly how. Recommendations are most welcome.
lib/src/mapbox_map.dart
Outdated
|
||
/// Add an interaction | ||
@experimental | ||
void addInteraction(Interaction interaction, OnInteraction action) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evil159 -- I'd appreciate a particularly thorough review of this section (setting up the listener and message channel. I model it broadly after our approach to gestures, but would welcome any suggested improvements. What do you think of this system of using both _InteractionListener
and _InteractionsList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the limitations - I think this is perfectly fine way to do it(it is also probably the only way to do it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing that is missing here is the ability to remove interaction.
Interaction interaction, int interactionID) async { | ||
try { | ||
return _channel | ||
.invokeMethod('interactions#add_interaction', <String, dynamic>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evil159 - any better approach here you can think of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be a way to go to define an internal structure in pigeon templates to carry all of this data and define a method that accepts this structure. It will not change much conceptually, but at least basics(serialization, message sending) will be handled by Pigeon.
part of mapbox_maps_flutter; | ||
|
||
// A feature that is a point of interest in the Standard style. | ||
class StandardPoiFeature extends FeaturesetFeature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@persidskiy and @evil159 - I've introduced these Standard-specific types here which also have corresponding FeatureState
s which can be used like this:
StandardBuildingState featureState = StandardBuildingState(highlight: true);
mapboxMap.addInteraction(tapInteraction, (_, FeaturesetFeature feature) {
mapboxMap.setFeatureStateForFeaturesetFeature(feature, featureState);
var buildingFeature = StandardBuildingsFeature(
feature.geometry, feature.properties, feature.state,
id: feature.id);
print("Building feature id: ${buildingFeature.id}");
});
I'd like to push the concept farther and return them instead of the generic FeaturesetFeature
, but I'm running into some limits of what we can do with our Dart and Pigeon setup. I can't think of how we'd pass a typed version of FeaturesetFeature
with OnInteraction
. Any ideas?
typedef void OnInteraction(
MapContentGestureContext context, FeaturesetFeature feature);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that the type system allows this:
interface class MyInterface {}
class MyImplementation implements MyInterface {
String? bar = "bar";
}
class MyImplementation2 implements MyInterface {
String? baz = "baz";
}
class MyInteraction<T extends MyInterface> {
final T impl;
MyInteraction(this.impl);
}
void foo<T extends MyInterface>(MyInteraction<T> i, void Function(T) callback) {
callback(i.impl);
}
void main() {
final i = MyInteraction(MyImplementation());
foo(i, (MyImplementation i) {
print(i.bar);
});
final i2 = MyInteraction(MyImplementation2());
foo(i2, (MyImplementation2 i) {
print(i.baz);
});
}
InteractionType interactionType; | ||
|
||
/// Whether to stop the propagation of the interaction to the map. Defaults to true. | ||
bool stopPropagation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I've included this here rather than in the callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in callback originally because the decision can depend on the feature itself, so you can decide it only in the callback. Let's aligh on this?
ios/mapbox_maps_flutter/Sources/mapbox_maps_flutter/Classes/MapInterfaceController.swift
Show resolved
Hide resolved
ios/mapbox_maps_flutter/Sources/mapbox_maps_flutter/Classes/MapInterfaceController.swift
Show resolved
Hide resolved
lib/src/mapbox_map.dart
Outdated
|
||
/// Add an interaction | ||
@experimental | ||
void addInteraction(Interaction interaction, OnInteraction action) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the limitations - I think this is perfectly fine way to do it(it is also probably the only way to do it :)
lib/src/mapbox_map.dart
Outdated
|
||
/// Add an interaction | ||
@experimental | ||
void addInteraction(Interaction interaction, OnInteraction action) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing that is missing here is the ability to remove interaction.
part of mapbox_maps_flutter; | ||
|
||
// A feature that is a point of interest in the Standard style. | ||
class StandardPoiFeature extends FeaturesetFeature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that the type system allows this:
interface class MyInterface {}
class MyImplementation implements MyInterface {
String? bar = "bar";
}
class MyImplementation2 implements MyInterface {
String? baz = "baz";
}
class MyInteraction<T extends MyInterface> {
final T impl;
MyInteraction(this.impl);
}
void foo<T extends MyInterface>(MyInteraction<T> i, void Function(T) callback) {
callback(i.impl);
}
void main() {
final i = MyInteraction(MyImplementation());
foo(i, (MyImplementation i) {
print(i.bar);
});
final i2 = MyInteraction(MyImplementation2());
foo(i2, (MyImplementation2 i) {
print(i.baz);
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjleonard37 The PR looks good. I left some comments here and there discussing different design choices
StandardBuildingState featureState = StandardBuildingState(highlight: true); | ||
|
||
// Add the tap interaction to the map, set the action to occur when a building is tapped (highlight it) | ||
mapboxMap.addInteraction(tapInteraction, (_, FeaturesetFeature feature) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On iOS/Android the Interaction itself consumes the callback upon construction, but in Flutter the callback is supplied into addInteraction
method. Is it some Flutter limitation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it some Flutter limitation?
Yes, because we need to serialize the Interaction here.
// Add the tap interaction to the map, set the action to occur when a building is tapped (highlight it) | ||
mapboxMap.addInteraction(tapInteraction, (_, FeaturesetFeature feature) { | ||
mapboxMap.setFeatureStateForFeaturesetFeature(feature, featureState); | ||
var buildingFeature = StandardBuildingsFeature( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the interaction generic so that the callback automatically return you the StandardBuildingsFeature?
// Define a tap interaction targeting the POI featureset in the Standard style, including a click radius | ||
// Do not stop propagation of the click event to lower layers | ||
var tapInteractionPOI = TapInteraction(Featureset.standardPoi(), | ||
radius: 10, stopPropagation: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Android/iOS we allow the propagation to be decided dynamically in the callback, because the decision can depend on the feature itself. Can we align on this in Flutter?
InteractionType interactionType; | ||
|
||
/// Whether to stop the propagation of the interaction to the map. Defaults to true. | ||
bool stopPropagation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in callback originally because the decision can depend on the feature itself, so you can decide it only in the callback. Let's aligh on this?
/// A featureset descriptor. | ||
/// | ||
/// The descriptor instance acts as a universal target for interactions or querying rendered features (see 'TapInteraction', 'LongTapInteraction') | ||
class FeaturesetDescriptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this to be generic over the feature type, like in iOS/Android?
@@ -168,6 +168,15 @@ enum ViewAnnotationAnchor { | |||
CENTER, | |||
} | |||
|
|||
/// The type of interaction, either tap/click or longTap/longClick | |||
enum InteractionType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't expose anything like this enum in iOS/Android to avoid problems with extensibility. Instead, only the distinct interaction classes are available.
Can we align on that?
@@ -0,0 +1,83 @@ | |||
part of mapbox_maps_flutter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Android/iOS we started with manual implementation of those classes, but quickly realized that there's no way to align on it, if we don't generate them from the style.
Can we generate them as well in Flutter?
lib/src/mapbox_map.dart
Outdated
/// Add an interaction | ||
@experimental | ||
void addInteraction(Interaction interaction, OnInteraction action) { | ||
interactionsList.interactions[action.hashCode] = _InteractionListener( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the hashCode documentation:
/// Objects that are not equal are allowed to have the same hash code.
So we cannot use hashcode as a key in the dictionary.
Can we generate some Id in the Interaction itself?
lib/src/mapbox_map.dart
Outdated
interactionID: action.hashCode, | ||
); | ||
|
||
InteractionsListener.setUp(interactionsList, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set-up this communication each time we use addInteraction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass the list with all interactions, but there is no way to know when the final interaction has been added (unlike gestures where we have a limited set so we can just set up once). This approach overwrites the channel with the new list of all interactions each time one is added so that we can be sure messages can be sent through all of them. Though correct me if I'm thinking about channels incorrectly, @evil159.
This .setup()
is generated by pigeon, so we do not have much flexibility in how we implement it.
@experimental | ||
void addInteraction(Interaction interaction, OnInteraction action) { | ||
interactionsList.interactions[action.hashCode] = _InteractionListener( | ||
onInteractionListener: action, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also by design the interactions order matters, so that latest added interaction executes first. If it returns true, then execution stops. If it returns false
, then the next interaction is invoked.
We need to check this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior in Flutter matches the platform behavior. But that behavior is not as you describe. It varies based on the featureset. Standard Place Labels & Standard POIs work as you describe, and the last added interaction executes first. Standard Buildings is the opposite with the first added triggering first. You can test this in the iOS SDK example:
LongPressInteraction(.standardBuildings) { _, _ in
print("Buildings 1")
return false
}
LongPressInteraction(.standardPoi) { _, _ in
print("POIs 1")
return false
}
LongPressInteraction(.standardBuildings) { _, _ in
print("Buildings 2")
return false
}
LongPressInteraction(.standardPoi) { _, _ in
print("POIs 2")
return false
}
LongPressInteraction(.standardBuildings) { _, _ in
print("Buildings 3")
return false
}
LongPressInteraction(.standardPoi) { _, _ in
print("POIs 3")
return false
}
If you long press (or tap) on a POI over a building this will print:
POIs 3
POIs 2
POIs 1
Buildings 1
Buildings 2
Buildings 3
I replicated this on Android as well. I'm not sure why this is, and we should ticket for investigation.
b433ae0
to
aa8f4ef
Compare
What does this pull request do?
This PR introduces the experimental Interactions API introduced on iOS and Android in 11.8.0-beta.1. Specifically it:
MapboxMap.addInteractions
method, which allows you to add interactions to the map.TapInteraction
andLongTapInteraction
, which allow you to add tap and longTap interactions to the map.FeaturesetDescriptor
-- and convenience descriptors forStandardBuildings
,StandardPOIs
, andStandardPlaceLabels
-- which allow you to describe theFeatureset
s you wantInteractions
to target.queryRenderedFeatures
,querySourceFeatures
,setFeatureState
,getFeatureState
,removeFeatureState
,resetFeatureState
and adds interfaces for iOS and AndroidSimulator.Screen.Recording.-.iPhone.16.-.2025-01-08.at.19.57.40.mp4
Work for another PR:
What is the motivation and context behind this change?
Pull request checklist: