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

Navigate route with rerouting #316

Open
wants to merge 9 commits into
base: v.next
Choose a base branch
from

Conversation

shubham7109
Copy link
Collaborator

@shubham7109 shubham7109 commented Feb 4, 2025

Description

PR to add a new Kotlin sample "Navigate route with rerouting" in Routing and Logistics category.

Links and Data

Sample issue: #1414

What To Review

  • Review the code to make sure it is easy to follow like other samples on Android
  • README.md and README.metadata.json files

@shubham7109 shubham7109 added the New sample New Kotlin sample using ArcGIS Maps SDK label Feb 4, 2025
@shubham7109 shubham7109 self-assigned this Feb 4, 2025
@shubham7109 shubham7109 marked this pull request as ready for review February 5, 2025 17:49
Copy link
Collaborator

@TADraeseke TADraeseke left a comment

Choose a reason for hiding this comment

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

LGTM! Just some suggestions below, but happy to proceed to second review! Nice!

@eri9000 eri9000 self-requested a review February 11, 2025 18:24
Copy link
Collaborator

@eri9000 eri9000 left a comment

Choose a reason for hiding this comment

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

@shubham7109 Great work, nice sample.
Some suggestions.

Comment on lines +185 to +186
// Reset navigation to initial state
resetNavigation()
Copy link
Collaborator

Choose a reason for hiding this comment

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

wondering why this needs to be called on initialization?
In what cases will the navigation needs to be reset when initializing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use cases:

initialize():

  • Sets up location display
  • Sets up text-to-speech
  • Loads map and load route result
  • Tigger reset

resetNavigation():

  • Cancels navigation job. (not needed for initialization)
  • Fix map rotation to north up. (not needed for initialization)
  • Set viewpoint to route result. (needed for initialization & reset)
  • Create the route ahead graphics and add them to the map. (needed for initialization & reset)
  • Set up button states. (needed for initialization & reset)

To avoid repeated code on initialize & resetNavigation, I designed it so initialize loads, calculate the required resources, and then call the reset function to reset navigation to an initial (non-navigating) state. If there is a name change that may work better than resetNavigation, feel free to comment below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the well detailed explanation.

@shubham7109
Copy link
Collaborator Author

Thank you for the review @eri9000, I added in the PR comments & feedback changes, and the PR is now ready for your re-review. 👍

@shubham7109 shubham7109 requested a review from eri9000 February 12, 2025 22:11
Copy link
Collaborator

@eri9000 eri9000 left a comment

Choose a reason for hiding this comment

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

Good work 👍🏼 LGTM

fontWeight = FontWeight.Bold
)
Text(
text = timeRemainingText,
Copy link
Collaborator

Choose a reason for hiding this comment

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

To improve visibility, can you add a space in between the title and text?
applies to the others as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New sample New Kotlin sample using ArcGIS Maps SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants