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: implement new Symbol class #1637

Merged

Conversation

fynngodau
Copy link
Collaborator

Android annotations API proposal implementation PR as a part of #1491. Closes #1493.


  • Symbol class is now usable.
  • Updates should propagate correctly, except for icon updates in case a Symbol is attached to a manager directly.
  • Existing tests for SymbolManager have been updated, and pass.
  • To avoid conflicts, the following classes have temporary names that differ from their intended names:
    • SymbolKSymbol
    • AnnotationKAnnotation
    • AnnotationContainerKAnnotationContainer
  • Some unresolved TODOs will be resolved later on.

@louwers
Copy link
Collaborator

louwers commented Sep 15, 2023

Why are existing tests commented out?

These are only tests for functionality that was marked as deprecated, right?

@fynngodau
Copy link
Collaborator Author

fynngodau commented Sep 15, 2023

@louwers I had to comment out the following classes:

  • Circle, CircleManager, CircleManagerTest
  • Line, LineManager, LineManagerTest
  • Fill, FillManager, FillManagerTest

This was needed to make changes to AnnotationManager without adapting these classes (yet – since it is planned as a next step). Namely, AnnotationManager now deals with instances of KAnnotation and not AbstractAnnotation.

These classes have been commented out because they will be removed, but are still useful for reference in the next step:

  • CircleOptions
  • LineOptions
  • FillOptions
  • all subclasses of OnAnnotationClickListener, OnAnnotationLongClickListener, OnAnnotationDragListener (will be replaced with type aliases as well)

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.

Thanks for the explanation @fynngodau, that makes sense.

@artakka Any chance you can help me review the Kotlin code added in this PR?

@artakka
Copy link
Collaborator

artakka commented Sep 20, 2023

Some questions and suggestions, but overall looks good!

@fynngodau
Copy link
Collaborator Author

fynngodau commented Sep 20, 2023

@artakka Thank you for reviewing. I have added b3c65a1 as a new commit to address your feedback; feel free to Resolve the remaining conversations above if you think everything is in order now.

@louwers louwers merged commit cd693d5 into maplibre:android-annotations Sep 23, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants