[android] Add manage route functionality #10151

Merged

gpesquero merged 3 commits from github/fork/gpesquero/manage_route into master 2025-03-01 13:06:25 +00:00

Conversation 77 Commits 3 Files changed 86 +1341 -7

gpesquero commented 2025-01-28 23:53:44 +00:00 (Migrated from github.com)

Copy link

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 #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:**    - closes https://git.omaps.dev/organicmaps/organicmaps/issues/2551

gpesquero commented 2025-01-28 23:53:45 +00:00 (Migrated from github.com)

Copy link

requested review from @ghost

requested review from `@ghost`

gpesquero commented 2025-01-28 23:53:45 +00:00 (Migrated from github.com)

Copy link

requested review from @ghost

requested review from `@ghost`

Jean-BaptisteC commented 2025-01-29 05:41:05 +00:00

Member

Copy link

A crash appears when you rotate screen on planning routing screen 🙂

A crash appears when you rotate screen on planning routing screen 🙂

gpesquero commented 2025-01-29 22:25:59 +00:00 (Migrated from github.com)

Copy link

A crash appears when you rotate screen on planning routing screen 🙂

Thanks @Jean-BaptisteC for your testing... You're right!! It crashes on landscape screen mode. I've already fixed it...

And I've added additional code to expand the "manage route" bottom sheet dialog to make sure that it's always fully displayed...

> A crash appears when you rotate screen on planning routing screen 🙂 Thanks `@Jean-BaptisteC` for your testing... You're right!! It crashes on landscape screen mode. I've already fixed it...  And I've added additional code to expand the "manage route" bottom sheet dialog to make sure that it's always fully displayed... 

pastk commented 2025-01-30 06:21:10 +00:00

Owner

Copy link

This is an awesome feature to have, many thanks for working on it!!

Some early feedback:

1. Dragging of points/items is a bit counter-intuitive: a user has to long tap (pause a little) on the handle to start dragging - it'd be better if its possible to drag right away.
2. Its possible to drag / swap to an item next to it only. I.e. to move the item for 2 positions a user has to drag two times separately. It'd be great to move to an arbitrary spot in one go.
3. Initially intermediate stops are automatically re-shuffled by distance from the start. IDK, maybe its fine, but could be confusing also.
4. Try to have two intermediate points and change their position (so that a point closer to the origin comes after one which is further). For me it doesn't work, i.e. the order remains set by distance despite my manual change.

This is an awesome feature to have, many thanks for working on it!! Some early feedback: 1. Dragging of points/items is a bit counter-intuitive: a user has to long tap (pause a little) on the handle to start dragging - it'd be better if its possible to drag right away. 2. Its possible to drag / swap to an item next to it only. I.e. to move the item for 2 positions a user has to drag two times separately. It'd be great to move to an arbitrary spot in one go. 3. Initially intermediate stops are automatically re-shuffled by distance from the start. IDK, maybe its fine, but could be confusing also. 4. Try to have two intermediate points and change their position (so that a point closer to the origin comes after one which is further). For me it doesn't work, i.e. the order remains set by distance despite my manual change.

gpesquero commented 2025-02-01 09:47:43 +00:00 (Migrated from github.com)

Copy link

Some early feedback:

Thanks @pastk for your feedback. I will dig into these issues and get back to you with a fix/proposal.

> Some early feedback: Thanks `@pastk` for your feedback. I will dig into these issues and get back to you with a fix/proposal.

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

Copy link

approved this merge request

approved this merge request

pastk commented 2025-02-07 14:45:30 +00:00

Owner

Copy link

mentioned in issue #2551

mentioned in issue #2551

RobfromVI commented 2025-02-07 19:57:59 +00:00 (Migrated from github.com)

Copy link

Very much looking forward to this!!

Very much looking forward to this!!

gpesquero commented 2025-02-08 00:50:39 +00:00 (Migrated from github.com)

Copy link

Hi @pastk ! I've made some changes...

• It's no longer needed to make a long press to start dragging.
• Now it's possible to drag any stop directly to any other position (not limited to nearby position).
• Arrangement of intermediate stops now seems to work Ok.

One drawback, though:

• I cannot update the route stop icon while dragging (only when the dragging is finished). It's a minor visual issue, but in iOS, the icons are updated while dragging.

Some pending implementations (already available in iOS):

• Enabling route stop deletion by swiping to the left.
• Displaying an 'Own Position' button that sets the current position as starting point.

Please test the current implementation and provide feedback!!

Hi `@pastk` ! I've made some changes... * It's no longer needed to make a long press to start dragging. * Now it's possible to drag any stop directly to any other position (not limited to nearby position). * Arrangement of intermediate stops now seems to work Ok. One drawback, though: * I cannot update the route stop icon while dragging (only when the dragging is finished). It's a minor visual issue, but in iOS, the icons are updated while dragging. Some pending implementations (already available in iOS): * Enabling route stop deletion by swiping to the left. * Displaying an 'Own Position' button that sets the current position as starting point. Please test the current implementation and provide feedback!!

gpesquero commented 2025-02-09 21:35:09 +00:00 (Migrated from github.com)

Copy link

I've just added a 'My Position' icon-button in the top-right of the Manage Route bottom sheet that sets the current position as the starting point of the route.

Following the functionality in iOS, this icon is only displayed if 'My position' is not part of the current route.

Initial route state (own position is not part of the route):

Final route state (after pressing 'My Position' icon button):

I've just added a 'My Position' icon-button in the top-right of the Manage Route bottom sheet that sets the current position as the starting point of the route. Following the functionality in iOS, this icon is only displayed if 'My position' is not part of the current route. **Initial route state (own position is not part of the route):**  **Final route state (after pressing 'My Position' icon button):** 

gpesquero commented 2025-02-17 21:02:59 +00:00 (Migrated from github.com)

Copy link

Hi there!! I've implemented the pending 'Delete route point' functionality in the 'Manage Route' Bottom Sheet dialog.

Instead of using the swipe to the left gesture to show a "Delete" button (as it's implemented in iOS), I've added a 'Delete' icon placed at the right of every route point (this delete icon is displayed only if the number of points in the route is bigger that 2).

Current 'Delete' implementation in iOS:

Proposed 'Delete' implementation in Android:

Why have I chosen to add this delete icon-button instead of the swipe+delete button option? The truth is that I haven't been able to implement a good working swipe functionality, keeping also a fast drag and drop to move the route points.

The delete proposal for Android is not identical to the one implemented in iOS but, to my defense, this also happens in some other parts of OM (for example, bookmarks management also works with left and right swiping in iOS, but with long presses and bottom panels in Android).

Please provide feedback for the current implementation.

Hi there!! I've implemented the pending 'Delete route point' functionality in the 'Manage Route' Bottom Sheet dialog. Instead of using the swipe to the left gesture to show a "Delete" button (as it's implemented in iOS), I've added a 'Delete' icon placed at the right of every route point (this delete icon is displayed only if the number of points in the route is bigger that 2). **Current 'Delete' implementation in iOS:**  **Proposed 'Delete' implementation in Android:**  Why have I chosen to add this delete icon-button instead of the swipe+delete button option? The truth is that I haven't been able to implement a good working swipe functionality, keeping also a fast drag and drop to move the route points. The delete proposal for Android is not identical to the one implemented in iOS but, to my defense, this also happens in some other parts of OM (for example, bookmarks management also works with left and right swiping in iOS, but with long presses and bottom panels in Android). Please provide feedback for the current implementation.

gpesquero commented 2025-02-17 21:13:02 +00:00 (Migrated from github.com)

Copy link

mentioned in merge request !9936

mentioned in merge request !9936

PinguDEVoriginal commented 2025-02-18 12:50:26 +00:00 (Migrated from github.com)

Copy link

I don't know how to build this, but from what I saw, it definitely looks great and I'd love it, thank you!

I don't know how to build this, but from what I saw, it definitely looks great and I'd love it, thank you!

pastk commented 2025-02-22 12:56:10 +00:00

Owner

Copy link

It'd be great to add a video (or a few screenshots) to show how does it look and work on iOS!

It'd be great to add a video (or a few screenshots) to show how does it look and work on iOS!

pastk commented 2025-02-22 12:59:35 +00:00

Owner

Copy link

The delete proposal for Android is not identical to the one implemented in iOS

Its fine. Its more important to have parity in functionality, parity in look&feel is secondary.

And of course its important to follow UX patterns that could differ between the platforms.

E.g. I'm not sure swiping is commonly used on Android to e.g. delete things. But maybe I'm just too oldschool and don't use modern techniques ;)

It'd be great to have input on UX from @organicmaps/design

> The delete proposal for Android is not identical to the one implemented in iOS Its fine. Its more important to have parity in functionality, parity in look&feel is secondary. And of course its important to follow UX patterns that could differ between the platforms. E.g. I'm not sure swiping is commonly used on Android to e.g. delete things. But maybe I'm just too oldschool and don't use modern techniques ;) It'd be great to have input on UX from `@organicmaps/design`

pastk commented 2025-02-22 14:53:57 +00:00

Owner

Copy link

Sorry for the delay with testing.
Its so much better now!

Sorry for the delay with testing. Its so much better now!

pastk commented 2025-02-22 15:05:24 +00:00

Owner

Copy link

I've just added a 'My Position' icon-button in the top-right of the Manage Route bottom sheet that sets the current position as the starting point of the route.

The start item then should be called "My Location" (now its "Your Location").

Also the behavior becomes a bit confusing after "My Location" is moved to be a finish or an intermediate point (e.g. a location arrow will hide the finish / point marker on the map). Would it continue to track your current location if you move? I.e. would the intermediate point position change automatically?

> I've just added a 'My Position' icon-button in the top-right of the Manage Route bottom sheet that sets the current position as the starting point of the route. The start item then should be called "My Location" (now its "Your Location"). Also the behavior becomes a bit confusing after "My Location" is moved to be a finish or an intermediate point (e.g. a location arrow will hide the finish / point marker on the map). Would it continue to track your current location if you move? I.e. would the intermediate point position change automatically?

gpesquero commented 2025-02-23 16:43:00 +00:00 (Migrated from github.com)

Copy link

It'd be great to add a video (or a few screenshots) to show how does it look and work on iOS!

Here are some iOS screenshots (sorry, not possible to have a video from my side):

• Left pic: While route point dragging (I've just noticed that while dragging a "Drag here to remove" message appears on the top area of the screen, allowing the removal of the route stop. This functionality is also NOT implemented in Android).
• Right pic: Route point swiping to the left, where a "Delete button" is shown.

> It'd be great to add a video (or a few screenshots) to show how does it look and work on iOS! Here are some iOS screenshots (sorry, not possible to have a video from my side):   * **Left pic:** While route point dragging (I've just noticed that while dragging a "Drag here to remove" message appears on the top area of the screen, allowing the removal of the route stop. This functionality is also NOT implemented in Android). * **Right pic:** Route point swiping to the left, where a "Delete button" is shown.

gpesquero commented 2025-02-23 18:02:26 +00:00 (Migrated from github.com)

Copy link

The start item then should be called "My Location" (now its "Your Location").

We could change it to "My Position" (The string is already available), but please note that on iOS the string used is "Your Location".

> The start item then should be called "My Location" (now its "Your Location"). We could change it to "My Position" (The string is already available), but please note that on iOS the string used is "Your Location".

gpesquero commented 2025-02-23 18:04:15 +00:00 (Migrated from github.com)

Copy link

Also the behavior becomes a bit confusing after "My Location" is moved to be a finish or an intermediate point (e.g. a location arrow will hide the finish / point marker on the map). Would it continue to track your current location if you move? I.e. would the intermediate point position change automatically?

I have to make further checks in iOS and Android how it behaves if "Your Location" is placed as final destination or as an intermediate stop...

> Also the behavior becomes a bit confusing after "My Location" is moved to be a finish or an intermediate point (e.g. a location arrow will hide the finish / point marker on the map). Would it continue to track your current location if you move? I.e. would the intermediate point position change automatically? I have to make further checks in iOS and Android how it behaves if "Your Location" is placed as final destination or as an intermediate stop...

pastk commented 2025-02-24 03:37:32 +00:00

Owner

Copy link

Using "Your Location" seems inconsistent as e.g. when the location arrow is tapped its called "My Position".

Using "Your Location" seems inconsistent as e.g. when the location arrow is tapped its called "My Position".

hoizan commented 2025-02-25 01:32:48 +00:00 (Migrated from github.com)

Copy link

Tested in latest 2025.02.23-44-Google-beta version. Works great but if there are too many intermediate POIs, UX doesn't handle handle it well. The 'cancel'and 'plan' buttons are out of reach. The POI list should be scrollable.

Tested in latest 2025.02.23-44-Google-beta version. Works great but if there are too many intermediate POIs, UX doesn't handle handle it well. The 'cancel'and 'plan' buttons are out of reach. The POI list should be scrollable. 

RobfromVI commented 2025-02-25 04:52:16 +00:00 (Migrated from github.com)

Copy link

Also testing in Firebase beta 2025.02.23-44

I see that I am able to easily delete and reorder route points, however, I was already able to remove a route point in previous version so the only new feature I can see is to be able to reorder the points

Once I open the manage route with say 4 points, I am no longer able to move the visible part of the map in the upper window

I am also not able to relocate an intermediate route point that I can see.

Is there anything else I should be able to do with this?

Also testing in Firebase beta 2025.02.23-44 I see that I am able to easily delete and reorder route points, however, I was already able to remove a route point in previous version so the only new feature I can see is to be able to reorder the points Once I open the manage route with say 4 points, I am no longer able to move the visible part of the map in the upper window I am also not able to relocate an intermediate route point that I can see. Is there anything else I should be able to do with this?

gpesquero commented 2025-02-25 21:44:55 +00:00 (Migrated from github.com)

Copy link

Using "Your Location" seems inconsistent as e.g. when the location arrow is tapped its called "My Position".

Ok. I will change the string to "My Position"

> Using "Your Location" seems inconsistent as e.g. when the location arrow is tapped its called "My Position". Ok. I will change the string to "My Position"

gpesquero commented 2025-02-25 21:48:16 +00:00 (Migrated from github.com)

Copy link

Tested in latest 2025.02.23-44-Google-beta version. Works great but if there are too many intermediate POIs, UX doesn't handle handle it well. The 'cancel'and 'plan' buttons are out of reach. The POI list should be scrollable.

Thanks @hoizan for your testing and comment! The issue that you're reporting has to be solved indeed!!

I will check how to make the list scrollable (and still compatible with route point dragging).

> Tested in latest 2025.02.23-44-Google-beta version. Works great but if there are too many intermediate POIs, UX doesn't handle handle it well. The 'cancel'and 'plan' buttons are out of reach. The POI list should be scrollable. Thanks `@hoizan` for your testing and comment! The issue that you're reporting has to be solved indeed!! I will check how to make the list scrollable (and still compatible with route point dragging).

gpesquero commented 2025-02-25 22:18:40 +00:00 (Migrated from github.com)

Copy link

Also testing in Firebase beta 2025.02.23-44

Thanks @Rob-from-VI for your testing and comments!!

I see that I am able to easily delete and reorder route points, however, I was already able to remove a route point in previous version so the only new feature I can see is to be able to reorder the points

You're right. It was already possible to remove a route point by clicking on it. This new "Manage Route" functionality is another way to delete route points and also allows the change of the order of the route points.

Once I open the manage route with say 4 points, I am no longer able to move the visible part of the map in the upper window

This "modal" behaviour of the "Manage Route" bottom panel (disabling the access to the main screen) is the expected one, and similar to the implementation in iOS.

I am also not able to relocate an intermediate route point that I can see.

I don't fully undestand what you mean by "relocate an intermediate route point". Move an existing route point? This is not expected to be done in this "Manage Route" panel.

Is there anything else I should be able to do with this?

There's an additional feature: If the current route does not include the "My position" point, an additional button icon in the top-right corner of the "Manage Route" bottom panel appears. If you click on it, your current position will be set as the starting point of the route.

> Also testing in Firebase beta 2025.02.23-44 Thanks `@Rob-from-VI` for your testing and comments!! > I see that I am able to easily delete and reorder route points, however, I was already able to remove a route point in previous version so the only new feature I can see is to be able to reorder the points You're right. It was already possible to remove a route point by clicking on it. This new "Manage Route" functionality is another way to delete route points **and** also allows the change of the order of the route points. > Once I open the manage route with say 4 points, I am no longer able to move the visible part of the map in the upper window This "modal" behaviour of the "Manage Route" bottom panel (disabling the access to the main screen) is the expected one, and similar to the implementation in iOS. > I am also not able to relocate an intermediate route point that I can see. I don't fully undestand what you mean by "relocate an intermediate route point". Move an existing route point? This is not expected to be done in this "Manage Route" panel. > Is there anything else I should be able to do with this? There's an additional feature: If the current route does not include the "My position" point, an additional button icon in the top-right corner of the "Manage Route" bottom panel appears. If you click on it, your current position will be set as the starting point of the route.

RobfromVI commented 2025-02-25 23:59:49 +00:00 (Migrated from github.com)

Copy link

Thanks for clarifying!

Indeed I was hoping drag meant to drag (reposition) a route point on the map itself!!!

I shall keep wishing for this....

Thanks for clarifying! Indeed I was hoping drag meant to drag (reposition) a route point on the map itself!!! I shall keep wishing for this....

patepelo commented 2025-02-26 00:46:19 +00:00

Member

Copy link

Hey @gpesquero This is a really great addition and already working, just tested. What I'd add is the ability to swap down the panel to do the same as "plan route" button.

Hey `@gpesquero` This is a really great addition and already working, just tested. What I'd add is the ability to swap down the panel to do the same as "plan route" button.

pastk commented 2025-02-26 03:54:54 +00:00

Owner

Copy link

Hey @gpesquero This is a really great addition and already working, just tested. What I'd add is the ability to swap down the panel to do the same as "plan route" button.

I like the idea of easy panel dismissal (by swiping or by tapping on the map perhaps).
However I'm not sure whether it should perform a "Plan" action or a "Cancel".

Dismissal usually means "Cancel". But then a user might lose an already re-arranged route.

Would it be possible to re-route immediately after every change (deletion or movement)?

> Hey `@gpesquero` This is a really great addition and already working, just tested. What I'd add is the ability to swap down the panel to do the same as "plan route" button. I like the idea of easy panel dismissal (by swiping or by tapping on the map perhaps). However I'm not sure whether it should perform a "Plan" action or a "Cancel". Dismissal usually means "Cancel". But then a user might lose an already re-arranged route. Would it be possible to re-route immediately after every change (deletion or movement)?

pastk commented 2025-02-26 03:57:48 +00:00

Owner

Copy link

We could change it to "My Position" (The string is already available)

I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position".
So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location").

But let's ask native speakers first!
@RedAuburn @zyphlar

> We could change it to "My Position" (The string is already available) I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position". So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location"). But let's ask native speakers first! ` @RedAuburn` `@zyphlar`

pastk commented 2025-02-26 03:59:29 +00:00

Owner

Copy link

I will check how to make the list scrollable (and still compatible with route point dragging).

Great! If its easily fixable then we might include this feature into an upcoming March 2 release.
If its not then better not to rush and take time to polish.

> I will check how to make the list scrollable (and still compatible with route point dragging). Great! If its easily fixable then we might include this feature into an upcoming March 2 release. If its not then better not to rush and take time to polish.

patepelo commented 2025-02-26 11:33:04 +00:00

Member

Copy link

Dismissal usually means "Cancel". But then a user might lose an already re-arranged route.

Would it be possible to re-route immediately after every change (deletion or movement)?

Yeah, maybe for a part of users, but it'd be more frustrating for someone rearranging and then accidentaly dismissing. I'd make one of this toggable panels, you semi-swipe them down or you touch the map and they semi-hide with results showing on the map.

> Dismissal usually means "Cancel". But then a user might lose an already re-arranged route. > > Would it be possible to re-route immediately after every change (deletion or movement)? Yeah, maybe for a part of users, but it'd be more frustrating for someone rearranging and then accidentaly dismissing. I'd make one of this toggable panels, you semi-swipe them down or you touch the map and they semi-hide with results showing on the map.

RedAuburn commented 2025-02-26 11:40:22 +00:00

Member

Copy link

I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position". So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location").

This all sounds good to me 👍️

> I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position". So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location"). This all sounds good to me 👍️

gpesquero commented 2025-02-26 23:36:33 +00:00 (Migrated from github.com)

Copy link

Great! If its easily fixable then we might include this feature into an upcoming March 2 release.
If its not then better not to rush and take time to polish.

I've added the scroll functionality in the route point list. Size of list view is limited to 5 route points. Please test...

> Great! If its easily fixable then we might include this feature into an upcoming March 2 release. > If its not then better not to rush and take time to polish. I've added the scroll functionality in the route point list. Size of list view is limited to 5 route points. Please test...  

gpesquero commented 2025-02-26 23:45:56 +00:00 (Migrated from github.com)

Copy link

I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position".
So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location").

This change implies a new string with a new translation, so I'd keep it outside of this PR. I can launch an additional PR for this...

> I'm not 100% but my impression is that the most apps refer to it as "My Location". IDK why we use "position". > So I'd suggest to change "Your Location" to "My Location" and use it. (and maybe also change "My Position" to "My Location"). This change implies a new string with a new translation, so I'd keep it outside of this PR. I can launch an additional PR for this...

gpesquero commented 2025-02-27 00:26:32 +00:00 (Migrated from github.com)

Copy link

Dears @Rob-from-VI, @patepelo, @pastk...

Here's a "raw" (not processed) wish-list that I've collected from your comments about an ideal/improved implementation of the "Manage Route" functionality:

• Being able to move the map while managing the route.
• Relocate (move in the map) any route point.
• Swap down the panel to "Plan" (or to cancel?) the route.
• Automatic re-routing for every change (deletion or movement).
• Use of togglable panels to show the map changes.

I understand that the plan is to merge for the time being this PR with the current implementation (which just mimics the basic current implementation in iOS) and afterwards start to discuss/plan (in a new PR) what would be that "ideal/improved" implementation of the "Manage Route" functionality. Am I right?

Dears `@Rob-from-VI`, `@patepelo`, `@pastk...` Here's a "raw" (not processed) wish-list that I've collected from your comments about an ideal/improved implementation of the "Manage Route" functionality: * Being able to move the map while managing the route. * Relocate (move in the map) any route point. * Swap down the panel to "Plan" (or to cancel?) the route. * Automatic re-routing for every change (deletion or movement). * Use of togglable panels to show the map changes. I understand that the plan is to merge for the time being this PR with the current implementation (which just mimics the basic current implementation in iOS) and afterwards start to discuss/plan (in a new PR) what would be that "ideal/improved" implementation of the "Manage Route" functionality. Am I right?

pastk commented 2025-02-27 03:12:52 +00:00

Owner

Copy link

Yes the wishlist from above is for subsequent discussions and PRs.

Its working very good already, let's try to include this great feature in the upcoming release!

@organicmaps/android let's review the code

Yes the wishlist from above is for subsequent discussions and PRs. Its working very good already, let's try to include this great feature in the upcoming release! ` @organicmaps/android` let's review the code

pastk commented 2025-02-27 04:40:21 +00:00

Owner

Copy link

I've added the scroll functionality in the route point list. Size of list view is limited to 5 route points. Please test...

It works, though depending on the theme the scrollbar could be almost invisible, so some users might have hard time to discover it.

In screens like search or bookmarks its easier because one can scroll by holding and dragging any point on the list. But here it'd drag a single item and to scroll one has to drag at the very edge only.
Also I'm a bit concerned that a user might accidentally remove a point as delete buttons are just next to the scroll area.

I'm not sure what would be the best solution here. Maybe adding "drag handles" like in iOS?

Bottomline: I think its good to go as is. Then we'll get more feedback from users and can improve later.

> I've added the scroll functionality in the route point list. Size of list view is limited to 5 route points. Please test... It works, though depending on the theme the scrollbar could be almost invisible, so some users might have hard time to discover it.  In screens like search or bookmarks its easier because one can scroll by holding and dragging any point on the list. But here it'd drag a single item and to scroll one has to drag at the very edge only. Also I'm a bit concerned that a user might accidentally remove a point as delete buttons are just next to the scroll area. I'm not sure what would be the best solution here. Maybe adding "drag handles" like in iOS? Bottomline: I think its good to go as is. Then we'll get more feedback from users and can improve later.

gpesquero commented 2025-02-27 23:04:19 +00:00 (Migrated from github.com)

Copy link

It works, though depending on the theme the scrollbar could be almost invisible, so some users might have hard time to discover it.

I've changed the color of the scrollbar so now it shall be visible in both day and night themes.

In screens like search or bookmarks its easier because one can scroll by holding and dragging any point on the list. But here it'd drag a single item and to scroll one has to drag at the very edge only.

This is the drawback of having enabled the functionality of dragging route points with single-click dragging. The standard mode of the RecyclerView is dragging items with an initial long press. If we would have kept the long-press dragging, scrolling with a large number of route points would be much easier.

Also I'm a bit concerned that a user might accidentally remove a point as delete buttons are just next to the scroll area.

Yes, I've also noticed that the delete buttons are too close to the scroll area. One possible solution: deletion with a long press in the delete icon?

I'm not sure what would be the best solution here. Maybe adding "drag handles" like in iOS?

On iOS, dragging is enabled by clicking and dragging at any place of the route point item (not only in the "drag handles").

> It works, though depending on the theme the scrollbar could be almost invisible, so some users might have hard time to discover it. I've changed the color of the scrollbar so now it shall be visible in both day and night themes. > In screens like search or bookmarks its easier because one can scroll by holding and dragging any point on the list. But here it'd drag a single item and to scroll one has to drag at the very edge only. This is the drawback of having enabled the functionality of dragging route points with single-click dragging. The standard mode of the RecyclerView is dragging items with an initial long press. If we would have kept the long-press dragging, scrolling with a large number of route points would be much easier. > Also I'm a bit concerned that a user might accidentally remove a point as delete buttons are just next to the scroll area. Yes, I've also noticed that the delete buttons are too close to the scroll area. One possible solution: deletion with a long press in the delete icon? > I'm not sure what would be the best solution here. Maybe adding "drag handles" like in iOS? On iOS, dragging is enabled by clicking and dragging at any place of the route point item (not only in the "drag handles").

pastk commented 2025-02-28 05:11:04 +00:00

Owner

Copy link

This is the drawback of having enabled the functionality of dragging route points with single-click dragging. The standard mode of the RecyclerView is dragging items with an initial long press. If we would have kept the long-press dragging, scrolling with a large number of route points would be much easier.

Is it a common behavior in Android apps? Are there popular apps that behave like this? (I can't recall any, but maybe I didn't pay attention).
If its common and there is no problem with users discovering it then its fine to have an initial long tap.

(then having a drag handle icon might help to hint users that items are draggable?)

Yes, I've also noticed that the delete buttons are too close to the scroll area. One possible solution: deletion with a long press in the delete icon?

It sounds like a non-standard behavior and would be not easy to discover.
Instead of a direct delete button we can add a three dots menu which would contain a delete option?
And maybe some other options in the future like "move on the map"? Just a raw idea :)

> This is the drawback of having enabled the functionality of dragging route points with single-click dragging. The standard mode of the RecyclerView is dragging items with an initial long press. If we would have kept the long-press dragging, scrolling with a large number of route points would be much easier. Is it a common behavior in Android apps? Are there popular apps that behave like this? (I can't recall any, but maybe I didn't pay attention). If its common and there is no problem with users discovering it then its fine to have an initial long tap. (then having a drag handle icon might help to hint users that items are draggable?) > Yes, I've also noticed that the delete buttons are too close to the scroll area. One possible solution: deletion with a long press in the delete icon? It sounds like a non-standard behavior and would be not easy to discover. Instead of a direct delete button we can add a three dots menu which would contain a delete option? And maybe some other options in the future like "move on the map"? Just a raw idea :)

pastk commented 2025-02-28 07:30:09 +00:00

Owner

Copy link

nit: ic_my_position_blue.xml and ic_my_location_blue.xml names are confusingly similar

suggestion:
ic_my_position_blue.xml -> ic_location_arrow_blue.xml
ic_my_location_blue.xml -> ic_location_crosshair_blue.xml

nit: `ic_my_position_blue.xml` and `ic_my_location_blue.xml` names are confusingly similar suggestion: `ic_my_position_blue.xml` -> `ic_location_arrow_blue.xml` `ic_my_location_blue.xml` -> `ic_location_crosshair_blue.xml`

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

Owner

Copy link

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

Owner

Copy link

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

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

Owner

Copy link

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?

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

Owner

Copy link

    switch (mRoutePoints.get(position).mPointType)

```suggestion:-0+0 switch (mRoutePoints.get(position).mPointType) ```

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

Owner

Copy link

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?

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

Owner

Copy link

its better to avoid too obvious comments :)

its better to avoid too obvious comments :)

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

Owner

Copy link

crosshair button

crosshair button

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

Owner

Copy link

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

Owner

Copy link

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?

pastk commented 2025-02-28 08:17:46 +00:00

Owner

Copy link

Review: Approved

LGTM!
There are some minor things.

**Review:** Approved LGTM! There are some minor things.

pastk commented 2025-02-28 08:17:46 +00:00

Owner

Copy link

Review: Approved

LGTM!
There are some minor things.

**Review:** Approved LGTM! There are some minor things.

rtsisyk commented 2025-02-28 13:57:50 +00:00

Owner

Copy link

requested review from @ghost

requested review from `@ghost`

rtsisyk commented 2025-02-28 13:57:50 +00:00

Owner

Copy link

requested review from @ghost

requested review from `@ghost`

gpesquero commented 2025-02-28 20:16:17 +00:00 (Migrated from github.com)

Copy link

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 , 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 commented 2025-02-28 20:34:54 +00:00 (Migrated from github.com)

Copy link

I agree with your suggestion. I also think that for communality, we shall change ic_my_location.xml to ic_location_crosshair.xml

I agree with your suggestion. I also think that for communality, we shall change `ic_my_location.xml` to `ic_location_crosshair.xml`

gpesquero commented 2025-02-28 22:37:49 +00:00 (Migrated from github.com)

Copy link

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 commented 2025-02-28 22:39:33 +00:00 (Migrated from github.com)

Copy link

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 commented 2025-02-28 22:49:06 +00:00 (Migrated from github.com)

Copy link

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 commented 2025-02-28 22:49:21 +00:00 (Migrated from github.com)

Copy link

Modified!

Modified!

gpesquero commented 2025-02-28 22:50:08 +00:00 (Migrated from github.com)

Copy link

Comments removed...

Comments removed...

gpesquero commented 2025-02-28 22:51:22 +00:00 (Migrated from github.com)

Copy link

Modified!!

Modified!!

gpesquero commented 2025-02-28 23:22:35 +00:00 (Migrated from github.com)

Copy link

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.

gpesquero commented 2025-02-28 23:47:29 +00:00 (Migrated from github.com)

Copy link

requested review from @ghost

requested review from `@ghost`

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

Owner

Copy link

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

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

pastk commented 2025-03-01 02:48:27 +00:00

Owner

Copy link

approved this merge request

approved this merge request

rtsisyk commented 2025-03-01 09:05:14 +00:00

Owner

Copy link

approved this merge request

approved this merge request

rtsisyk commented 2025-03-01 09:07:19 +00:00

Owner

Copy link

Could you pleass re-base it quickly? It is on external branch, I can't do it on my own.

Could you pleass re-base it quickly? It is on external branch, I can't do it on my own.

gpesquero commented 2025-03-01 09:35:03 +00:00 (Migrated from github.com)

Copy link

Could you pleass re-base it quickly? It is on external branch, I can't do it on my own

Rebased !!

> Could you pleass re-base it quickly? It is on external branch, I can't do it on my own Rebased !!

gpesquero commented 2025-03-01 09:35:35 +00:00 (Migrated from github.com)

Copy link

Modified!!

Modified!!

gpesquero commented 2025-03-01 09:36:20 +00:00 (Migrated from github.com)

Copy link

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...

pastk commented 2025-03-01 10:33:06 +00:00

Owner

Copy link

mentioned in issue #7213

mentioned in issue #7213

rtsisyk merged commit into master 2025-03-01 13:06:25 +00:00

rtsisyk closed this pull request 2025-03-01 13:06:26 +00:00

pastk commented 2025-03-01 13:39:08 +00:00

Owner

Copy link

Many thanks for this great improvement!

Many thanks for this great improvement!

gpesquero commented 2025-03-04 21:43:35 +00:00 (Migrated from github.com)

Copy link

mentioned in merge request !10412

mentioned in merge request !10412

gpesquero commented 2025-03-04 21:44:50 +00:00 (Migrated from github.com)

Copy link

@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.

gpesquero commented 2025-03-07 23:15:25 +00:00 (Migrated from github.com)

Copy link

mentioned in merge request !10444

mentioned in merge request !10444

Dariton4000 (Migrated from github.com) approved these changes 2025-03-22 17:31:51 +00:00

pastk approved these changes 2025-03-22 17:31:51 +00:00

rtsisyk approved these changes 2025-03-22 17:31:51 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

Dariton4000

pastk

rtsisyk

Labels

Clear labels   

Accessibility

  

Address

  

Android

  

Android Auto

  

Android Automotive (AAOS)

  

API

  

AppGallery

  

AppStore

  

Battery and Performance

  

Blocker

  

Bookmarks and Tracks

  

Borders

  

Bug

  

Build

  

CarPlay

  

Classificator

  

Community

  

Core

  

CrashReports

  

Cycling

  

Desktop

  

DevEx

  

DevOps

  

dev_sandbox

  

Directions

  

Documentation

  

Downloader

  

Drape

  

Driving

  

Duplicate

  

Editor

  

Elevation

  

Enhancement

  

Epic

  

External Map Datasets

  

F-Droid

  

Fonts

  

Frequently User Reported

  

Fund

  

Generator

  

Good first issue

  

Google Play

  

GPS

  

GSoC

  

iCloud

  

Icons

  

iOS

  

Legal

  

Linux Desktop

  

Linux packaging

  

Linux Phone

  

Mac OS

  

Map Data

  

Metro

  

Navigation

  

Need Feedback

  

Night Mode

  

NLnet 2024-06-281

  

No Feature Parity

  

Opening Hours

  

Outdoors

  

POI Info

  

Privacy

  

Public Transport

  

Raw Idea

  

Refactoring

  

Regional

  

Regression

  

Releases

  

RoboTest

  

Route Planning

  

Routing

  

Ruler

  

Search

  

Security

  

Styles

  

Tests

  

Track Recording

  

Translations

  

TTS

  

UI

  

UX

  

Walk Navigation

  

Watches

  

Web

  

Wikipedia

  

Windows

  

Won't fix

  

World Map

No labels

Accessibility

Address

Android

Android Auto

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

Blocker

Bookmarks and Tracks

Borders

Bug

Build

CarPlay

Classificator

Community

Core

CrashReports

Cycling

Desktop

DevEx

DevOps

dev_sandbox

Directions

Documentation

Downloader

Drape

Driving

Duplicate

Editor

Elevation

Enhancement

Epic

External Map Datasets

F-Droid

Fonts

Frequently User Reported

Fund

Generator

Good first issue

Google Play

GPS

GSoC

iCloud

Icons

iOS

Legal

Linux Desktop

Linux packaging

Linux Phone

Mac OS

Map Data

Metro

Navigation

Need Feedback

Night Mode

NLnet 2024-06-281

No Feature Parity

Opening Hours

Outdoors

POI Info

Privacy

Public Transport

Raw Idea

Refactoring

Regional

Regression

Releases

RoboTest

Route Planning

Routing

Ruler

Search

Security

Styles

Tests

Track Recording

Translations

TTS

UI

UX

Walk Navigation

Watches

Web

Wikipedia

Windows

Won't fix

World Map

Milestone

Clear milestone

No items

No milestone

2025.03 Release

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

6 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps#10151

No description provided.