[WIP] [RFC] Include profile options in hash

[tbsmark86]

Hi!

This is an Idea to address the various issues around this topic. For instance #26 #666 maybe #472
The Idea is to extend the &profile=fastbike value to be itself a nested parameter list. I'am doing this by replacing it with an URI.

The current minimal prototype will create a url like this:
http://localhost:3000/#map=12/51.8751/12.3835/osm-mapnik-german_style,route-quality&lonlats=12.461743,51.882955;12.416908,51.840657&profile=bw%3Afastbike%3Fallow_ferries%3Dfalse
Which when when loaded again will still have the allow_ferries set to false.

I've planed four different formats:

• <profile-name> - just like it is currently when options are not required
• bw:<profile-name>?<arg>=<value> - for a standard profile with options
  It's an url with a custom scheme - 'bw:'
• bwl:local - for a manually edit profile
  Intention is to show a Message to anyone using the url that the profile is not available
• http://example.com/profile.brf - for an external profile
  This is for a future extension: Allowing the user to add custom profiles like they can add custom layers

Is there any interest in this approach?

Code wise I've started with creating a new class (ProfileData) to have more centralized state for all this profile related data.
My Intention is to move more of the current processing to this class so the UI-Elements can focus more on the UI part. Hopefully this would allow me to add some unit-tests for this stuff.
Of course this will require a lot of refactoring and likely new bugs. Also it will take quite some time.

[nrenner]

Thanks for the suggestion!

As I understand, this would be a pure client-side solution that still relies on modifying and uploading a custom profile?

However, the server now provides a way (according to #666) to pass profile parameters that override the corresponding variable of an unmodified standard profile, so the goal should rather be to get rid of using a custom profile for profile options and pass those as URL parameters to the server instead.

The server forwards URL parameters with a profile: prefix to be set in the RoutingContext. Although not using the latest master state, this already seems to work:

• https://brouter.de/brouter?lonlats=12.461743,51.882955|12.416908,51.840657&alternativeidx=0&format=geojson&profile=trekking&profile:allow_ferries=0
  (view in geojson.io)

I would expect the client URL to use a corresponding format, e.g. &profile=trekking&profile:allow_ferries=0.

• http://example.com/profile.brf - for an external profile
  This is for a future extension: Allowing the user to add custom profiles like they can add custom layers

abrensch and I agreed not to automatically load and display content from arbitrary external URLs. Instead, the decision was to offer a way to make a profile permanent by uploading it to a separate storage location that doesn't get cleaned, e.g. using a "share" button, and then to include the shared profile name for that in the URL. A first step for that was already made (shared profile dir commit), but still missing the upload and client part.

So I see #26 and #666 as two different things. #472 would be just some manual File operations that load/save to/from the profile editor, maybe with an additional URL field like for loading no-go areas, but not modifying the URL. Uploading would still be an additional manual step, just like with pasting or editing.

[tbsmark86]

As I understand, this would be a pure client-side solution that still relies on modifying and uploading a custom profile?
Yes.

However, the server now provides a way (according to #666) to
Well that reads like this was not really intended by the Server?
I'am not aware of any api doc for the server?

abrensch and I agreed not to automatically load and display content from arbitrary external URLs.
Yes. My Idea was to first ask the user.
fgcvfg
Overall I read that as a 'no' to my question?

Or this profile:xyz is the 'official' way for the backend and just drop the profile-is-a-nested-url part?

[nrenner]

Well that reads like this was not really intended by the Server?

That's not my understanding, I always found that a useful feature and the way to go. While it started as a quick hack for a prototype, recent changes, that are part of a larger, still ongoing update, apparently turned this into something proper.

I'am not aware of any api doc for the server?

General reference from the readme:

The API endpoints exposed by this HTTP server are documented in the brouter-server/src/main/java/btools/server/request/ServerHandler.java file.

But as zod mentions in #666, the profile: handling is not documented for HTTP yet, but the Android interface has a extraParams property for that.

abrensch and I agreed not to automatically load and display content from arbitrary external URLs.

Yes. My Idea was to first ask the user.

It might not be obvious from the external URL in a shared link alone if it loads offensive or malicious content or not, until the users actually load and see it. The reasoning was, that by only sharing content from our server, we can control and potentially delete what gets shared.

Overall I read that as a 'no' to my question?

To the approach as proposed, yes, I'm afraid, not to the general idea of including profile options in the hash. Having a separate ProfileData class might still be useful though.

Or this profile:xyz is the 'official' way for the backend and just drop the profile-is-a-nested-url part?

I strongly think that passing profile:xyz parameters to the server instead of a custom profile is the way to go for profile options. By using it, we would make it official. If there is more work required on the server it would be part of the job or a feature request for the server. But from my understanding of the discussion in #666 this is ready to use.

Ideally, the URL request to the server would be reflected in the same way in the client URL hash, instead of profile-is-a-nested-url.

So if you're still interested, my suggestion would be to limit the PR to profile options, using profile:xyz server parameters instead of a custom profile as suggested in #666 and to exclude profile sharing (#26) and loading (#472).

[tbsmark86]

So if you're still interested
Probably. Also feels more complex moving around multiple values instead of one opaque string I have think about that.
But don't expect it soon.
Maybe I split this into some refactoring first for smaller merges and getting a better view.

Also I think once this split of ProfileData class is in place it should be more simple to add localStorage solution for saving you 'personal' custom profile more easly. In a way this external url stuff would still be there you just have to copy&paste it once.
But let's not get ahead of ourself here.

General Question: The current code is lots of "callback hell". Is this simply because it's rather old or are there any reasons to avoid more modern Promise or async/await ?

P.s. Sry about not being aware of the server. I don't want to dive into yet another code base.

[nrenner]

Also feels more complex moving around multiple values instead of one opaque string I have think about that.

But that's only when converting to/from URL, internally it would be a single object with key/value properties?

I don't like encoded "blobs" and would prefer to keep the URL unencoded and readable whenever possible. An alternative to multiple profile:*-prefixed URL parameters could be a single parameter with the key=value pairs separated e.g. by semicolon (;) instead of the parameter seperator (&), like we do e.g. in the lonlats parameter. The URL structure should be the same with that sent to the server, so this would need to be discussed in a server issue.

Also I think once this split of ProfileData class is in place it should be more simple to add localStorage solution for saving you 'personal' custom profile more easly. In a way this external url stuff would still be there you just have to copy&paste it once.

For me, using localStorage to remember a profile is less a technical but more a conceptional issue, how it fits with permanent/shared profiles on the server, or if localStorage is even needed once you can permanently store profiles on the server.

General Question: The current code is lots of "callback hell". Is this simply because it's rather old or are there any reasons to avoid more modern Promise or async/await ?

Using callbacks is not automatically "hell". In my understanding that term refers specifically to multiple nested callbacks, which I don't think we have that many. Originally I intended to still support older browsers and Babel was only introduced very late in the project. Now we can use modern JS features for new stuff and also gradually refactor what is being worked on (but please in a separate commit).