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

export user's POI (point of interest) #226

Merged
merged 4 commits into from
Oct 10, 2019

Conversation

bagage
Copy link
Collaborator

@bagage bagage commented Aug 6, 2019

Fixes #222

Still left to do:

  • extract translations
  • input validation: how to ensure that POI name is valid? For gpx there is no limitation (except XML itself), not sure about the others.
  • allow user to chose icon type? cf Leaflet.awesome-markers for now, all POI are displayed with ⭐ icon. We can extend it quite easily if needed
  • persist markers in URL (from / to)
  • refresh url on marker modification (move/delete/add)
  • write markers in generated file, either by passing them to brouter , or by editing the file after it has been generated
  • custom marker icon when drawing POIs?
  • escape should cancel drawing mode as route?
  • drawing route and POI edition should be exclusive
  • shortcut to start drawing POI
  • allow to remove all POI in delete button menu
  • allow to remove POI from popup

Questions:

  • does it make sense to add the POIs layer in layers control so that it can be shown/hidden? Or should it be added to the map directly

@bagage
Copy link
Collaborator Author

bagage commented Aug 6, 2019

@nrenner what about dropping all file formats and only use one (say, gpx), then external libraries to convert from GPX to anything else?

It would ease developing new features related to file - yet I'm not sure how to tackle it otherwise.

edit: togeojson supports GPX and KML to geojson conversion. Would only need a GPX to kml conversion to be fine? Or maybe verrazzano that supports GPX to both?

@bagage bagage force-pushed the 222-add-users-poi branch from c03e6ed to a992103 Compare August 6, 2019 12:33
@Phyks
Copy link
Contributor

Phyks commented Aug 6, 2019

My two cents on this (did not yet have time to test the code locally), while you are changing the POIs and the markers, what about adding a distinction between markers (start / end and waypoints)?

I had missed #129, sorry :/

@bagage
Copy link
Collaborator Author

bagage commented Aug 6, 2019

Would be great @Phyks, but it is actually another issue yet-non-trivial: #129.

@nrenner
Copy link
Owner

nrenner commented Aug 7, 2019

what about dropping all file formats and only use one (say, gpx), then external libraries to convert from GPX to anything else?

What do you mean exactly with "dropping all file formats" and converting? Ignore the other formatAs* methods in BRouter OsmTrack and convert in the client? But still pass all values (user POIs here) to the server to get the primary format (GPX?)? With the download attribute, we currently don't get hold of the server result, that would need to be changed.

@bagage
Copy link
Collaborator Author

bagage commented Aug 7, 2019

What do you mean exactly with "dropping all file formats" and converting? Ignore the other formatAs* methods in BRouter OsmTrack and convert in the client?

Yes.

But still pass all values (user POIs here) to the server to get the primary format (GPX?)?

No, in this case we would insert them directly in the client, after we got the gpx generated. The alternative is to modify backend to accept a list of POIs and write them for each file format.

With the download attribute, we currently don't get hold of the server result, that would need to be changed.

In that case, yes.

Not sure which solution is better yet.

@nrenner
Copy link
Owner

nrenner commented Aug 9, 2019

Not sure either. After the discussion in abrensch/brouter#91 and with trackName already implemented it seemed more straightforward and reasonable to do those in the server, so I sort of gave up the idea of client-side export formatting.

And with route waypoint export already implemented in the server, I'm not sure it's worth starting a different way just for user POI waypoints.

Future features regarding export that come to mind:

  • GPX route download + upload #16 (GPX route download + upload): with route waypoints now implemented in the server it probably makes sense to do the same with rte, and might be useful for other clients too
  • Track Timestamp? #196 (Track Timestamp?): time deltas currently not available, would need to be added in the server anyway
  • Allow straight lines #68 (Allow straight lines): converting from server GPX won't help here; to do this client-side we would probably need to completely replace the current export that sends all route points to the server, aggregate the route segments we already have ourselves and probably convert from GeoJSON

@abrensch would you mind user POI waypoints getting passed to OsmTrack just for file formatting or would you prefer those handled completely in the client?

@bagage bagage force-pushed the 222-add-users-poi branch from a992103 to c395d7f Compare August 10, 2019 17:37
@Phyks
Copy link
Contributor

Phyks commented Aug 17, 2019

No, in this case we would insert them directly in the client, after we got the gpx generated.

Another advantage of this (thinking of #231 (comment)) is that BRouter-web could add a comment in the GPX file with the URL used to generate the track. Not sure this is worth the pain though.

@bagage
Copy link
Collaborator Author

bagage commented Aug 17, 2019

For now, and based on @nrenner advice, I'll probably go for the first solution (implement POI export in backend) - URL could be another query param option (?description=Generated with URL <url> or so)

@bagage bagage changed the title add users poi WIP: add users poi Aug 17, 2019
@bagage bagage changed the title WIP: add users poi WIP: export user's POI (point of interest) Aug 17, 2019
@nrenner
Copy link
Owner

nrenner commented Aug 17, 2019

Not sure, but I think the server request URL is pretty much the same as in the client, so a simple regex replacement would probably do - or at least the server gets all information to reconstruct the important part of the client URL (except for the map parameter).

There is an issue for this: abrensch/brouter#152.

@bagage bagage force-pushed the 222-add-users-poi branch from 63ac490 to 66de1eb Compare August 19, 2019 17:43
@bagage bagage changed the title WIP: export user's POI (point of interest) export user's POI (point of interest) Sep 3, 2019
@bagage bagage changed the title export user's POI (point of interest) WIP: export user's POI (point of interest) Sep 3, 2019
@bagage bagage force-pushed the 222-add-users-poi branch 6 times, most recently from 2707f8f to 19fabc2 Compare October 1, 2019 15:59
@bagage bagage changed the title WIP: export user's POI (point of interest) export user's POI (point of interest) Oct 1, 2019
@bagage bagage force-pushed the 222-add-users-poi branch 2 times, most recently from 8e46ed2 to a4da9f1 Compare October 1, 2019 16:05
@bagage
Copy link
Collaborator Author

bagage commented Oct 1, 2019

OK I'm done with it. brouter counterpart is abrensch/brouter#196 (also this might addresses #194, since I do XML/JSON escaping properly there).

Comments are welcome :).

@bagage bagage force-pushed the 222-add-users-poi branch from a4da9f1 to 63acdb0 Compare October 1, 2019 17:16
@bagage
Copy link
Collaborator Author

bagage commented Oct 1, 2019

Basically looking like:

image

Copy link
Owner

@nrenner nrenner left a comment

Choose a reason for hiding this comment

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

Looking good overall, see inline comments.

js/plugin/POIMarkers.js Outdated Show resolved Hide resolved
js/router/BRouter.js Outdated Show resolved Hide resolved
js/router/BRouter.js Outdated Show resolved Hide resolved
@bagage bagage force-pushed the 222-add-users-poi branch from 63acdb0 to 882c4da Compare October 9, 2019 11:37
@bagage bagage force-pushed the 222-add-users-poi branch from 882c4da to fa5af58 Compare October 10, 2019 19:07
@nrenner nrenner merged commit ddf8e27 into nrenner:master Oct 10, 2019
@nrenner
Copy link
Owner

nrenner commented Oct 10, 2019

Thanks!

@nrenner nrenner mentioned this pull request Nov 21, 2019
5 tasks
nrenner added a commit that referenced this pull request Dec 11, 2019
@nrenner
Copy link
Owner

nrenner commented Dec 11, 2019

I moved the POI button below the routing toolbar, because I found it too prominent for a secondary feature and better in line this way with the probable order of user actions. Let me know if you disagree, I'm open for discussion.

@bagage
Copy link
Collaborator Author

bagage commented Dec 11, 2019

No problem @nrenner, I think it makes sense!

@nrenner nrenner modified the milestone: 0.11.0 Jan 13, 2020
@bagage bagage deleted the 222-add-users-poi branch February 8, 2021 14:59
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.

Add POIs in the exported route
3 participants