otsaloma / poor-maps

Maps and navigation for Sailfish OS
https://openrepos.net/content/otsaloma/poor-maps
GNU General Public License v3.0
43 stars 10 forks source link

Merge OSM Scout routers into one #41

Closed rinigus closed 7 years ago

rinigus commented 7 years ago

This PR merges 2 separate routers that we had for OSM Scout Server. It required changes in OSM Scout Server side that are implemented already and will be released as 0.11 version.

In particular, I have implemented a simple parser of Valhalla's router request API. This allowed me to process the call and ask libosmscout router to calculate the route if the user has indicated that (s)he wants to use that particular router. To make my life easier, I respond to this call not in Valhalla's format, but in the format used earlier. To indicate that the response is coming from libosmscout, I have added a property API version to the response's JSON object and set it equal to libosmscout V1. So, on Poor Maps side, I just check for this property and act accordingly.

This allowed me to get rid of "libosmscout" and "Valhalla" versions and merge them into one.

There is one more PR that I would like to do and its also concerns Valhalla/Mapzen. Namely, I would like to add to and from ferry icons from Mapzen's turn-by-turn icons set. Ideally, I would do it after this PR goes through.

Testing this PR is not easy right now since the server 0.10 does not support such handling. Also, since I have added reverse geocoding to geocoder-nlp, the database format has changed and I cannot just give you a new version - it would break geocoding. So, we should probably wait till I release 0.11 and maybe I should start preparing for its release earlier than I though to allow testing.

otsaloma commented 7 years ago

There's one change that's needed. The config name needs to match the filename, i.e. if you have routers/osmscout.{json,py} then you'll need to use poor.conf.routers.osmscout.type in Python and routers.osmscout.type in QML. That's how the router ids, CONF_DEFAULTS etc. are designed to work.

Related to that, if we're reusing poor.conf.routers.osmscout.type with different values than before, then we'll need a migration, basically just blanking any previous config, something like

https://github.com/otsaloma/poor-maps/blob/master/poor/config.py#L95

if version < (0,30):
    routers = values.setdefault("routers", {})
    routers.pop("osmscout", None)
    routers.pop("osmscout_valhalla", None)

If this is unclear, I can do it, but it might take at least a couple days.

Apart from that it looks fine.

rinigus commented 7 years ago

I should have caught the config name change. I made that change and added the suggested config migration code. However, I don't know how to test the migration code. The rest seems to be doing fine.

otsaloma commented 7 years ago

The same name change is still needed in osmscout_settings.qml.

Testing the migration code is easy. Since what it does is blanks the configuration, you can find a bicycle route, quit Poor Maps, restart, and try to find a route again, car (the default) should be selected instead of bicycle. The configuration will persist once we bump the version number and the config file gets saved with 0.30.

If you do the above name change and testing, I can merge this.

A ferry icon addition would be fine, but I think I'd prefer it from Maki Icons. The 15x15 icon from there padded to 20x20 should be amount of detail wise more consistent with the existing icons in Poor Maps.

rinigus commented 7 years ago

Thank you for the help! I will test tomorrow and would commit the changes.

Re icons: I use Maki icons for Mapnik, so it should be easy to find the corresponding one. Although, there may be no specific icon for getting on or off the ferry.

otsaloma commented 7 years ago

I think you can use the ferry icon for boarding and "depart" (the dot-arrow icon already in Poor Maps) for getting off.

rinigus commented 7 years ago

I sincerely hope that this time its OK now and you haven't wasted more time for reviewing than writing it.

The configuration blanking has been tested and looks to be fine (haven't tested with 0.30 version bump though). Selecting Bicycle in OSM Scout, it stays if the application is running and changes back to the default (Car) after restart of Poor Maps.

otsaloma commented 7 years ago

Thanks, merging with minor tweaks.

rinigus commented 7 years ago

Thank you very much!