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 : remainingWaypoints Convenience Accessor on TripState #435

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ben-burwood
Copy link

Added remainingWaypoints extension function to TripState, replicating the functionality with the remainingSteps accessor.

Minor change but took me a while to notice remainingWaypoints was even accessible here as casting to the underlying TripStates was not obvious (to me at least...).
For now can cover my use case, would be interested if it is worth including this in the NavigationUiState so that it could be easily accessible from the ViewModel (similarily to remainingSteps)? Didn't implement currently to avoid congesting that class.

@ianthetechie ianthetechie self-requested a review January 30, 2025 11:57
@ianthetechie
Copy link
Contributor

Thanks! @Archdoog do you have any opinions on adding this to NavigationUiState? That seems like a pretty reasonable addition to me.

If we're agreed on that, I can also add the iOS implementation; seems like the same thing will be useful there (no worries on only pushing for PRs for one arch @ben-burwood; I can handle maintaining parity as needed if you're only developing an app on one platform).

@Archdoog
Copy link
Collaborator

This is actually a great prompt to discuss "observing detailed progress". Likely for purposes other than the standard navigation UI. I've actually wanted something like this for the purpose caching basic state, collecting basic journey stats for user history, as well as advanced logging for certain cases.

The key discussion is:

  1. Keep it simple and keep exposing more navigation progress details as done in this PR. This is probably a fair move for items that are likely relatively general use on the core navigation state as it allows simplicity using parts of this object. Anything that's commonly used in UI obviously fits this case.
  2. We whip up a progress observation ticket that's effectively a more detailed but separate stats Flow/Publisher. This would potentially include the full navigation state alongside other detailed params that are easy to capture like distance travelled, etc. This ties directly into our snapshot testing concept as well as general purpose stats collection/output. Historically with Mapbox, this was an iOS delegate/selection of android object callbacks, where you could effectively listen to key events like didArrive, did go off course, progress updated with a user location & progress struct, etc. This made it super easy to add a rich stats/debugging/etc system in one place. We could still use a flow/publisher for this if it was ideal w/ various mappings.

We may want both 1 and 2 to solve this as they provide different use cases.

@ben-burwood to help decide on this specific PR + general direction, how do you feel about this being on a standard UI flow/publisher? Or would it help you long term to get started on this richer status/progress flow/publisher? In other words are your goals UI focused or more service layer?

As an aside, I'm not opposed to merging this with a deprecation flag for now it can be used while we sort out the larger task of 2 if we decide that's ideal.

@ianthetechie
Copy link
Contributor

Thanks for bringing that up! If @ben-burwood's use case is to detect specific events (ex: "cleared a waypoint"), then we should probably start thinking about building out interfaces for observing specific types of events. That will make code easier to reason about than watching the proverbial fire hose for changes in one or two properties.

But as you also said, these are for slightly different use cases. The change in the PR is pretty non-controversial and won't be made obsolete by anything we add; it's just a convenience to make property access more discoverable. Similarly, if someone wants to have UI state driven by remaining waypoints, that seems perfectly legit to me. So I don't think we'd need to label anything as deprecated.

@ben-burwood
Copy link
Author

Hi both,

Thanks for the activity on this, really great to hear all your ideas - still just getting our toes wet with the Ferrostar implementation.

To give some more background as to why this would be useful for us, this is really used in the service layer to monitor the users progress against the given route. One way the waypoint tracking is needed by us is for route deviation handling where we want the ability to react to the user skipping x points in the route and rejoin it to pick up from a later point (e.g. when bypassing a closed road), in this scenario we don't care about the steps that the user takes or doesn't take, just that they are able to re-join the route, skipping as few waypoints as possible.

On reflection, exposing the waypoints in the UI state when not being used (by us anyway) for updating anything about the UI is perhaps the wrong approach, and option 2 really does make more sense here. Coming from Mapbox, and using the ArrivalObserver, having a similar WaypointPassed callback would make for a cleaner implementation... having said that Waypoints are the only event that we really care about this for.

This is not critical for our, potentially niche, usecase as collecting the TripState and filtering from there is sufficient.

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.

3 participants