[android] Add manage route functionality #10151

gpesquero · 2025-01-28T23:53:44Z

gpesquero commented

2025-01-28 23:53:44 +00:00

This PR adds the "manage route" functionality in Android that enables the re-arranging of routes.

The fell-and-look and functionality is similar to the implementation that is already working in iOS.

Some screenshots:

closes organicmaps/organicmaps#2551

This PR adds the "manage route" functionality in Android that enables the re-arranging of routes. The fell-and-look and functionality is similar to the implementation that is already working in iOS. **Some screenshots:** ![android_1a](https://github.com/user-attachments/assets/2581a0d1-e5b6-4faa-8bbb-9cfdea18be4f) ![android_1b](https://github.com/user-attachments/assets/e70ff26a-a854-4ae0-8bfc-9cc91d4a12d4) ![android_1c](https://github.com/user-attachments/assets/66d01f10-30f2-4b73-b273-216fa862c5f9) - closes https://git.omaps.dev/organicmaps/organicmaps/issues/2551

👍 10 ❤️ 1 🎉 2 🚀 1

Dariton4000 (Migrated from github.com) approved these changes 2025-02-06 06:07:00 +00:00

pastk approved these changes 2025-02-28 08:17:46 +00:00

pastk left a comment

LGTM!
There are some minor things.

LGTM! There are some minor things.

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

pastk commented

2025-02-28 07:58:09 +00:00

    switch (mRoutePoints.get(position).mPointType)

```suggestion switch (mRoutePoints.get(position).mPointType) ```

pastk commented

2025-02-28 08:09:58 +00:00

crosshair button

pastk commented

2025-02-28 08:11:30 +00:00

here its obvious a my location item is present as we just added it

so call mManageRouteListener.showMyLocationIcon() directly

here its obvious a my location item is present as we just added it so call `mManageRouteListener.showMyLocationIcon()` directly

pastk commented

2025-02-28 08:14:34 +00:00

nit:

the type could be easily derived from the position
so storing it looks redundant?
but probably requires some refactoring of the existing code?

nit: the type could be easily derived from the position so storing it looks redundant? but probably requires some refactoring of the existing code?

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

					
				@ -0,0 +74,4 @@

				      case RoutePointInfo.ROUTE_MARK_INTERMEDIATE: // Intermediate stop.

				        TypedArray iconArray = mContext.getResources().obtainTypedArray(R.array.route_stop_icons);

				        iconId = iconArray.getResourceId(mRoutePoints.get(position).mIntermediateIndex,

pastk commented

2025-02-28 08:00:28 +00:00

what would happen if there are more than 20 points?
prob it should default to a generic stop icon?

what would happen if there are more than 20 points? prob it should default to a generic stop icon?

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

					
				@ -0,0 +184,4 @@

				                                          0, true, true, false, myLocation.getLat(),

				                                          myLocation.getLon()));

				    // Update data.

pastk commented

2025-02-28 08:09:04 +00:00

its better to avoid too obvious comments :)

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

pastk commented

2025-02-28 07:42:53 +00:00

It'd be better to fetch a fresh location here as it could've moved since the dialog was created.

Its fine to do in a separate PR.

It'd be better to fetch a fresh location here as it could've moved since the dialog was created. Its fine to do in a separate PR.

pastk commented

2025-02-28 07:46:27 +00:00

also better to check for location availability right here;

ideally the icon would update if location availability changes, but we can live with a simpler flow for now

also better to check for location availability right here; ideally the icon would update if location availability changes, but we can live with a simpler flow for now

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

					
				@ -0,0 +145,4 @@

				        Framework.addRoutePoint(newRoutePoints.get(pos));

				      // Intermediate route points are added sorted by distance.

				      // We have to make sure that they follow the requested order.

pastk commented

2025-02-28 07:52:43 +00:00

So the core optimizes the points order by default and here we re-arrange the points again to force the core to a wanted order.
This logic is strange :)

Wouldn't it be better to just disable the core optimization while adding points?

So the core optimizes the points order by default and here we re-arrange the points again to force the core to a wanted order. This logic is strange :) Wouldn't it be better to just disable the core optimization while adding points?

gpesquero reviewed 2025-02-28 20:16:17 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

					
				@ -0,0 +74,4 @@

				      case RoutePointInfo.ROUTE_MARK_INTERMEDIATE: // Intermediate stop.

				        TypedArray iconArray = mContext.getResources().obtainTypedArray(R.array.route_stop_icons);

				        iconId = iconArray.getResourceId(mRoutePoints.get(position).mIntermediateIndex,

gpesquero commented

2025-02-28 20:16:17 +00:00

Yes, we already have this default/generic stop icon , with id R.drawable.route_point_20, which was also taken from map icons placed here: data/styles/default/light/symbols/route-point-xx.svg

But the code is wrong and it sets as default route point 1. I will change the code to set the default one:

iconId = iconArray.getResourceId(mRoutePoints.get(position).mIntermediateIndex,
                                         R.drawable.route_point_20);

Yes, we already have this default/generic stop icon ![route-point-20](https://github.com/user-attachments/assets/eac6e385-6375-47a6-82ef-e43a7258899e), with id `R.drawable.route_point_20`, which was also taken from map icons placed here: `data/styles/default/light/symbols/route-point-xx.svg` But the code is wrong and it sets as default route point 1. I will change the code to set the default one: ``` iconId = iconArray.getResourceId(mRoutePoints.get(position).mIntermediateIndex, R.drawable.route_point_20); ```

gpesquero reviewed 2025-02-28 20:34:54 +00:00

gpesquero reviewed 2025-02-28 22:37:49 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

gpesquero commented

2025-02-28 22:37:49 +00:00

I've removed the mMyLocation class variable and included the fetching of a fresh location:

// Get current location.
MapObject myLocation = MwmApplication.from(getContext()).getLocationHelper().getMyPosition();

// Set 'My Location' as starting point of the route.
if (myLocation != null)
  mManageRouteAdapter.setMyLocationAsStartingPoint(myLocation);

I've removed the `mMyLocation` class variable and included the fetching of a *fresh* location: ``` // Get current location. MapObject myLocation = MwmApplication.from(getContext()).getLocationHelper().getMyPosition(); // Set 'My Location' as starting point of the route. if (myLocation != null) mManageRouteAdapter.setMyLocationAsStartingPoint(myLocation); ```

gpesquero reviewed 2025-02-28 22:39:33 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

gpesquero commented

2025-02-28 22:39:33 +00:00

Done:

@Override
public void showMyLocationIcon(boolean showMyLocationIcon)
{
  // Get current location.
  MapObject myLocation = MwmApplication.from(getContext()).getLocationHelper().getMyPosition();

  UiUtils.showIf(showMyLocationIcon && myLocation != null, mMyLocationImageView);
}

ideally the icon would update if location availability changes, but we can live with a simpler flow for now

Yes, we can leave leave this for later...

Done: ``` @Override public void showMyLocationIcon(boolean showMyLocationIcon) { // Get current location. MapObject myLocation = MwmApplication.from(getContext()).getLocationHelper().getMyPosition(); UiUtils.showIf(showMyLocationIcon && myLocation != null, mMyLocationImageView); } ``` > ideally the icon would update if location availability changes, but we can live with a simpler flow for now Yes, we can leave leave this for later...

gpesquero reviewed 2025-02-28 22:49:06 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

					
				@ -0,0 +145,4 @@

				        Framework.addRoutePoint(newRoutePoints.get(pos));

				      // Intermediate route points are added sorted by distance.

				      // We have to make sure that they follow the requested order.

gpesquero commented

2025-02-28 22:49:06 +00:00

Yes, the logic here is a little bit tricky.

The best would be indeed to have a core function for adding points that does not optimize distances. I dug into the core code and I did not found it direct to have this non-optimizing function, so I decided to implement that trick of later rearranging of the intermediate points.

I could have a second look at the core code to see if we can implements that non-optimizing add stop function.

Yes, the logic here is a little bit *tricky*. The best would be indeed to have a core function for adding points that does not optimize distances. I dug into the core code and I did not found it direct to have this non-optimizing function, so I decided to implement that *trick* of later rearranging of the intermediate points. I could have a second look at the core code to see if we can implements that non-optimizing add stop function.

gpesquero reviewed 2025-02-28 22:49:21 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

gpesquero commented

2025-02-28 22:49:21 +00:00

Modified!

gpesquero reviewed 2025-02-28 22:50:08 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

					
				@ -0,0 +184,4 @@

				                                          0, true, true, false, myLocation.getLat(),

				                                          myLocation.getLon()));

				    // Update data.

gpesquero commented

2025-02-28 22:50:08 +00:00

Comments removed...

gpesquero reviewed 2025-02-28 22:51:22 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

gpesquero commented

2025-02-28 22:51:22 +00:00

Modified!!

gpesquero reviewed 2025-02-28 23:22:35 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

gpesquero commented

2025-02-28 23:22:35 +00:00

Route point type is later used by Framework.addRoutePoint(), so that's the reason to store it.

Route point type is later used by `Framework.addRoutePoint()`, so that's the reason to store it.

pastk reviewed 2025-03-01 02:46:09 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

					
				@ -0,0 +145,4 @@

				        Framework.addRoutePoint(newRoutePoints.get(pos));

				      // Intermediate route points are added sorted by distance.

				      // We have to make sure that they follow the requested order.

pastk commented

2025-03-01 02:46:09 +00:00

And AFAIK the iOS version doesn't optimize the points at all? So there should be some option in the core already.

pastk approved these changes 2025-03-01 02:48:27 +00:00

rtsisyk approved these changes 2025-03-01 09:05:14 +00:00

gpesquero reviewed 2025-03-01 09:35:36 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteAdapter.java

gpesquero commented

2025-03-01 09:35:35 +00:00

Modified!!

gpesquero reviewed 2025-03-01 09:36:20 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

					
				@ -0,0 +145,4 @@

				        Framework.addRoutePoint(newRoutePoints.get(pos));

				      // Intermediate route points are added sorted by distance.

				      // We have to make sure that they follow the requested order.

gpesquero commented

2025-03-01 09:36:20 +00:00

And AFAIK the iOS version doesn't optimize the points at all? So there should be some option in the core already.

I will take a look at the iOS code, to see how it's handled there...

> And AFAIK the iOS version doesn't optimize the points at all? So there should be some option in the core already. I will take a look at the iOS code, to see how it's handled there...

gpesquero reviewed 2025-03-04 21:44:51 +00:00

android/app/src/main/java/app/organicmaps/routing/ManageRouteBottomSheet.java

					
				@ -0,0 +145,4 @@

				        Framework.addRoutePoint(newRoutePoints.get(pos));

				      // Intermediate route points are added sorted by distance.

				      // We have to make sure that they follow the requested order.

gpesquero commented

2025-03-04 21:44:50 +00:00

@pastk: I've reviewed the core code and found that the native function RoutingManager::AddRoutePoint() makes a call to the function ReorderIntermediatePoints().
I've launched PR 10412 to make optional this auto-reordering of intermediate stops, so we can remove this ugly extra-code.

@pastk: I've reviewed the core code and found that the native function `RoutingManager::AddRoutePoint()` makes a call to the function `ReorderIntermediatePoints()`. I've launched [PR 10412](https://git.omaps.dev/organicmaps/organicmaps/pulls/10412) to make optional this auto-reordering of intermediate stops, so we can remove this ugly extra-code.

This repo is archived. You cannot comment on pull requests.

No reviewers

Dariton4000

pastk

rtsisyk