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

[Draft] [gpx] Add gpx import #5166

Merged
merged 19 commits into from Jun 4, 2023
Merged

[Draft] [gpx] Add gpx import #5166

merged 19 commits into from Jun 4, 2023

Conversation

cyber-toad
Copy link
Contributor

@cyber-toad cyber-toad commented May 15, 2023

Hi,
I try to finish the work started by @dvdmrtnz in #4067.
I made some tests and updated Manifest file but still was not able to have .gpx file associated with OrganicMaps. I asked on stackoverflow and searched on web, but was not able to find an answer.
@rtsisyk @arnaudvergnet - as Android experts could you give me any advice on it?
@biodranik - could you please advice, what minimal set of items is required to be implemented for merge? I checked your comment here #4067 so I think it should be enough to fix codestyle issues that you mentioned and add more tests (what do we want to check - large files, some details of GPX spec, something else)?

Closes #4067
Fixes #2953
Fixes #2681
Fixes #624

@cyber-toad cyber-toad changed the title [gpx] Add gpx import [Draft] [gpx] Add gpx import May 15, 2023
@Jean-BaptisteC
Copy link
Member

Jean-BaptisteC commented May 15, 2023

For Manifest check this https://stackoverflow.com/questions/3400072/pathpattern-to-match-file-extension-does-not-work-if-a-period-exists-elsewhere-i
I have try debug version, import gpx works, track is correctly show on map 🎉

Tested with track
Tested with 42500 POI in GPX

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.

Thanks for your efforts, it's a good start! Let's focus on properly testing and supporting different GPX files/versions and 1.1 spec, and clean the code a bit.

android/jni/app/organicmaps/Framework.cpp Show resolved Hide resolved
kml/kml_tests/gpx_tests.cpp Outdated Show resolved Hide resolved
kml/kml_tests/gpx_tests.cpp Outdated Show resolved Hide resolved
kml/kml_tests/gpx_tests.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.hpp Outdated Show resolved Hide resolved
kml/serdes_gpx.hpp Outdated Show resolved Hide resolved
map/bookmark_helpers.cpp Show resolved Hide resolved
map/bookmark_helpers.hpp Outdated Show resolved Hide resolved
map/bookmark_manager.cpp Show resolved Hide resolved
@biodranik
Copy link
Member

This file was not imported on iOS

https://www.walkhighlands.co.uk/download.php?w=197

@biodranik
Copy link
Member

This repo has many samples. Please check if there's something not supported yet: https://github.com/gps-touring/sample-gpx

@cyber-toad
Copy link
Contributor Author

Need to have a look can we implement color support mapped to our current KML

@cyber-toad
Copy link
Contributor Author

cyber-toad commented May 18, 2023

This file was not imported on iOS

https://www.walkhighlands.co.uk/download.php?w=197

Checked this one, it is not a recorded track, but a planned route rteType that is not supported for now. I'll check how to support it, added here: https://github.com/cyber-toad/organicmaps/wiki/GPX#todo-for-initial-release

@biodranik
Copy link
Member

Checked this one, it is not a recorded track, but a planned route rteType that is not supported for now. I'll check how to support it, added here: https://github.com/cyber-toad/organicmaps/wiki/GPX#todo-for-initial-release

  1. It would be great to support it, is it complex?
  2. Please link your issue with our GPX issue. It would be better for other contributors and for us to monitor and fix it, than using your one.

@cyber-toad
Copy link
Contributor Author

cyber-toad commented May 18, 2023

1. It would be great to support it, is it complex?

I added support and test in today's commit and I was able to load "Falls of Glomach" route that you mentioned

2. Please link your issue with our GPX issue. It would be better for other contributors and for us to monitor and fix it, than using your one.

I decided just to move this wiki page to comment in #624 issue so it is all together there

@biodranik
Copy link
Member

If you see that any other related code can be improved by using string_view instead of string, don't stop and fix it :)

kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Show resolved Hide resolved
@cyber-toad
Copy link
Contributor Author

For Manifest check this https://stackoverflow.com/questions/3400072/pathpattern-to-match-file-extension-does-not-work-if-a-period-exists-elsewhere-i I have try debug version, import gpx works, track is correctly show on map tada

Tested with track Tested with 42500 POI in GPX

I tried couple of suggestions for manifest, but on my Android device I still need to select application for file. Did you test with Android? Was OrganicMaps mapped to .gpx extension in your case?

@Jean-BaptisteC
Copy link
Member

I store gpx file in root directory, in download directory, i can't open gpx file

@biodranik
Copy link
Member

I store gpx file in root directory, in download directory, i can't open gpx file

Are KML files opened correctly from the same directory?

@Jean-BaptisteC
Copy link
Member

Yes i can open kml file from download directory or root directory

@biodranik
Copy link
Member

Please also check that files with non-ascii symbols and spaces in their names are correctly opened on iOS and Android.

@cyber-toad
Copy link
Contributor Author

cyber-toad commented May 23, 2023

Please also check that files with non-ascii symbols and spaces in their names are correctly opened on iOS and Android.

I checked non-ascii and spaces for Android and it is fine, need someone with Apple to help with check for iOS.

@cyber-toad
Copy link
Contributor Author

Just to summarize remaining items because I was not able to have any progress on them

  1. Add file association for Android: I asked on SO, tried the same config as OM has for KML, also checked OSMAnd implementation - but still no luck. I don't know could it be a reason that I deploy OM version from Android Studio - not using GooglePlay
  2. Check non-asci files for iOS - can't do it myself - no iOS device available
  3. Rework file extensions to string_view [Draft] [gpx] Add gpx import #5166 (comment) - I would prefer to look on it separately to avoid increase scope of initial PR

Looks like all other boxes are completed.

@biodranik
Copy link
Member

(1) is a blocker and should be fixed. However, it looks working on my devices, how did you test it? From which apps did you try to open gpx? On which Android version? Did it fail to suggest Organic Maps open that file type? Does it fail if the app wasn't launched and work if the app was launched at least once?

Please also add GPX mime type scan to getBookmarksFilenameFromUri.

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.

Thank you very much! We're almost there :)

android/AndroidManifest.xml Show resolved Hide resolved
android/jni/app/organicmaps/Framework.cpp Show resolved Hide resolved
data/gpx_test_data/color.gpx Show resolved Hide resolved
iphone/Maps/OMaps.plist Show resolved Hide resolved
kml/kml_tests/CMakeLists.txt Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
kml/serdes_gpx.cpp Outdated Show resolved Hide resolved
@cyber-toad
Copy link
Contributor Author

cyber-toad commented May 28, 2023

(1) is a blocker and should be fixed. However, it looks working on my devices, how did you test it? From which apps did you try to open gpx? On which Android version? Did it fail to suggest Organic Maps open that file type? Does it fail if the app wasn't launched and work if the app was launched at least once?

Please also add GPX mime type scan to getBookmarksFilenameFromUri.

Ok, looks like there is some issue with default Xiaomi Mi file browser. When I try to open a gpx file it always ask me some strange question about file type,
image
but after I save OM for this file type it starts to open file with OM after I select "Docs" in this dialog. Don't know why Xiaomi asks for file type each time.

I tried in another file browser (Cx File Explorer) and it was fine. It displays list of applications and OM is there. But would be good if we can test on several devices - I have only Xiaomi available, so can only try different file browsers.

image

getBookmarksFilenameFromUri is fixed, but how I can test it - is it for import of file without extension?

@biodranik
Copy link
Member

getBookmarksFilenameFromUri is fixed, but how I can test it - is it for the import of files without extension?

This code was written by @rtsisyk , maybe he can give details about testing? I assume it's for cases when the full file name with extension can't be retrieved for some reason, but there is a correct mime type for the file. @rtsisyk do you remember when it is possible and how to test it?

In the worst case, it is a no-op, if a file extension is always available from the system.

Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
Signed-off-by: cyber-toad <the.cyber.toad@proton.me>
@biodranik
Copy link
Member

Ready to merge and release? Are there any issues left to be fixed?

@cyber-toad
Copy link
Contributor Author

cyber-toad commented Jun 4, 2023

Ready to merge and release? Are there any issues left to be fixed?

I tested on my Android with cold start and it worked. There are 3 items that I have difficulties with resolve, for 2 of them I added my comments, for one of them about missing m_org I can't find an answer for now. Additionally 2 comments are displayed as "Outdated", so I can't view them here: https://github.com/organicmaps/organicmaps/pull/5166/files but I think I haven't done anything for them. My other action items are resolved.

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.

Please add an exception test in a follow-up pull request. We'll prepare these changes for the release.

@biodranik
Copy link
Member

biodranik commented Jun 4, 2023

And thank you very much for your time and efforts to finish this important task! Do you want us to mention your GitHub nickname (or real name) in our news? @cyber-toad @dvdmrtnz

@biodranik biodranik merged commit dce7c21 into organicmaps:master Jun 4, 2023
13 checks passed
@biodranik
Copy link
Member

Let's create an issue for GPX export. It would be great to finish it too.

@Jean-BaptisteC
Copy link
Member

Done :)

@cyber-toad
Copy link
Contributor Author

And thank you very much for your time and efforts to finish this important task! Do you want us to mention your GitHub nickname (or real name) in our news? @cyber-toad @dvdmrtnz

@biodranik Thanks a lot for your patience and help with this PR, I haven't use C++ for many-many years, so it was not easy for me to work with modern standard. If you want to put any details in news, feel free to use my GitHub handle.I'll try to work on remaining items once I have some free time.

@dvdmrtnz
Copy link
Contributor

dvdmrtnz commented Jun 5, 2023

And thank you very much for your time and efforts to finish this important task! Do you want us to mention your GitHub nickname (or real name) in our news? @cyber-toad @dvdmrtnz

I think @cyber-toad deserves all the credit here! But if you want to mention me too there's no problem!

@cyber-toad Great job!

@randomsnowflake
Copy link

Great to hear this feature has been implemented! When will he new app hit the iOS App store? I would love to try it out.

@biodranik
Copy link
Member

biodranik commented Jun 25, 2023

@cyber-toad how is time data from gpx stored in kml now after the import?

@cyber-toad
Copy link
Contributor Author

@cyber-toad how does time data from gpx is stored in kml now after the import?

We don't parse datetime data (if you are talking about <time> tag) now. Do we have a place in our internal model for it?

@biodranik
Copy link
Member

@cyber-toad It will be needed to implement track recording, ideally together with any other custom data. Also, it is useful to display the imported or recorded track statistics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants