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

Android annotations: click, long-click and dragging #1737

Conversation

fynngodau
Copy link
Collaborator

@fynngodau fynngodau commented Oct 5, 2023

Android annotations API proposal implementation PR as a part of #1491. Closes #1634. To be merged after #1724.


  • Click listeners, long-click listeners and dragging, as well as drag listeners are usable.
  • Existing tests for DraggableAnnotationController have been updated, and pass.
  • The "Add Markers in Bulk" activity in the test app has been modified to allow manual testing: the symbol number 0 toasts when you click on it, and symbol number 1 is draggable.
  • To avoid conflicts, the following classes still have temporary names that differ from their intended names:
    • AnnotationKAnnotation
    • AnnotationContainerKAnnotationContainer
  • It is not yet certain whether any lag while dragging comes primarily from debug builds, or from any new annotations code. I have not been able to find problems with annotations code but would welcome any clues.

@fynngodau fynngodau requested a review from louwers October 5, 2023 14:34
@@ -56,18 +55,16 @@ class PolygonActivity : AppCompatActivity(), OnMapReadyCallback {
override fun onMapReady(map: MapLibreMap) {
maplibreMap = map
map.setStyle(Style.getPredefinedStyle("Streets"))
map.setOnPolygonClickListener { polygon: Polygon ->
/*map.setOnPolygonClickListener { polygon: Fill ->
Toast.makeText(
this@PolygonActivity,
"You clicked on polygon with id = " + polygon.id,
Copy link
Collaborator

@louwers louwers Oct 9, 2023

Choose a reason for hiding this comment

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

Can this example be revived?

map.setOnPolygonClickListener still works right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@louwers

map.setOnPolygonClickListener still works right?

Actually, no, this is not covered by the spec. Probably it should still be added – I think it would be best to do this at the same time that we add functionality for users being able to add managers to MapLibreMap (which, now to notice, is not actually covered by any of the existing issues. I'd suggest to do it after automatic grouping is implemented since we wanted a method that adds a new manager above a specific z layer).

I reinstated the click listener by adding it to the individual polygon.

@louwers
Copy link
Collaborator

louwers commented Oct 9, 2023

I created a trace dragging the annotation around on the Bulk Marker Activity...

image

@louwers
Copy link
Collaborator

louwers commented Oct 9, 2023

The dragging issue seems to get progressively worse with more markers.

Are you iterating over all the markers to handle the drag event?

@fynngodau fynngodau force-pushed the android-annotations-click-drag-listener branch from c92b39a to 96ebea9 Compare October 13, 2023 12:22
Comment on lines 50 to 52
if (annotation is Symbol) annotation.icon?.let { style?.addImage(it.image.toString(), it.image) }
if (annotation is Line) annotation.pattern?.let { style?.addImage(it.toString(), it) }
if (annotation is Fill) annotation.pattern?.let { style?.addImage(it.toString(), it) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you iterating over all the markers to handle the drag event?

@louwers No, like previously, we are only calling AnnotationManager.queryMapForFeatures one time to get the annotation to be dragged when dragging begins. What I considered could be a bottleneck is that we are re-adding the icon image to the map every update. However, commenting out the lines marked above does not noticeably improve the situation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the performance this bad before too?

Let's have a chat on Slack when you have time.

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

There are some performance issues, but it's unclear if they are regressions or not.

I spoke with @fynngodau and we will merge this for now and when the project is complete we can do another pass for performance QA.

@fynngodau fynngodau force-pushed the android-annotations-click-drag-listener branch from 96ebea9 to b940c21 Compare March 20, 2024 10:03
@fynngodau
Copy link
Collaborator Author

fynngodau commented Mar 20, 2024

(Tests fail because we didn't have the render tests at the time. I'll try merging main into android-annotations after this is merged.)

@fynngodau fynngodau merged commit 78a441c into maplibre:android-annotations Mar 20, 2024
3 of 4 checks passed
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.

None yet

2 participants