Load Track from file and add as Route #312

printpagestopdf · 2020-06-14T13:57:37Z

This pull request is related to #254 and #274. It adds a dropdown item and modal dialog to import a local trackfile and using parts of the linestring points to add it as route.
I haven't done yarn push-transifex, because in my understanding should be done after merge (otherwise please tell me).
Feel free to change or request changes from me.

rkflx · 2020-06-16T09:11:10Z

Nice, great feature!

Make sure to run yarn prettier to automatically reformat the code and yarn test or yarn build to check whether everything builds in release mode (currently it stumbles on let, which does not seem supported here, among other things).

UI-wise, would it be possible to combine this with the regular track loading? Having two entries could be slightly confusing for users not knowing about the difference between routes and tracks. Also, there is currently no way to convert a track after is has already been added.

Here are some ideas:

Only provide a single menu entry: "Load tracks", which opens a file chooser in multi-file mode as before.
If only a single file was selected, open a dialog containing:
- A big button which directly adds the file as a background layer after clicking on it.
- An explanation for what the button does (so the user is aware the track cannot be modified).
- Another big button to add the file as an editable route. A "More" button or a separate dialog could be used for the "Tuning", "POI" and (reworded) "Copy track to background layer" options.
- An explanation for what the button does (so the user knows the current route will be extended).
- Possibly a "Remember my choice for the future" checkbox.
If multiple files were selected: Assume the user wants to add all of them as tracks, no need to ask.
In the "Layers" sidebar tab, add a clickable icon after each track to open the conversion dialog.
Get rid of the "Format" chooser (it should always work automatically, instead of having the user to do the work), replace with a help button listing the formats available and an error message if things went wrong.

printpagestopdf · 2020-06-16T11:42:08Z

Ups thank you for the yarn checking commands, wasn't aware that let is not allowed (all specifications I read prefer it). I like your suggestions, will try to implement it.
One question: haven't make changes on a existing pull request before. What is the best method to do? Is it enough to make the changes in my fork or do I have to make a new pull request ...?

nrenner · 2020-06-16T14:45:56Z

Prettier and Eslint should automatically be executed as pre-commit hook, don't know why that apparently didn't work:

brouter-web/package.json

Lines 17 to 20 in 8621c44

    
           "husky": { 
        
               "hooks": { 
        
                   "pre-commit": "pretty-quick --bail && yarn lint" 
        
               }

Ups thank you for the yarn checking commands, wasn't aware that let is not allowed (all specifications I read prefer it).

let and other ES6 features in your code are failing because we don't support ES6 yet (#249).

But no need to remove those - I intend to introduce Babel, just want to get a bugfix release out first, so probably will postpone merging after that.

nrenner · 2020-06-16T15:43:23Z

UI-wise, would it be possible to combine this with the regular track loading? Having two entries could be slightly confusing for users not knowing about the difference between routes and tracks.

I had thought about having a common dialog, but wasn't sure. For everyday use two separate menu entries might be more efficient and less bloated. On the other hand, it might be useful for common settings like choosing the color for the the track(s) to load.

I personally don't really mind either way for this PR, as we already got the users confused that the loaded tracks aren't editable.

Also, there is currently no way to convert a track after is has already been added.

I don't want to raise the bar for merging this PR. This should be an enhancement for later in a separate issue, as we currently have no general concept for interacting with tracks (changing color, removing) - neither on the map nor in the layer switcher, also see #297 (comment).

nrenner

Thanks for your PR! Some minor comments inline.

nrenner · 2020-06-16T16:04:15Z

js/plugin/RouteLoaderConverter.js

+
+			if(isBusy === true)
+				$("#loadedittrackdlg #msg_busy").removeClass("invisible");
+				// $("#loadedittrackdlg #busy_img").addClass("d-block");


Do you want to keep commented code (two more)?

nrenner · 2020-06-16T16:06:51Z

js/plugin/RouteLoaderConverter.js

+				turf.featureEach(this._currentGeoJSON,(feature,idx) => {
+				   if(turf.getType(feature) == "Point")
+				   {
+					   console.log("POI",feature);


Please remove the console.log

nrenner · 2020-06-16T16:12:59Z

js/plugin/RouteLoaderConverter.js

+            if (toGeoJSON === undefined) {
+                if (typeof window !== 'undefined') {
+                    toGeoJSON = require('togeojson');
+                } else {
+                    toGeoJSON = require('togeojson')(root);
+                }
+            }


We don't need this? Could you move this to external code when using in other contexts?

nrenner · 2020-06-16T16:24:57Z

One question: haven't make changes on a existing pull request before. What is the best method to do? Is it enough to make the changes in my fork or do I have to make a new pull request ...?

You shold be able just to push more commits into your branch, see the GitHub hint at end of comments section below:

Add more commits by pushing to the loadtrackasroute branch on printpagestopdf/brouter-web.

printpagestopdf · 2020-06-17T10:16:06Z

UI-wise, would it be possible to combine this with the regular track loading? Having two entries could be slightly confusing for users not knowing about the difference between routes and tracks.

I had thought about having a common dialog, but wasn't sure. For everyday use two separate menu entries might be more efficient and less bloated. On the other hand, it might be useful for common settings like choosing the color for the the track(s) to load.

I personally don't really mind either way for this PR, as we already got the users confused that the loaded tracks aren't editable.

Also, there is currently no way to convert a track after is has already been added.

I don't want to raise the bar for merging this PR. This should be an enhancement for later in a separate issue, as we currently have no general concept for interacting with tracks (changing color, removing) - neither on the map nor in the layer switcher, also see #297 (comment).

I am with you, that the best approach is, to move these UI suggestions in new feature requests. So I am not touching the UI for now

printpagestopdf · 2020-06-17T10:20:10Z

Sorry my fault I believed to close only this discussion thread so I have ot reopen it.

Beneath the code changes you mentioned (I was too lazy to recheck it) I will although try to remove the ES6 features (understanding that Babel would sove this, but I will try to follow the current coding rules).

printpagestopdf · 2020-06-17T18:51:27Z

Hopefully I found everything that has to be fixed. Removed the items mentioned by @nrenner including ES6 features. Following @rkflx suggestions removed the format selection select and ran yarn prettier.

nrenner · 2020-06-19T16:20:21Z

Thanks for your contribution!

nrenner · 2020-06-19T16:25:00Z

I haven't done yarn push-transifex, because in my understanding should be done after merge

Only by the maintainers, so this is done by me.

printpagestopdf · 2020-06-19T17:50:22Z

Many thanks for maintaining this nice project.

- common track style - TracksLoader now also adds points as POIs - RouteLoaderConverter does not add route points as POI

Loading a track as route with lots of waypoints (simplify tolerance of 0) caused a long pause before even handling the first route request. This seems to be caused by a repaint for every added marker/layer. Adding them all at once in FeatureGroup.addTo(map) helps (although still added in a loop).

to prevent code in GPX getting executed like this: <name><img src="xyz" onerror="alert('script executed')"></name>

Load Track from file and add as Route

5b73d7e

nrenner reviewed Jun 16, 2020

View reviewed changes

printpagestopdf closed this Jun 17, 2020

printpagestopdf reopened this Jun 17, 2020

Format selection removed

a357d18

nrenner merged commit 0d6cd61 into nrenner:master Jun 19, 2020

nrenner added this to the 0.13.0 milestone Jun 19, 2020

printpagestopdf deleted the loadtrackasroute branch June 19, 2020 17:48

nrenner added a commit that referenced this pull request Jun 25, 2020

Refactor common track loading parts (#312)

5ad12a7

- common track style - TracksLoader now also adds points as POIs - RouteLoaderConverter does not add route points as POI

nrenner mentioned this pull request Jun 26, 2020

Error "null description for: n_..._..." abrensch/brouter#254

Closed

nrenner added a commit that referenced this pull request Jul 14, 2020

Sanitize track name (#312)

9500481

to prevent code in GPX getting executed like this: <name><img src="xyz" onerror="alert('script executed')"></name>

pyrog mentioned this pull request Jul 20, 2022

Use an OSM route as a route #588

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load Track from file and add as Route #312

Load Track from file and add as Route #312

printpagestopdf commented Jun 14, 2020

rkflx commented Jun 16, 2020 •

edited

Loading

printpagestopdf commented Jun 16, 2020

nrenner commented Jun 16, 2020

nrenner commented Jun 16, 2020

nrenner left a comment

nrenner Jun 16, 2020

nrenner Jun 16, 2020

nrenner Jun 16, 2020

nrenner commented Jun 16, 2020

printpagestopdf commented Jun 17, 2020

printpagestopdf commented Jun 17, 2020

printpagestopdf commented Jun 17, 2020

nrenner commented Jun 19, 2020

nrenner commented Jun 19, 2020

printpagestopdf commented Jun 19, 2020

Load Track from file and add as Route #312

Load Track from file and add as Route #312

Conversation

printpagestopdf commented Jun 14, 2020

rkflx commented Jun 16, 2020 • edited Loading

printpagestopdf commented Jun 16, 2020

nrenner commented Jun 16, 2020

nrenner commented Jun 16, 2020

nrenner left a comment

Choose a reason for hiding this comment

nrenner Jun 16, 2020

Choose a reason for hiding this comment

nrenner Jun 16, 2020

Choose a reason for hiding this comment

nrenner Jun 16, 2020

Choose a reason for hiding this comment

nrenner commented Jun 16, 2020

printpagestopdf commented Jun 17, 2020

printpagestopdf commented Jun 17, 2020

printpagestopdf commented Jun 17, 2020

nrenner commented Jun 19, 2020

nrenner commented Jun 19, 2020

printpagestopdf commented Jun 19, 2020

rkflx commented Jun 16, 2020 •

edited

Loading