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

[routing] Manually placed route origin is no longer replaced by current location after app restart #7786

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

fgbg03
Copy link
Contributor

@fgbg03 fgbg03 commented Apr 2, 2024

The application replaced the origin point solely based on if it had found the user's location. Added a parameter to the RouteMarkData struct which is saved alongside the start point. This is used on start up to determine if the origin point of the route should be kept or replaced with the user's location (if one is found).

…ation when re-opening the app

The application replaced the origin point solely based on if it had found the user's location. Added a parameter to the RouteMarkData struct which is saved alongside the start point. This is used on start up to determine if the origin point of the route should be kept or replaced with the user's location (if one is found).

Signed-off-by: Fábio Gomes <gabriel.gomes@tecnico.ulisboa.pt>
@biodranik
Copy link
Member

Thanks! Is there any issue about it? What are the steps to reproduce the issue and confirm that it's fixed? Does it reproduce on iOS or Android?

@fgbg03
Copy link
Contributor Author

fgbg03 commented Apr 2, 2024

Thanks! Is there any issue about it? What are the steps to reproduce the issue and confirm that it's fixed? Does it reproduce on iOS or Android?

About issue #2833

The only trouble I had with this solution is just when I'm inside I sometimes don't receive a location update in time for it to register and start following the location. But that also happens in the unpatched version and I guess it's my problem for not being outside.

[For Android]
To verify the occurrence of the bug you just need (with GPS turned on) to create a route from a manually placed mark to any other place. After the route is created, close and re-open the app.
Before: The route is reloaded but the starting point is replaced by the GPS location
After: The route is kept as it was.

You can repeat the process with a route from your GPS location (by selecting only the destination point). In both cases this should result in the route being re-established with your GPS location as the origin.

[For iOS]
I cannot provide info on this, as I do not own an iPhone.
I should also add my previous comment about the bug being present on iOS was edited with a note. I may ask my friend who has an iPhone to provide more information/ test further on his device if needed.

@biodranik
Copy link
Member

Thanks for the details! Looks like route planning mode is not restored on iOS after killing the app. And it is likely for a reason, because iOS usually doesn't kill apps in the background )

@fgbg03
Copy link
Contributor Author

fgbg03 commented Apr 2, 2024

Just to be clear. By closing I meant fully closing. I'm not too familiar with iPhone, but on Android you must hit the apps on background button and swipe to close the app. I guess iOS has something similar.
If I had made this clear enough in the previous comment, then I guess I have nothing to add.
Is there anything more I could do in regards to this issue?

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -136,6 +138,8 @@ RouteMarkData DeserializeRoutePoint(json_t * node)
FromJSONObject(node, "x", data.m_position.x);
FromJSONObject(node, "y", data.m_position.y);

FromJSONObject(node, "replaceWithMyPosition", data.m_replaceWithMyPosition);
Copy link
Member

Choose a reason for hiding this comment

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

Where is this json stored? Is there a possibility that old version stored old json, and newly updated version failed to load old json? Won't it crash?

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be just a FromJSONObjectOptional, I believe. It will write the value to there if it exists, otherwise leave it (in this case, in the false default).

@@ -22,6 +22,7 @@ struct RouteMarkData
bool m_isVisible = true;
bool m_isMyPosition = false;
bool m_isPassed = false;
bool m_replaceWithMyPosition = false;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

Suggested change
bool m_replaceWithMyPosition = false;
bool m_replaceWithMyPositionAfterRestart = false;

?

Copy link
Member

Choose a reason for hiding this comment

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

That wasn't updated? Was there some github error?

@rtsisyk @Tmpod

@Jean-BaptisteC Jean-BaptisteC changed the title [routing] Fix #2833: Manually placed route origin is no longer replaced by current location after app restart [routing] Manually placed route origin is no longer replaced by current location after app restart Apr 4, 2024
Copy link
Member

@Jean-BaptisteC Jean-BaptisteC left a comment

Choose a reason for hiding this comment

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

✅ Pixel 6 - Android 14

@rtsisyk
Copy link
Contributor

rtsisyk commented Apr 24, 2024

What is blocking this PR from merging?

@rtsisyk rtsisyk merged commit 3cdb79f into organicmaps:master Apr 24, 2024
15 checks passed
@Tmpod
Copy link
Contributor

Tmpod commented Apr 24, 2024

I just noticed I had left an e-mail reply to this PR, but it was meant to be sent to #7964... rtsisyk sent identical questions to both PRs and I think I selected the wrong e-mail when replying 🤦.

This PR wasn't in fact ready to be merged, afaik. There were unresolved discussions OP didn't resolve. @fgbg03

@fgbg03
Copy link
Contributor Author

fgbg03 commented Apr 24, 2024

This PR wasn't in fact ready to be merged, afaik. There were unresolved discussions OP didn't resolve. @fgbg03

Yes. Looks like I hadn't commited the suggested change. Now that this was merged I guess it's past, but my bad.

@biodranik
Copy link
Member

@fgbg03 it's better later than never )

@fgbg03
Copy link
Contributor Author

fgbg03 commented Apr 25, 2024

I can update that, but since this is closed how do I go about changing it?

@Jean-BaptisteC
Copy link
Member

Create a new PR :)

@fgbg03
Copy link
Contributor Author

fgbg03 commented Apr 25, 2024

Figured. On it.

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.

[Android] Routing mode resets origin point to current location when re-opening app
5 participants