-
-
Notifications
You must be signed in to change notification settings - Fork 858
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
UI to export bookmarks as GPX #8137
Conversation
FIY, app crash when you press button to export bookmarks :) |
Do you mean "Export all" button? I tried different exports and was not able to reproduce, is there a log available? Also do you build from latest source, there was an issue with accidental file removal, but I fixed it couple of days ago. |
@Jean-BaptisteC could you please help me to reproduce the crash that you mentioned, what are the steps? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirylkaveryn can you please review and fix failing iOS code? And maybe add iOS buttons to export? :)
Not able to reproduce crash |
@cyber-toad I've updated the ios part. |
@biodranik please have a look at this PR. I included ios changes, added translations (without regeneration of string files for now). Let me know if something else except translation verification & string regeneration is required. It is not clear what should be done with #8137 (comment), other items are implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks. A menu item on ios is not translated, does the key matches the android one?
@rtsisyk can you please help and release an alpha today with all PRs marked in the milestone including this one?
@cyber-toad unrelated question: what's missing in the core to export/import timestamps for each point of a track for both kml and gpx?
Any other data that should be saved for recorded tracks?
map/map_tests/bookmarks_test.cpp
Outdated
auto checker = [](BookmarkManager::SharingResult const & result) | ||
{ | ||
auto fileName = result.m_sharingPath; | ||
TEST(fileName.find("Some random route.gpx") != std::string::npos, ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TEST_EQUAL 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get what should be compared with 0? fileName is "/tmp/Some random route.gpx" in this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's confusing, then it should be renamed to filePath.
I've made the screen records before the string was translated and regenerated. I will make an additional recordings soon and will add here. |
@kirylkaveryn another issue I saw in iOS simulator when saving a GPX to Files, do we need to somehow specify/add a definition for the saved/exported .gpx file extension/mime type?
|
Hmm... I can't catch such warnings on either the device and the simulator. And we haven't any extensions. And we already have all related type... |
kml::MultiGeometry that is used in TrackData uses PointWithAltitude that is (x, y, h) without timestamp. Once there is a timestamp there we can update parsers to populate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @cyber-toad can you please squash all commits, and make a separate strings regeneration commit, that will also include iOS strings?
map/map_tests/bookmarks_test.cpp
Outdated
auto checker = [](BookmarkManager::SharingResult const & result) | ||
{ | ||
auto fileName = result.m_sharingPath; | ||
TEST(fileName.find("Some random route.gpx") != std::string::npos, ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's confusing, then it should be renamed to filePath.
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A few nits before merge.
@@ -662,7 +662,8 @@ | |||
<string name="bookmarks_empty_list_title">This list is empty</string> | |||
<string name="bookmarks_empty_list_message">To add a bookmark, tap a place on the map and then tap the star icon</string> | |||
<string name="category_desc_more">…more</string> | |||
<string name="export_file">Export file</string> | |||
<string name="export_file">Export KML/KMZ file</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the regenerated commit to avoid conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like strings.xml is still modified here. Can it be moved to another commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the changes were not pushed yet...
@@ -662,7 +662,8 @@ | |||
<string name="bookmarks_empty_list_title">This list is empty</string> | |||
<string name="bookmarks_empty_list_message">To add a bookmark, tap a place on the map and then tap the star icon</string> | |||
<string name="category_desc_more">…more</string> | |||
<string name="export_file">Export file</string> | |||
<string name="export_file">Export KML/KMZ file</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like strings.xml is still modified here. Can it be moved to another commit?
yep, I replied during applying changes should be pushed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, and a few (hopefully final!) nits.
android/app/src/main/java/app/organicmaps/bookmarks/BookmarkCategoriesFragment.java
Outdated
Show resolved
Hide resolved
Signed-off-by: cyber-toad <[email protected]>
Signed-off-by: cyber-toad <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience and for finishing a new feature!
I was in the middle of testing this feature... Have you got at least one review from anybody who works on Android? |
The author tested it on Android, JB has tested it on Android, and I have tested it on Android. Any discovered issues can be fixed. |
|
Once code change are completed I'll create translations & generate strings.
Also iOS changes will be required.
@biodranik WDYT about the implementation?
Closes #5271