Add multiselect flag for geometry layers (fill, line) #582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe updates across various files introduce multi-select functionality for layer features in a mapping application. This involves adding a new method to handle multi-selection, updating existing classes to support this feature, and ensuring that the multi-select state is passed through constructors and used in selection logic. The changes span across Android and iOS bridging layers, JNI implementations, and shared public descriptions, enhancing the app's interactive capabilities with vector layers. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
zimmermannubique
approved these changes
Jan 26, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (19)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/tiled/vector/Tiled2dMapVectorLayerSelectionCallbackInterface.kt (2 hunks)
- bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerSelectionCallbackInterface.cpp (2 hunks)
- bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerSelectionCallbackInterface.h (2 hunks)
- bridging/ios/MCTiled2dMapVectorLayerSelectionCallbackInterface+Private.mm (2 hunks)
- bridging/ios/MCTiled2dMapVectorLayerSelectionCallbackInterface.h (1 hunks)
- djinni/map/layers/tiled/vector/tiled_vector_layer.djinni (1 hunks)
- shared/public/BackgroundVectorLayerDescription.h (1 hunks)
- shared/public/LineVectorLayerDescription.h (1 hunks)
- shared/public/PolygonVectorLayerDescription.h (1 hunks)
- shared/public/RasterVectorLayerDescription.h (1 hunks)
- shared/public/SymbolVectorLayerDescription.h (1 hunks)
- shared/public/Tiled2dMapVectorLayerSelectionCallbackInterface.h (2 hunks)
- shared/public/Tiled2dMapVectorTile.h (1 hunks)
- shared/public/VectorLayerDescription.h (2 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (5 hunks)
- shared/src/map/layers/tiled/vector/tiles/Tiled2dMapVectorTile.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (1 hunks)
Additional comments: 25
shared/public/Tiled2dMapVectorLayerSelectionCallbackInterface.h (1)
- 18-18: The addition of
didMultiSelectLayerFeaturesmethod inTiled2dMapVectorLayerSelectionCallbackInterfaceis aligned with the PR's objective to support multi-selection of layer features. Ensure corresponding implementations handle this new method correctly.bridging/ios/MCTiled2dMapVectorLayerSelectionCallbackInterface.h (1)
- 15-18: The addition of
didMultiSelectLayerFeaturesmethod inMCTiled2dMapVectorLayerSelectionCallbackInterfacefor iOS bridging is consistent with the PR's goal to enable multi-selection functionality. Ensure the iOS implementation correctly handles the array ofMCVectorLayerFeatureInfo.shared/public/VectorLayerDescription.h (2)
- 29-29: The addition of the
multiselectboolean member inVectorLayerDescriptionis crucial for enabling or disabling multi-selection on vector layers. This change is necessary for the feature's implementation.- 62-72: The constructor of
VectorLayerDescriptionnow includes themultiselectparameter, ensuring that layer descriptions can specify whether they support multi-selection. This change is essential for the feature's flexibility.shared/src/map/layers/tiled/vector/tiles/Tiled2dMapVectorTile.cpp (1)
- 30-30: Initializing the
multiselectmember variable inTiled2dMapVectorTile's constructor with the value from thedescriptionparameter is a necessary change to support the new multi-selection feature.bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/tiled/vector/Tiled2dMapVectorLayerSelectionCallbackInterface.kt (2)
- 13-14: The addition of
didMultiSelectLayerFeaturesas an abstract function inTiled2dMapVectorLayerSelectionCallbackInterfacefor Kotlin is aligned with the PR's objective to support multi-selection of layer features on Android. Ensure the Android implementation correctly handles the ArrayList ofVectorLayerFeatureInfo.- 38-43: The implementation of
didMultiSelectLayerFeaturesin theCppProxyclass withinTiled2dMapVectorLayerSelectionCallbackInterface.ktensures that the multi-selection functionality is correctly bridged to the native code. This is a critical part of enabling the feature on Android.shared/public/BackgroundVectorLayerDescription.h (1)
- 68-68: The modification to include the
multiselectparameter in the constructor call ofBackgroundVectorLayerDescriptionis necessary for specifying multi-selection capability at the layer level. This change is consistent with the feature's requirements.bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerSelectionCallbackInterface.h (2)
- 37-37: The declaration of
didMultiSelectLayerFeaturesinNativeTiled2dMapVectorLayerSelectionCallbackInterfaceis a necessary addition to support the new multi-selection functionality in the JNI layer for Android.- 46-46: The JNI method ID for
didMultiSelectLayerFeaturesis correctly obtained, ensuring that the new multi-selection functionality can be invoked from the native code on Android.shared/public/Tiled2dMapVectorTile.h (1)
- 80-80: The addition of the
multiselectboolean member variable inTiled2dMapVectorTileis essential for tracking the multi-selection state of vector tiles. This change supports the feature's implementation.shared/public/PolygonVectorLayerDescription.h (1)
- 101-109: The inclusion of the
multiselectparameter in the constructor ofPolygonVectorLayerDescriptionand its subsequent passing to theVectorLayerDescriptionconstructor is a necessary change to support multi-selection for polygon vector layers.djinni/map/layers/tiled/vector/tiled_vector_layer.djinni (1)
- 95-95: The addition of
did_multi_select_layer_featuresto thetiled_2d_map_vector_layer_selection_callback_interfacein the Djinni interface definition file is crucial for enabling multi-selection functionality across different platforms. This change ensures that the feature is accessible through the generated bindings.bridging/ios/MCTiled2dMapVectorLayerSelectionCallbackInterface+Private.mm (2)
- 47-57: The implementation of
didMultiSelectLayerFeaturesinMCTiled2dMapVectorLayerSelectionCallbackInterface+Private.mmcorrectly bridges the multi-selection functionality from Objective-C to C++. This ensures that the feature works as expected on iOS.- 83-91: The C++ proxy implementation for
didMultiSelectLayerFeaturescorrectly handles the conversion from Objective-C types to C++ types, enabling the multi-selection feature's functionality within the native code on iOS.shared/public/LineVectorLayerDescription.h (1)
- 136-144: The inclusion of the
multiselectparameter in the constructor ofLineVectorLayerDescriptionand its subsequent passing to theVectorLayerDescriptionconstructor is a necessary change to support multi-selection for line vector layers. This ensures that the feature is correctly implemented at the layer level.bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerSelectionCallbackInterface.cpp (2)
- 30-40: The implementation of
didMultiSelectLayerFeaturesinNativeTiled2dMapVectorLayerSelectionCallbackInterface.cppcorrectly bridges the multi-selection functionality from Java to C++. This ensures that the feature works as expected on Android.- 69-78: The JNI function
Java_io_openmobilemaps_mapscore_shared_map_layers_tiled_vector_Tiled2dMapVectorLayerSelectionCallbackInterface_00024CppProxy_native_1didMultiSelectLayerFeaturescorrectly handles the conversion from Java types to C++ types, enabling the multi-selection feature's functionality within the native code on Android.shared/public/RasterVectorLayerDescription.h (1)
- 147-147: The addition of a
falseparameter to theVectorLayerDescriptionconstructor withinRasterVectorLayerDescriptioncorrectly implements the multiselect functionality as disabled by default for raster vector layers. Ensure that all existing calls to this constructor are reviewed and updated if necessary to accommodate the new parameter.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (1)
- 330-345: The modifications to the
onClickConfirmedmethod to support multiselect functionality are correctly implemented. The method now collects selected features into a vector whenmultiselectis enabled and appropriately delegates the selection logic based on the state of themultiselectflag. This change aligns with the objective of enabling multiselect functionality for polygon tiles.shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (1)
- 412-428: The modifications to the
onClickConfirmedmethod to support multiselect functionality for line tiles are correctly implemented. The method now collects selected features into a vector whenmultiselectis enabled and appropriately delegates the selection logic based on the state of themultiselectflag. This change aligns with the objective of enabling multiselect functionality for line tiles.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (1)
- 413-428: The modifications to the
onClickConfirmedmethod to support multiselect functionality for polygon pattern tiles are correctly implemented. The method now collects selected features into a vector whenmultiselectis enabled and appropriately delegates the selection logic based on the state of themultiselectflag. This change aligns with the objective of enabling multiselect functionality for polygon pattern tiles.shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (3)
- 233-241: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [226-238]
The implementation correctly introduces a
layerMultiselectboolean variable to capture themultiselectmetadata from the style JSON. However, it's important to ensure that this variable is utilized effectively in all relevant layer descriptions where multiselect functionality is applicable.
- 308-309: The
layerMultiselectvariable is correctly passed to theLineVectorLayerDescriptionconstructor, enabling multiselect functionality for line vector layers as intended.- 387-388: The
layerMultiselectvariable is correctly passed to thePolygonVectorLayerDescriptionconstructor, enabling multiselect functionality for polygon vector layers as intended.
| @@ -510,7 +510,7 @@ class SymbolVectorLayerDescription: public VectorLayerDescription { | |||
| SymbolVectorStyle style, | |||
| std::optional<int32_t> renderPassIndex, | |||
| std::shared_ptr<Value> interactable): | |||
| VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable), | |||
| VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, false), | |||
There was a problem hiding this comment.
The constructor of SymbolVectorLayerDescription initializes VectorLayerDescription with a hardcoded false for the multiselect parameter. This contradicts the PR objectives of adding multiselect functionality.
- VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, false),
+ VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, true),Consider making multiselect a parameter of SymbolVectorLayerDescription's constructor to allow dynamic control over this feature.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, false), | |
| VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, true), |
| @@ -360,7 +365,7 @@ Tiled2dMapVectorLayerParserResult Tiled2dMapVectorLayerParserHelper::parseStyleJ | |||
| filter, | |||
| style, | |||
| renderPassIndex, | |||
| interactable); | |||
| interactable); // in-layer layerMultiselect not yet supported | |||
There was a problem hiding this comment.
The comment indicates that in-layer layerMultiselect support is not yet implemented for SymbolVectorLayerDescription. This is a missed opportunity to fully integrate multiselect functionality across all layer types. Consider extending the multiselect support to symbol vector layers as well.
- interactable); // in-layer layerMultiselect not yet supported
+ interactable, layerMultiselect);Ensure that the SymbolVectorLayerDescription constructor and class definition are updated accordingly to support this parameter.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| interactable); // in-layer layerMultiselect not yet supported | |
| interactable, layerMultiselect); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit