GNOME Bugzilla – Bug 728695
Implement routing via GraphHopper
Last modified: 2014-06-24 12:42:17 UTC
This is my current work on adding routing to Maps. It incorporates changes done to the original work I did this last summer and the work me and Jonas did together at FOSDEM to split out stuff in a sort of MVC-ish way. The reason for this is that both the mapView and the sidebar needs to handle and show bot route search result data and query data. So the original approach of just hacking it together turned into spaghetti. This version also uses the new v1 REST API of GraphHopper. My plan is to port the code for issuing a request from a map bubble and from the sidebar over ASAP.
Created attachment 274859 [details] [review] Add Route classes A Route is an array of Route.Segment and metadata. A Route.Segment is a lineString (an array of coordinates) that forms the road until the next segment, a human readable description of how to continue onto that road and some other metadata.
Created attachment 274860 [details] [review] Add abstract RouteService class The RouteService class is meant as a base class for route service classes.
Created attachment 274861 [details] [review] Add EPAF decoder Add a decoder for Encoded Polyline Algorithm Format (EPAF). EPAF is a Google format for space efficient serialization of coordinate series. Both OSRM and GraphHopper uses it for encoding their routes.
Created attachment 274862 [details] [review] Utils: add isArray-function Utils.isArray checks if an object is an array. This isn't very straight forward in JavaScript unfortunately so best to make a separate function for it. More information: - http://blog.niftysnippets.org/2010/09/say-what.html - http://stackoverflow.com/questions/4775722/javascript-check-if-object-is-array
Created attachment 274863 [details] [review] Utils: add isDefined-function Utils.isDefined returns false if a value is either undefined or null.
Created attachment 274864 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 274865 [details] [review] RouteService: Add GraphHopper backend Add a GraphHopper implementation of the RouteService class.
Created attachment 274866 [details] [review] Add a Route query model This will represent the route query state in the app.
Created attachment 274867 [details] [review] RouteService: hook up with models Make the route service a global service and hook it up with the query- and result models.
Created attachment 274868 [details] [review] Mapview: Add support for showing routes Connect to the route model and when it changes also update the map.
Created attachment 274869 [details] [review] TEMP: Test the routing
Cool! Thanks for this, I will start reviewing this tonight probably. Great work!
(In reply to comment #0) > > My plan is to port the code for issuing a request from a map bubble and from > the sidebar over ASAP. I think that you should only do the sidebar right now. And wait for the new marker code to get in before we do request from map bubble. If it looks like the new markers will not get in before 3.14 we could re-visit.
Review of attachment 274859 [details] [review]: Looks fine otherwise ::: src/route.js @@ +96,3 @@ + // TODO: implement (should depend on isDestination above) + return undefined; + } Maybe not include this until something wants to use it and we have an implementation.
Review of attachment 274860 [details] [review]: Do we expect to add more routeServices and have more than one at the same time? ::: src/routeService.js @@ +28,3 @@ +const EPAF = imports.epaf; +const HTTP = imports.http; + The EPAF and HTTP imports should not be here. Since they are not even present in the git tree at this point. @@ +34,3 @@ + + _init: function() { + this._session = new Soup.SessionAsync({ user_agent : "gnome-maps" }); SoupSessionAsync is deprecated. "As of libsoup 2.42, this is deprecated in favor of the plain SoupSession class (which uses both asynchronous and synchronous I/O, depending on the API used). See the porting guide." Should just be a matter of changing to Soup.Session which is no longer an abstract class and has a queue_message function that does async.
Review of attachment 274863 [details] [review]: ::: src/utils.js @@ +142,3 @@ + return (typeof obj !== 'undefined' && obj !== null); +} + What is the benefit of adding this instead of going if (obj) { ... } ?
Review of attachment 274864 [details] [review]: Some questions :) ::: src/http.js @@ +30,3 @@ + let str = Utils.isDefined(obj) + ? obj.toString() + : ''; Why do we expect the object to be undefined? @@ +49,3 @@ + if(typeof this._query[key] === 'undefined') { + this._query[key] = []; + } Same question here about the undefined-ness.
Review of attachment 274865 [details] [review]: Nit and question below ::: src/routeService.js @@ +121,3 @@ + time: 0 + }), + rest = instructions.map(this._createTurnPoint.bind(this, path)); Indentation problem. @@ +145,3 @@ + case 5: return Route.TurnPointType.VIA; + default: return null; + }; So I guess we might want to add another route service at some point. But couldn't we make our TurnPointType numbers match GraphHoppers so this function becomes unnecessary? Or is our scheme superior?
Review of attachment 274866 [details] [review]: ::: src/route.js @@ +75,3 @@ + Transportation.CAR) + }, + Why does this object need to be a GObject? And what is the gain of defining the properties like this? And if this is better then just having this.from and this.to then why aren't we doing more of it? I do not see this done in Documents or shell gjs code. @@ +100,3 @@ + this._changeSignalId = this.connect('notify', this.emit.bind(this, 'change')); + this.emit('change'); + }, Is it possible to use block/unblock for signals in gjs code?
Review of attachment 274867 [details] [review]: ::: src/mainWindow.js @@ +86,3 @@ + routeModel.reset(); + } + }).bind(this)); If routeService is global for the application then there is no reason for this code to be in the mainWindow. Can't all this be in some initRouting style method in mapView? Or is it because we need to send the routeModel to sidebar as well? Can't the query/model live inside of the routeService? Or am I just not thinking clearly? :)
(In reply to comment #13) > (In reply to comment #0) > > > > My plan is to port the code for issuing a request from a map bubble and from > > the sidebar over ASAP. > > I think that you should only do the sidebar right now. And wait for the new > marker code to get in before we do request from map bubble. If it looks like > the new markers will not get in before 3.14 we could re-visit. True. If this works out as planned, the code for doing this is like 2-3 lines though. :)
Review of attachment 274859 [details] [review]: Cool! ::: src/route.js @@ +54,3 @@ + this.time = time; + this.bbox = bbox || this._createBBox(path); + // TODO: check that the data passed actually means we have a new route Same here btw. Will fix this (or remove TODO:) before pushing. @@ +96,3 @@ + // TODO: implement (should depend on isDestination above) + return undefined; + } Yeah. It is a placeholder that I will fill in when I port over the sidebar.
Review of attachment 274860 [details] [review]: That's a good comment. I used to think I wanted to support both OSRM and GH at one point. I think I will just merge this with the Graphhopper patch though and not have an abstract class. It makes the code less readable for no value. ::: src/routeService.js @@ +28,3 @@ +const EPAF = imports.epaf; +const HTTP = imports.http; + True, thanks! @@ +34,3 @@ + + _init: function() { + this._session = new Soup.SessionAsync({ user_agent : "gnome-maps" }); Ah nice.
Review of attachment 274863 [details] [review]: ::: src/utils.js @@ +142,3 @@ + return (typeof obj !== 'undefined' && obj !== null); +} + Not sure actually. Will think about this. I wrote this helper last summer. :P
Review of attachment 274864 [details] [review]: I need to go over this patch again. I think I might have been tired when I wrote this code. Could use some comments too. ::: src/http.js @@ +35,3 @@ + +const Query = new Lang.Class({ + Name: 'HTTPQuery', Either change the Name property to Query or change the const name to HTTPQuery @@ +37,3 @@ + Name: 'HTTPQuery', + + // _query: {}, Remove this. @@ +49,3 @@ + if(typeof this._query[key] === 'undefined') { + this._query[key] = []; + } Yeah not sure unfortunately. Will go over it tonight. @@ +60,3 @@ + toString: function() { + let vars = []; + // log(JSON.stringify(this._query)); Remove this.
Review of attachment 274865 [details] [review]: ::: src/routeService.js @@ +121,3 @@ + time: 0 + }), + }); Nah, it's just this kind of block: let x, y; ...but with a few extra lines in between. However, I can add a let before the "rest" var actually. @@ +145,3 @@ + case 5: return Route.TurnPointType.VIA; + default: return null; + bbox = new Champlain.BoundingBox(); Yeah you are right. I'll do that actually.
Review of attachment 274866 [details] [review]: ::: src/route.js @@ +75,3 @@ + Transportation.CAR) + }, + '', This is how you add real gobject properties. What this lets me do is do a GBinding with the sidebar form later. @@ +100,3 @@ + this._changeSignalId = this.connect('notify', this.emit.bind(this, 'change')); + this.emit('change'); + There's thaw and freeze, but if you freeze and then do a bunch of changes and then thaw you'll still get a series of emits from my understanding.
Review of attachment 274867 [details] [review]: ::: src/mainWindow.js @@ +86,3 @@ + routeModel.reset(); + } + routeModel.update(result); What I want to do is something like: routeQuery.connect('change', (function() { if(routeQuery.from && routeQuery.to) { Application.routeService.getRoute(routeQuery, resultModel); } else { // TODO: implement // NOTE: think about whether we should reset here routeModel.reset(); }); That way the init code there is much smaller. But you are correct that the reason for not doing it in mapView is because the model needs to be sent to sidebar too. It might make sense to add the models to the routeService as in parameters to its constructor. Then I could init it all in application instead. At the same time currently the routeService is only concerned with getting a route and not with updating and handling models. So might be worth keeping it that way for SoC reasons. Really not sure, and don't care too much either. As long as the init is kept out of mapView :P
*** Bug 698470 has been marked as a duplicate of this bug. ***
Review of attachment 274863 [details] [review]: ::: src/utils.js @@ +142,3 @@ + return (typeof obj !== 'undefined' && obj !== null); +} + Understood what you meant here when we spoke earlier, I thought you meant what the reason for having this function was. (Which is a valid question, and I'm going to double check if I actually need this function where I currently use it). This function just checks for null and undefined. It sees other "falsy" values as being "defined". From a node session I run just now, to illustrate this: > function isDefined(obj) { return (typeof obj !== 'undefined' && obj !== null); } > isDefined(9) true > isDefined(0) true > isDefined("") true > isDefined(false) true > isDefined({}) true > isDefined(undefined) false > isDefined(null) false
Review of attachment 274864 [details] [review]: ::: src/http.js @@ +30,3 @@ + let str = Utils.isDefined(obj) + ? obj.toString() + : ''; Well say you want to produce a query like "?name=Pelle&surname=Olsson&age=0&debug" then you'd do like this: > let q = new HTTP.Query({ name: "Pelle", surname: "Olsson", age: 0, debug: null /* or undefined */ }); Soup.URI.encode takes a string so we need to convert the data we get to strings, and undefined and null doesn't have toString on them. @@ +49,3 @@ + if(typeof this._query[key] === 'undefined') { + this._query[key] = []; + } A key can have multiple values. For example for queries like this: "?point=11.96,57.10&point=12.96,58.10" So the above line kind of checks if the field "key" is set to something (it doesn't check if the object has such a field, but only if it is actually just "undefined". I think I should change it to looking if the field is actually an array: if(!Utils.isArray(this._query[key])) { this._query[key] = []; } That seems clearer.
Review of attachment 274866 [details] [review]: ::: src/route.js @@ +75,3 @@ + Transportation.CAR) + }, + '', Giovanni uses this in gnome-app-tool here (just to explain what I try to do): https://github.com/gcampax/gnome-app-tool/blob/master/src/project.js#L40 https://github.com/gcampax/gnome-app-tool/blob/master/src/project.js#L283
Created attachment 275586 [details] [review] Add EPAF decoder Add a decoder for Encoded Polyline Algorithm Format (EPAF). EPAF is a Google format for space efficient serialization of coordinate series. Both OSRM and GraphHopper uses it for encoding their routes.
Created attachment 275587 [details] [review] Utils: add isArray-function Utils.isArray checks if an object is an array. This isn't very straight forward in JavaScript unfortunately so best to make a separate function for it. More information: - http://blog.niftysnippets.org/2010/09/say-what.html - http://stackoverflow.com/questions/4775722/javascript-check-if-object-is-array
Created attachment 275588 [details] [review] Utils: add hasValue-function Utils.hasValue returns true if a value is neither undefined nor null.
Created attachment 275589 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 275590 [details] [review] Add Route result model The Route result model encapsulates all data that makes a route in our application.
Created attachment 275591 [details] [review] Add a Route query model This will represent the route query state in the app.
Created attachment 275592 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 275593 [details] [review] Application: make RouteService a global Make the route service a global service.
Created attachment 275594 [details] [review] Mapview: Add support for showing routes Connect to the route model and when it changes also update the map.
Created attachment 275595 [details] [review] TEMP: Test the routing
I think I got all the comments fixed, except the one about the gobject-properties.
Review of attachment 274863 [details] [review]: ::: src/utils.js @@ +142,3 @@ + return (typeof obj !== 'undefined' && obj !== null); +} + Can't we just ensure that object is always defined (i-e initialized to null)?
Review of attachment 274863 [details] [review]: ::: src/utils.js @@ +142,3 @@ + return (typeof obj !== 'undefined' && obj !== null); +} + Yeah, currently this is just used in http.js, and there I could easily state that undefined is not a valid parameter, and just crash. That's ok I think. If we ever really /need/ to check for undefined and null at the same time we can reintroduce that patch then.
Created attachment 275633 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Review of attachment 275586 [details] [review]: Looks fine.
Review of attachment 275587 [details] [review]: ::: src/utils.js @@ +138,3 @@ + return Object.prototype.toString.call(obj) === '[object Array]'; +} + Can't this be used? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray "Summary The Array.isArray() method returns true if an object is an array, false if it is not." It seems to be used in gnome-shell.
Review of attachment 275590 [details] [review]: See below, looks fine otherwise. ::: src/route.js @@ +55,3 @@ + this.time = time; + this.bbox = bbox || this._createBBox(path); + // TODO: check that the data passed actually means we have a new route Is this difficult to do right now? Or could it be done before we commit this? @@ +97,3 @@ + // TODO: implement (should depend on isDestination above) + return undefined; + } If it's not used and not implemented maybe we could leave it out?
Review of attachment 275591 [details] [review]: Query might be called a non-trivial class. So it should go in a module of its own. Would that pose problems? ::: src/route.js @@ +76,3 @@ + Transportation.CAR) + }, + There is a reason for why this need to be proper properties I gather. Will it be super apparent with later patches or do we need a comment?
Review of attachment 275587 [details] [review]: ::: src/utils.js @@ +138,3 @@ + return Object.prototype.toString.call(obj) === '[object Array]'; +} + Yes. Awkward. :)
Review of attachment 275590 [details] [review]: ::: src/route.js @@ +55,3 @@ + this.time = time; + this.bbox = bbox || this._createBBox(path); + // TODO: check that the data passed actually means we have a new route Hm, I don't think we should do this actually. If get a new route that is the same as the old one it's a bug (if we want to check for this at all it should be done by comparing the query object instead). This is just a case of me being a bit too defensive. I'll remove the TODO. @@ +97,3 @@ + // TODO: implement (should depend on isDestination above) + return undefined; + } Yeah, it shouldn't go in like this at least. Will either remove it or implement it tonight.
Review of attachment 275591 [details] [review]: ::: src/route.js @@ +76,3 @@ + Transportation.CAR) + }, + '', Yeah, the GBindings-stuff should make it super easy to bind the model to the UI-widgets. I could add a comment here about that though, no problem. :)
Review of attachment 275592 [details] [review]: Fine otherwise, I think :) ::: src/routeService.js @@ +47,3 @@ + this._key = "VCIHrHj0pDKb8INLpT4s5hVadNmJ1Q3vi0J4nJYP"; + this._baseURL = "http://graphhopper.com/api/1/route?"; + this._locale = 'en_US'; // TODO: get this from env What does the locale control? Is it important to get this done before committing? @@ +57,3 @@ + this._query.transportation); + else + this._route.reset(); Nit. My preference when something spans more than one line of an if is to use brackets to help readability. @@ +83,3 @@ + default: return null; + } + }, Is there no better name than _vehicle? or is that a GraphHopper term? Guess it is. @@ +101,3 @@ + }, + + // TODO: error handling What error handling is missing here? What can go wrong? @@ +107,3 @@ + path = EPAF.decode(route.points), + turnPoints = this._createTurnPoints(path, route.instructions), + bbox = new Champlain.BoundingBox(); Ok. So this kind of let syntax. I see you using it here and below and above in this file. With the comma between vars. We do not use it in other places and I don't see it in other gjs projects. It might be better but maybe we should be consistent and not use it? Zeeshan, what do you think? @@ +145,3 @@ + return min <= type && type <= max + ? type + : undefined; Maybe clearer to use an if-statement instead of a multi line ? operator?
Review of attachment 275593 [details] [review]: Looks fine.
Review of attachment 275594 [details] [review]: Looks good. ::: src/mapView.js @@ +76,3 @@ + this._routeLayer.set_stroke_width(2.0); + this.view.add_layer(this._routeLayer); + Don't know if this should be addressed here but the _init of mapView sure is growing. Maybe breaking out stuff to separate init functions is called for.
Review of attachment 275633 [details] [review]: Some nits ::: src/http.js @@ +31,3 @@ + ? null + : Soup.URI.encode(obj.toString(), null); +} Nit: I think I prefer: if (!obj) return null; return Soup... @@ +48,3 @@ + if(!Utils.isArray(this._query[key])) { + this._query[key] = []; + } Nit: brackets not needed. @@ +55,3 @@ + else { + this._query[key].push(value); + } Nit: brackets not needed
Review of attachment 275592 [details] [review]: ::: src/routeService.js @@ +47,3 @@ + this._key = "VCIHrHj0pDKb8INLpT4s5hVadNmJ1Q3vi0J4nJYP"; + this._baseURL = "http://graphhopper.com/api/1/route?"; + this._locale = 'en_US'; // TODO: get this from env It determines in what language the returned routing instructions should be in. It is important to add before release, but could potentially wait. @@ +57,3 @@ + this._query.transportation); + else + this._route.reset(); Sure! @@ +83,3 @@ + default: return null; + } + }, Hm, nah I'll figure out a better name for this. :) @@ +101,3 @@ + }, + + // TODO: error handling JSON.parse mainly, but also if we're unlucky something changes on the GH side in the returned data (unlikely), and it would be good not to crash in that case. Maybe I'm being paranoid? @@ +107,3 @@ + path = EPAF.decode(route.points), + turnPoints = this._createTurnPoints(path, route.instructions), + bbox = new Champlain.BoundingBox(); Hm, we /used/ to have a bunch of code aligned like that but not anymore apparently. I prefer it since I find it easier to read and it's the style I'm used to from other projects. :) @@ +145,3 @@ + return min <= type && type <= max + ? type + : undefined; I don't agree. I almost always find expressions easier to read than multiple statements. However just pick something and I'll fix up the code. :)
Review of attachment 275594 [details] [review]: ::: src/mapView.js @@ +76,3 @@ + this._routeLayer.set_stroke_width(2.0); + this.view.add_layer(this._routeLayer); + I think we should split out the various layers into different classes and add getter-properties for them and then move some of the methods and init code out to each of those classes (and ofcourse keep a bunch in mapView). To show a marker you'd do something like this: > mapView.markerLayer.add(new Marker()); In the mean time, I could add an extra patch that splits out the layer adding to something like _initLayers or so.
Created attachment 275709 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 275710 [details] [review] Add Route result model The Route result model encapsulates all data that makes a route in our application.
Created attachment 275711 [details] [review] Add a Route query model This will represent the route query state in the app.
Created attachment 275712 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 275713 [details] [review] Application: make RouteService a global Make the route service a global service.
Created attachment 275714 [details] [review] Mapview: Add support for showing routes Connect to the route model and when it changes also update the map.
Created attachment 275715 [details] [review] TEMP: Test the routing
Created attachment 275716 [details] [review] Make GeoClue a global service Move GeoClue out from mapView and put it together with the other services.
Created attachment 275717 [details] [review] GeoClue: Remove trailing comma
Created attachment 275718 [details] [review] MapLocation: Add route search button Make it possible to make simple route searches via location bubbles. More advanced searches will have to be made via the sidebar.
Sorry for pushing some identical versions of patches that were already accepted. I was in a bit of a hurry yesterday night leaving from our hacking venue. :)
Review of attachment 275633 [details] [review]: ::: src/http.js @@ +31,3 @@ + ? null + : Soup.URI.encode(obj.toString(), null); +} We can't do that since obj might be either a value or an object (and javascript coercions etc). However I can do if(obj === null) return null; Also if you can come up with a good name for a "something" that can be either constr or var, value or object and even null then I'm all ears. :) My current thought is to call it either "a", "toBeEncoded" or "encodable". @@ +48,3 @@ + if(!Utils.isArray(this._query[key])) { + this._query[key] = []; + } Sure @@ +55,3 @@ + else { + this._query[key].push(value); + } Sure
And in the haste from yesterday, some rebase failures apparently. Gah.
Created attachment 275770 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 275771 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 275772 [details] [review] Add Route query model The Route query model encapsulates all data that makes up a route request query in Maps.
Created attachment 275773 [details] [review] Add Route result model The Route result model encapsulates all data that makes a route in our application.
Created attachment 275774 [details] [review] MapLocation: Add route search button Make it possible to make simple route searches via location bubbles. More advanced searches will have to be made via the sidebar.
Created attachment 275775 [details] [review] MapView: cleanup _init Split out some of the mapView initialization into its own _init* methods.
Created attachment 275776 [details] [review] RouteService: Add error checking Add error checking for the route fetching. At some point we want to show notifications when we get errors also, but that's not today.
Tried to only attach the patches that I changed, but I then failed to apply them to a clean master. Re-doing. (note: is this supposed to work do you think?)
Created attachment 275777 [details] [review] Add EPAF decoder Add a decoder for Encoded Polyline Algorithm Format (EPAF). EPAF is a Google format for space efficient serialization of coordinate series. Both OSRM and GraphHopper uses it for encoding their routes.
Created attachment 275778 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 275779 [details] [review] Add Route result model The Route result model encapsulates all data that makes a route in our application.
Created attachment 275780 [details] [review] Add Route query model The Route query model encapsulates all data that makes up a route request query in Maps.
Created attachment 275781 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 275782 [details] [review] Application: make RouteService a global Make the route service a global service.
Created attachment 275783 [details] [review] Mapview: Add support for showing routes Connect to the route model and when it changes also update the map.
Created attachment 275784 [details] [review] TEMP: Test the routing
Created attachment 275785 [details] [review] Make GeoClue a global service Move GeoClue out from mapView and put it together with the other services.
Created attachment 275786 [details] [review] GeoClue: Remove trailing comma
Created attachment 275787 [details] [review] MapLocation: Add route search button Make it possible to make simple route searches via location bubbles. More advanced searches will have to be made via the sidebar.
Created attachment 275788 [details] [review] MapView: cleanup _init Split out some of the mapView initialization into its own _init* methods.
Created attachment 275789 [details] [review] RouteService: Add error checking Add error checking for the route fetching. At some point we want to show notifications when we get errors also, but that's not today.
Review of attachment 275633 [details] [review]: ::: src/http.js @@ +31,3 @@ + ? null + : Soup.URI.encode(obj.toString(), null); +} Named it "data" :)
Review of attachment 275592 [details] [review]: ::: src/routeService.js @@ +47,3 @@ + this._key = "VCIHrHj0pDKb8INLpT4s5hVadNmJ1Q3vi0J4nJYP"; + this._baseURL = "http://graphhopper.com/api/1/route?"; + this._locale = 'en_US'; // TODO: get this from env Added this in the latest patches. @@ +83,3 @@ + default: return null; + } + }, Added a toString-method on the Transportation object instead. @@ +101,3 @@ + }, + + // TODO: error handling Added a commit with error handling. @@ +107,3 @@ + path = EPAF.decode(route.points), + turnPoints = this._createTurnPoints(path, route.instructions), + bbox = new Champlain.BoundingBox(); Went with the extra let's here. @@ +145,3 @@ + return min <= type && type <= max + ? type + : undefined; Went with the statement style here.
Review of attachment 275594 [details] [review]: ::: src/mapView.js @@ +76,3 @@ + this._routeLayer.set_stroke_width(2.0); + this.view.add_layer(this._routeLayer); + Did some split out of init code in one of the new patches.
Review of attachment 275591 [details] [review]: ::: src/route.js @@ +76,3 @@ + Transportation.CAR) + }, + '', I got side tracked with other stuff today (error checking and locales and such) but I'll look at this later. If the GBindings stuff is as easy and nice to use as I've understood it to be, I'll add a note here. Otherwise I'll remove it. But I'll leave it here until I know more.
Review of attachment 275777 [details] [review]: ack
Review of attachment 275778 [details] [review]: A part from nit, ack. ::: src/http.js @@ +25,3 @@ +const Lang = imports.lang; + +const Utils = imports.utils; Nit: Utils not used anymore.
Review of attachment 275779 [details] [review]: Apart from nit, ack. ::: src/route.js @@ +23,3 @@ +const GeoCode = imports.gi.GeocodeGlib; +const GObject = imports.gi.GObject; + Nit: these are not used.
Review of attachment 275779 [details] [review]: ::: src/route.js @@ +33,3 @@ + BIKE: 1, + PEDESTRIAN: 2 +}; This is not used here, btw.
Review of attachment 275780 [details] [review]: ack
Review of attachment 275781 [details] [review]: Fine otherwise I think. ::: src/routeService.js @@ +59,3 @@ + } else + this._route.reset(); + }).bind(this)); Maybe we can get a comment above this code block? To make it easier to parse. @@ +72,3 @@ + this.route.update(this._parseResult(result)); + } else { + log("Error: " + message.status_code); Utils.debug I think, or if we expect this to actually happen sometimes then maybe notify users in some way.
Review of attachment 275782 [details] [review]: ack
Review of attachment 275783 [details] [review]: ack otherwise. ::: src/mapView.js @@ +204,3 @@ + this.view.ensure_visible(route.bbox, true); + }, + Same comment I had over the phone :) I think we should zoom in to the center of the bbox then getting the zoom-level that fits the bbox of the route. If you want to do this at a later point, maybe add a TOOD?
Review of attachment 275787 [details] [review]: I do not want to add this right now. I think the sidebar way of getting routes can go in now. And we wait for the GtkPopover markers before routing from locations.
Review of attachment 275785 [details] [review]: This might be a good idea. But maybe it should be done as part of bug #727706 instead.
Review of attachment 275778 [details] [review]: ::: src/http.js @@ +25,3 @@ +const Lang = imports.lang; + +const Utils = imports.utils; Fixed.
Review of attachment 275779 [details] [review]: ::: src/route.js @@ +23,3 @@ +const GeoCode = imports.gi.GeocodeGlib; +const GObject = imports.gi.GObject; + Fixed @@ +33,3 @@ + BIKE: 1, + PEDESTRIAN: 2 +}; Yup, a little of yesterdays rebase failure that got left behind. Fixed.
Review of attachment 275781 [details] [review]: ::: src/routeService.js @@ +59,3 @@ + } else + this._route.reset(); + }).bind(this)); Hm, while trying to come up with a good comment I realized I really can't justify resetting the route when the query isn't well-formed. Would it be clear enough if I just remove the else branch? @@ +72,3 @@ + this.route.update(this._parseResult(result)); + } else { + log("Error: " + message.status_code); Yeah, this gets fixed in the Error handling patch later. I don't like having code added in one patch and removed in another. But rebasing this around yesterday got hairy. Would it be ok to just either remove this check against 200 here (and have no error checking what so ever in this patch) and just add all error checking in the error checking commit?
Review of attachment 275785 [details] [review]: Hm yeah, makes sense. I will need it in the sidebar though and since I won't make the sidebar a child of mapView anymore navigating to the geoclue service will need to do something similar, so this bug will depend on #727706 in that case. Maybe that's what is needed though.
Review of attachment 275783 [details] [review]: ::: src/mapView.js @@ +204,3 @@ + this.view.ensure_visible(route.bbox, true); + }, + Yeah, I'll either add a TODO or fix this tonight.
Comment on attachment 275786 [details] [review] GeoClue: Remove trailing comma Attachment 275786 [details] pushed as 1e6f623 - GeoClue: Remove trailing comma
Review of attachment 275788 [details] [review]: ack
Review of attachment 275789 [details] [review]: Why do we not want to show notifications today? I think it's important to separate an error parsing the JSON we get back. With the fact that no route was found. I tried to get a route from New York to Malmö and it got reportad as an error in the console log. I'm guessing there are a lot of places that we cannot found a route between so it should not be treated as an exception. I would expect to get information saying that no route was found between the two points I wanted to get a route between. We have in-app notification right now in Maps, why not use it for that case? ::: src/routeService.js @@ +79,3 @@ + [JSON.stringify(path)]); + } +}); Do we really need this here? It seems a bit much. The ParseMsgError could be moved to Utils and used for when we do HTTP-stuff and need to debug errors. But the RouteError stuff, can't we just Utils.debug the JSON and construct human readable errors for the case of "Error occurred when fetching route" and "No route was found from X to Y" @@ +124,3 @@ + e.debug(); + } else + throw e; Where are we supposed to catch this? And what error can this be?
Review of attachment 275781 [details] [review]: ::: src/routeService.js @@ +72,3 @@ + this.route.update(this._parseResult(result)); + } else { + log("Error: " + message.status_code); I would rather this commit be merged with the error handling commit.
Review of attachment 275781 [details] [review]: ::: src/routeService.js @@ +59,3 @@ + } else + this._route.reset(); + }).bind(this)); Hm, I'll do this I think. @@ +72,3 @@ + this.route.update(this._parseResult(result)); + } else { + log("Error: " + message.status_code); The whole commit?
Review of attachment 275783 [details] [review]: ::: src/mapView.js @@ +204,3 @@ + this.view.ensure_visible(route.bbox, true); + }, + Fixed
Created attachment 277104 [details] [review] HTTP: add a HTTP helper module Http.Query is a class for easy construction of HTTP query-lines, that is the part after '?' and before '#' in an HTTP URL.
Created attachment 277105 [details] [review] Add Route result model The Route result model encapsulates all data that makes a route in our application.
Created attachment 277106 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 277107 [details] [review] Mapview: Add support for showing routes Connect to the route model and when it changes also update the map.
Created attachment 277108 [details] [review] Add RouteService module Add a RouteService module with a GraphHopper class as only implementation.
Created attachment 277110 [details] [review] Make GeoClue a global service Move GeoClue out from mapView and put it together with the other services.
Review of attachment 277104 [details] [review]: Ack
Review of attachment 277105 [details] [review]: Ack
Review of attachment 277107 [details] [review]: Ack
Review of attachment 277108 [details] [review]: Ack.
Review of attachment 277110 [details] [review]: Ok
Created attachment 278230 [details] [review] Split out geocode to its own service This is needed for when you want to perform a geocode query without access to MapView.
Created attachment 278231 [details] [review] Update: RouteService - Make fetchRoute take GeocodePlace instead of a coordinate. - emit 'updated' instead of 'change' Squash with: 876117b Add RouteService module
Created attachment 278232 [details] [review] Update: Route Remove unneeded parameter Squash with: 7d6d3b1 Add Route query model
Created attachment 278233 [details] [review] Update: RouteQuery Some changes to the routequery needed for the bind_property stuff that I didn't anticipate. - Change signal from change to updated (and use glib signals) - Add setters and getters that does notify() calls - some small refactorings Squash with: 7d6d3b1 Add Route query model
Created attachment 278234 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes.
These five patches above is the code needed for the current sidebar work. The Update-patches are meant to be squashed with their respective commits. I didn't re-upload those patches since they already are accepted. My thought is that we can iterate on those and then squash them before pushing to master. Notable stuff missing: - Some icons (see https://bugzilla.gnome.org/show_bug.cgi?id=702567) - a gray line between the result listbox and the search form (I tried for an hour to add it, will try more later and update patches) - nothing happens when you click an instruction in the sidebar (intentionally left out for Dario to work on, he already has some code for that)
Review of attachment 278230 [details] [review]: It might make sense to let this depend on the mapView and initialize it in mainWindow or so.
Review of attachment 278232 [details] [review]: This should be squashed with the patch that adds route.js obviously
Review of attachment 278230 [details] [review]: What is your thinking with still having the geocodeSearch method on the mapView module? ::: src/geocodeService.js @@ +30,3 @@ + Name: 'GeocodeService', + + _init: function() { }, Maybe we could have a 'bounded' property on this object? That defaults to false. In other words add setter/getter for bounded? Maybe that's over designing this. If we need to do bounded searches then we could add it.
Review of attachment 278231 [details] [review]: Looks fine
Review of attachment 278232 [details] [review]: Ok, looks fine.
Review of attachment 278233 [details] [review]: Some nits. ::: src/routeQuery.js @@ +88,3 @@ + }, + + set transportation(t) { Maybe t => transportation? Or is that not possible? @@ +105,3 @@ + this._updatedId = this.connect('notify', (function() { + this.emit('updated'); + }).bind(this)); Is the anonymous function needed or can we use this.emit.bind(this, 'updated'); ?
Review of attachment 278234 [details] [review]: Thanks! About the PlaceEntry class. If we want it we might want to use it for our main SearchEntry as well, as I know you know. Then it would need to be broken out. That would probably also make the ui file prettier. ::: src/mainWindow.js @@ +20,3 @@ * Author: Cosimo Cecchi <cosimoc@redhat.com> * Zeeshan Ali (Khattak) <zeeshanak@gnome.org> + * Mattias Bengtsson <mattias.jc.bengtsson@gmail.com> Does this patch alone merit this? I'm not saying your name shouldn't be here, but maybe in a commit of its own then? @@ +55,3 @@ this._configureId = 0; let ui = Utils.getUIObject('main-window', [ 'app-window', + 'main-grid', Am I blind or is this not used anywhere? @@ +71,3 @@ + overlay.add_overlay(this._sidebar); + Application.routeService.route.connect('update', + this._setRevealSidebar.bind(this, true)); Would this be possible and/or prettier with a bind_property? @@ +154,3 @@ action, 'enabled', GObject.BindingFlags.SYNC_CREATE); + Newline added here? Remove please. @@ +310,3 @@ + + let reveal = variant.get_boolean(); + if(!reveal) { Space after if. @@ +320,3 @@ + this.window + .lookup_action('toggle-sidebar') + .change_state(GLib.Variant.new_boolean(value)); I think I would prefer to split this up. let action = this.window.lookup_action('toggle-sidebar'); action.change_state... ::: src/route.js @@ +108,3 @@ + // case dir.END: return '/org/gnome/maps/direction-end'; + // case dir.VIA: return '/org/gnome/maps/direction-via'; + // case dir.START: return '/org/gnome/maps/direction-start'; These will come with via points I presume? I think we can remove this, as we are not likely to forget. ::: src/sidebar.js @@ +36,1 @@ Don't align the imports. @@ +42,3 @@ + this.parent({ visible: true, + transition_type: Gtk.RevealerTransitionType.SLIDE_LEFT, + transition_duration: 400, Should we use a const variable for the duration? Not sure. Maybe? A comment that it is in ms? @@ +73,3 @@ + _initTransportationToggles: function(pedestrian, bike, car) { + let query = Application.routeService.query; + let transport = RouteQuery.Transportation; Don't align these. @@ +86,3 @@ + pedestrian.active = query.transportation === transport.PEDESTRIAN; + car.active = query.transportation === transport.CAR; + bike.active = query.transportation === transport.BIKE; Is this prettier than a switch statement or using ifs? @@ +89,3 @@ + }); + }, + Btw same thing with these blocks above: do not align this way. @@ +92,3 @@ + _createEntry: function(propName, completion) { + completion.model = Application.placeStore; + completion.match_func = PlaceStore.completionMatchFunc; Here as well. @@ +126,3 @@ +const PlaceEntry = new Lang.Class({ + Name: 'PlaceEntry', + Extends: Gtk.Entry, Shouldn't this be Gtk.SearchEntry? @@ +130,3 @@ + 'place': GObject.ParamSpec.object('place', + '', + '', Should we fill in blurb and description? We would if this was C. @@ +132,3 @@ + '', + GObject.ParamFlags.READABLE | + GObject.ParamFlags.WRITABLE, READWRITE does not work? @@ +176,3 @@ + if (places === null) { + popover.hide(); + log(places); This log seems odd. ::: src/sidebar.ui @@ +208,3 @@ + </child> + </object> + whitespace problems
Thanks for the patches! Very cool. I would love for Andreas and/or other designers to comment on the general experience as well. Right now it is kind of hard to apply this as you will have to do interactive bz apply and know the order. Is there some branch you could pointer people to that want to try this out? Some general comments from me: - The size of the popovers are a bit to wide I feel. SearchPopup is hard coded to be 500 pixels wide atm. That feels unnecessary here. Would it be to much of a hack to have it steal the width of the relativeTo widget? That might help. - Will there be any icon representing the start and end of a route on the map? I feel that would improve the ui. - I am not 100% sure about clearing the sidebar from/to and removing the route when we remove the sidebar. But I haven't played around with it all that much so I'm not sure. At all. - It really really is a shame that we cannot do remote autocomplete. The ui is a bit awkward with having to press enter and then select.
Also: Is there no way we can get the distance for each part of the route? And display it somehow? Like a "after x meters turn left" sort of thing.
Review of attachment 278233 [details] [review]: ::: src/routeQuery.js @@ +88,3 @@ + }, + + set transportation(t) { Sure! @@ +105,3 @@ + this._updatedId = this.connect('notify', (function() { + this.emit('updated'); + }).bind(this)); Nah, maybe I should've added a comment there, but there's two parameters passed to the 'notify'-callback that if passed on to the 'updated'-emit makes it barf (since that signal take zero parameters).
Review of attachment 278234 [details] [review]: Regarding PlaceEntry: should I just split it out into its own commit and module or also try to convert the headerbar version to it? I'd like to do that but I'd also like to wait til this is merged if you're ok with that. ::: src/mainWindow.js @@ +20,3 @@ * Author: Cosimo Cecchi <cosimoc@redhat.com> * Zeeshan Ali (Khattak) <zeeshanak@gnome.org> + * Mattias Bengtsson <mattias.jc.bengtsson@gmail.com> Hm, yeah. I can actually just put it out of there. @@ +55,3 @@ this._configureId = 0; let ui = Utils.getUIObject('main-window', [ 'app-window', + 'main-grid', Not used. Will remove it. :) @@ +71,3 @@ + overlay.add_overlay(this._sidebar); + Application.routeService.route.connect('update', + this._setRevealSidebar.bind(this, true)); Hm, you could probably make a property called "isEmpty" or "hasValue" or something and bind to that. I'm not sure that would be prettier though. What do you think? @@ +154,3 @@ action, 'enabled', GObject.BindingFlags.SYNC_CREATE); + Yes! @@ +310,3 @@ + + let reveal = variant.get_boolean(); + if(!reveal) { Ah true. @@ +320,3 @@ + this.window + .lookup_action('toggle-sidebar') + .change_state(GLib.Variant.new_boolean(value)); Sure! ::: src/route.js @@ +108,3 @@ + // case dir.END: return '/org/gnome/maps/direction-end'; + // case dir.VIA: return '/org/gnome/maps/direction-via'; + // case dir.START: return '/org/gnome/maps/direction-start'; END and START will come when Andreas draws those icons. VIA will come with the viapoints patches indeed. I'll remove the via and update with START/END as soon as Andreas has those icons done. ::: src/sidebar.js @@ +36,1 @@ Ok sure. @@ +42,3 @@ + this.parent({ visible: true, + transition_type: Gtk.RevealerTransitionType.SLIDE_LEFT, + transition_duration: 400, Yeah a comment seems enough I think. I'll add that. @@ +73,3 @@ + _initTransportationToggles: function(pedestrian, bike, car) { + let query = Application.routeService.query; + let transport = RouteQuery.Transportation; Ok sure. @@ +86,3 @@ + pedestrian.active = query.transportation === transport.PEDESTRIAN; + car.active = query.transportation === transport.CAR; + bike.active = query.transportation === transport.BIKE; I went back and forth a bit here. A switch seems fine. @@ +89,3 @@ + }); + }, + Ok sure. @@ +92,3 @@ + _createEntry: function(propName, completion) { + completion.model = Application.placeStore; + completion.match_func = PlaceStore.completionMatchFunc; Ok. @@ +126,3 @@ +const PlaceEntry = new Lang.Class({ + Name: 'PlaceEntry', + Extends: Gtk.Entry, Ah yeah that's probably true. @@ +130,3 @@ + 'place': GObject.ParamSpec.object('place', + '', + '', Yeah perhaps. I'll read up on what blurb is add a description as well. @@ +132,3 @@ + '', + GObject.ParamFlags.READABLE | + GObject.ParamFlags.WRITABLE, I'll look into that. I had the impression that that might not be available in gjs. @@ +176,3 @@ + if (places === null) { + popover.hide(); + log(places); Sure does. :) Will remove it. ::: src/sidebar.ui @@ +208,3 @@ + </child> + </object> + Ah thx! I'll google that git command you used in London and use that in the future.
(In reply to comment #149) > Also: Is there no way we can get the distance for each part of the route? And > display it somehow? Like a "after x meters turn left" sort of thing. Ah no that's totally possible, and might even be pretty simple.
(In reply to comment #148) > Thanks for the patches! Very cool. > > I would love for Andreas and/or other designers to comment on the general > experience as well. Yes, me too! > Right now it is kind of hard to apply this as you will have to do interactive > bz apply and know the order. Is there some branch you could pointer people to > that want to try this out? wip/routing2 :) > Some general comments from me: > > - The size of the popovers are a bit to wide I feel. SearchPopup is hard coded > to be 500 pixels wide atm. That feels unnecessary here. Would it be to much of > a hack to have it steal the width of the relativeTo widget? That might help. Yeah I totally forgot that. Watching it be like that for a couple of weeks made me blind to it. Will look into some solution. Either a property to the PlaceEntry or something like what you proposed. > - Will there be any icon representing the start and end of a route on the map? > I feel that would improve the ui. Yeah, waiting for icons from Andreas (that's the two commented out lines in that switch-statement you commented on btw). > - I am not 100% sure about clearing the sidebar from/to and removing the route > when we remove the sidebar. But I haven't played around with it all that much > so I'm not sure. At all. I think at least hiding the route is what we should do. There needs to be a way to get out of "route mode" so to speak. Maybe make some logic later to restore it if you press the route button again. > - It really really is a shame that we cannot do remote autocomplete. The ui is > a bit awkward with having to press enter and then select. I totally agree. It's the same with the searchEntry in the headerbar but it feels even worse here.
Comment on attachment 275784 [details] [review] TEMP: Test the routing Not needed anymore.
Created attachment 278300 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes.
Review of attachment 278234 [details] [review]: ::: src/mainWindow.js @@ +20,3 @@ * Author: Cosimo Cecchi <cosimoc@redhat.com> * Zeeshan Ali (Khattak) <zeeshanak@gnome.org> + * Mattias Bengtsson <mattias.jc.bengtsson@gmail.com> Fixed. @@ +55,3 @@ this._configureId = 0; let ui = Utils.getUIObject('main-window', [ 'app-window', + 'main-grid', Fixed. @@ +154,3 @@ action, 'enabled', GObject.BindingFlags.SYNC_CREATE); + Fixed. @@ +310,3 @@ + + let reveal = variant.get_boolean(); + if(!reveal) { Fixed. ::: src/route.js @@ +108,3 @@ + // case dir.END: return '/org/gnome/maps/direction-end'; + // case dir.VIA: return '/org/gnome/maps/direction-via'; + // case dir.START: return '/org/gnome/maps/direction-start'; Removed the VIA-branch. ::: src/sidebar.js @@ +36,1 @@ Fixed @@ +86,3 @@ + pedestrian.active = query.transportation === transport.PEDESTRIAN; + car.active = query.transportation === transport.CAR; + bike.active = query.transportation === transport.BIKE; Changed to a switch. @@ +89,3 @@ + }); + }, + Fixed. @@ +126,3 @@ +const PlaceEntry = new Lang.Class({ + Name: 'PlaceEntry', + Extends: Gtk.Entry, Fixed. @@ +132,3 @@ + '', + GObject.ParamFlags.READABLE | + GObject.ParamFlags.WRITABLE, Fixed. @@ +176,3 @@ + if (places === null) { + popover.hide(); + log(places); Fixed. Also added code to remove the old place if the result is null (like if you search for a new place and get no result, you still are looking for a new route so would be weird I think to keep the old route). ::: src/sidebar.ui @@ +208,3 @@ + </child> + </object> + Fixed.
Review of attachment 278230 [details] [review]: Regarding having geocodeSearch on mapView still it's for convenience. But maybe it's better to rip that out. Hm. And yeah we should think about this a bit. Would be nice if this knew about the visible bbox somehow like I say below. ::: src/geocodeService.js @@ +30,3 @@ + Name: 'GeocodeService', + + _init: function() { }, Hm, maybe a bounded-parameter to search() instead (since setting a stateful property that might affect later searches if we forget to reset it seems wrong) and let this service know about the current bbox somehow. Maybe through a reference to mapView that is set somehow or via some global mapView model-object (with only readables I guess). Or something. I think for now the best is to just have this as it is.
Review of attachment 278233 [details] [review]: ::: src/routeQuery.js @@ +88,3 @@ + }, + + set transportation(t) { Fixed.
Created attachment 278301 [details] [review] Split out geocode to its own service This is needed for when you want to perform a geocode query without access to MapView.
Created attachment 278302 [details] [review] Update: RouteService - Make fetchRoute take GeocodePlace instead of a coordinate. - emit 'updated' instead of 'change' - Some if() => if () Squash with: 876117b Add RouteService module
Created attachment 278303 [details] [review] Update: Route Remove unneeded parameter Squash with: 7d6d3b1 Add Route query model
Created attachment 278304 [details] [review] Update: RouteQuery Some changes to the routequery needed for the bind_property stuff that I didn't anticipate. - Change signal from change to updated (and use glib signals) - Add setters and getters that does notify() calls - some small refactorings - Some if() => if () Squash with: 7d6d3b1 Add Route query model
Created attachment 278305 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes.
Created attachment 278310 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes.
(In reply to comment #148) > - The size of the popovers are a bit to wide I feel. SearchPopup is hard coded > to be 500 pixels wide atm. That feels unnecessary here. Would it be to much of > a hack to have it steal the width of the relativeTo widget? That might help. Fixed this by setting the width_request on the popover manually in the latest version of the sidebar patch.
Regarding the PlaceEntry class, I did a split out of that. I think it's generally useful so I put it in a separate bug here: https://bugzilla.gnome.org/show_bug.cgi?id=731545
(In reply to comment #153) > > - Will there be any icon representing the start and end of a route on the map? > > I feel that would improve the ui. > > Yeah, waiting for icons from Andreas (that's the two commented out lines in > that switch-statement you commented on btw). > And those will be used both on the map and in the sidebar? Like marking the start-point/end-point of the drawn route.
Review of attachment 278301 [details] [review]: Acky!
Review of attachment 278302 [details] [review]: Looks fine to squash.
Review of attachment 278303 [details] [review]: Ok!
Review of attachment 278304 [details] [review]: Ok to squash
Review of attachment 278310 [details] [review]: Looks fine, will give final accepted when PlaceEntry is split out and icons have arrived. ::: src/sidebar.ui @@ +40,3 @@ + </attributes> + </child> + </object> These will not be needed when we get a stand-alone PlaceEntry, right? Just a reminder :)
Review of attachment 278301 [details] [review]: Found a nitty nit. ::: src/geocodeService.js @@ +43,3 @@ + right: bbox.right + }); + } If bbox is null, maybe we should set the search_area to null as well. Or even refuse to search. We do not perform any searches without a bbox search_area today, but we might(?)
(In reply to comment #152) > (In reply to comment #149) > > Also: Is there no way we can get the distance for each part of the route? And > > display it somehow? Like a "after x meters turn left" sort of thing. > > Ah no that's totally possible, and might even be pretty simple. Cool, I think that would be a big win. Both having a distance on each entry of the instructionlist so that you can see how big of a part of the route it is. And later we should have some kind of estimation of time as well. For the complete route. So that I can check how long it will take me to bike from a to b etc.
Created attachment 278379 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes. - Add icons
Review of attachment 278301 [details] [review]: ::: src/geocodeService.js @@ +43,3 @@ + right: bbox.right + }); + } What difference does setting it to null and not setting it at all do? The PlaceEntries in the sidebar doesn't set a boundingbox currently. Not sure if I should or not?
Review of attachment 278310 [details] [review]: ::: src/sidebar.ui @@ +40,3 @@ + </attributes> + </child> + </object> Yeah, I should probably just create them in code (but that's for that other bug).
Review of attachment 278301 [details] [review]: ::: src/geocodeService.js @@ +43,3 @@ + right: bbox.right + }); + } Yeah you are right, I missthinked. Each search creates a new GeocodeForward and the search-area will be null already. And you should not I think... hmm, or maybe you should. Having a search-area coupled with bounded=false will make the Nominatim search prefer results that are within the bbox, but still search world-wide. A bounded=true would only return stuff inside the bbox. So it should set a bbox if we want to prefer the current view.
Review of attachment 278301 [details] [review]: ::: src/geocodeService.js @@ +43,3 @@ + right: bbox.right + }); + } Yeah, the solution is to just use the new PlaceEntry code in that other branch and pass the mapView to that.
Created attachment 278509 [details] [review] Split out geocode to its own service This is needed for when you want to perform a geocode query without access to MapView. - Updated placeEntry to use the new geocode service instead.
Created attachment 278510 [details] [review] Add a sidebar for route interaction This is a total re-implementation of the sidebar for searching and interacting with routes. - Use the PlaceEntry from #731545
wip/routing2 is now rebased upon wip/placeEntry (#731545). I also squashed the "update"-patches there. Changed the geocode patch to just remove the geocodeSearch from mapView and updated the placeEntry code for that. If you want we can just skip the geocode patch altogether for now. Also, if you want to I can push new versions of all the patches here since I guess it's even harder to do git bz apply now than before (but the branch is still wip/routing2).
Created attachment 278631 [details] [review] Update: move out user_agent property Move out user_agent property to Config.USER_AGENT so others can use that later. Squash with: - 1395656 Add RouteService module
Review of attachment 278509 [details] [review]: Seems fine.
Review of attachment 278510 [details] [review]: Except for nit below ::: src/mainWindow.js @@ -63,3 @@ this.mapView = new MapView.MapView(); overlay.add(this.mapView); - Do not remove this.
Review of attachment 278631 [details] [review]: Sure.
Attachment 275777 [details] pushed as f80873a - Add EPAF decoder Attachment 275780 [details] pushed as 840e6d2 - Add Route query model Attachment 275782 [details] pushed as 85bca92 - Application: make RouteService a global Attachment 275788 [details] pushed as 909157d - MapView: cleanup _init Attachment 277104 [details] pushed as 7d13cb5 - HTTP: add a HTTP helper module Attachment 277105 [details] pushed as 4c2c402 - Add Route result model Attachment 277107 [details] pushed as d90af21 - Mapview: Add support for showing routes Attachment 277108 [details] pushed as 34b684a - Add RouteService module Attachment 277110 [details] pushed as 25c0ec4 - Make GeoClue a global service Attachment 278509 [details] pushed as 8840dd9 - Split out geocode to its own service Attachment 278510 [details] pushed as 23f6998 - Add a sidebar for route interaction
This totally doesn't work against latest of deps from git, at least for me. I'd suggest you use jhbuild for testing git master of gnome-maps so all new features get tested against git master of all deps as next version of Maps will most likely be used with next version of the deps, Maps being part of GNOME release cycle. Since it seems only two people have tested this so far and it doesn't work for one of them, resolving this bug as FIXED isn't a good idea. :) Not that I wont be the happiest person on this planet when it does get fixed. :)
(In reply to comment #189) > This totally doesn't work against latest of deps from git, at least for me. I'd > suggest you use jhbuild for testing git master of gnome-maps so all new > features get tested against git master of all deps as next version of Maps will > most likely be used with next version of the deps, Maps being part of GNOME > release cycle. > > Since it seems only two people have tested this so far and it doesn't work for > one of them, resolving this bug as FIXED isn't a good idea. :) Apparently, I wasn't completely correct here. Firstly other people have tested routing and it does work for them. It also works for me but its just that it doesn't work for all the places I have tried previously. E.g 'London to Brighton' works but 'Brixton to Tulse Hill' doesn't work. From this video, I see that I'm not alone in that: https://www.youtube.com/watch?v=ByEoe2vLubk i-e for many places, you neither get any result nor any errors. If you haven't thought about it already, I'd suggest show a spinner in the sidebar (where the route appears) to indicate if route search is in progress.
Seems an issue with popover: bug#732054.
(In reply to comment #189) > This totally doesn't work against latest of deps from git, at least for me. I'd > suggest you use jhbuild for testing git master of gnome-maps so all new > features get tested against git master of all deps as next version of Maps will > most likely be used with next version of the deps, Maps being part of GNOME > release cycle. No need to assume I don't use JHBuild, of course I do and have been for 1,5 years. > Since it seems only two people have tested this so far and it doesn't work for > one of them, resolving this bug as FIXED isn't a good idea. :) Not that I wont > be the happiest person on this planet when it does get fixed. :) Alex D tested it as well as least. I would hope Jonas tested it as well.
(In reply to comment #190) > (In reply to comment #189) > > This totally doesn't work against latest of deps from git, at least for me. I'd > > suggest you use jhbuild for testing git master of gnome-maps so all new > > features get tested against git master of all deps as next version of Maps will > > most likely be used with next version of the deps, Maps being part of GNOME > > release cycle. > > > > Since it seems only two people have tested this so far and it doesn't work for > > one of them, resolving this bug as FIXED isn't a good idea. :) > > Apparently, I wasn't completely correct here. Firstly other people have tested > routing and it does work for them. It also works for me but its just that it > doesn't work for all the places I have tried previously. E.g 'London to > Brighton' works but 'Brixton to Tulse Hill' doesn't work. From this video, I > see that I'm not alone in that: > > https://www.youtube.com/watch?v=ByEoe2vLubk Brixton to Tulse Hill seems to work fine for me. > i-e for many places, you neither get any result nor any errors. If you haven't > thought about it already, I'd suggest show a spinner in the sidebar (where the > route appears) to indicate if route search is in progress. Hm yeah that seems like something we should do. Currently we run the spinner in the panel instead, but doing it in the sidebar seems better.
(In reply to comment #192) > (In reply to comment #189) > > This totally doesn't work against latest of deps from git, at least for me. I'd > > suggest you use jhbuild for testing git master of gnome-maps so all new > > features get tested against git master of all deps as next version of Maps will > > most likely be used with next version of the deps, Maps being part of GNOME > > release cycle. > > No need to assume I don't use JHBuild, of course I do and have been for 1,5 > years. Sorry, the reason I assumed that was that during our chat about this and in your videos, I didn't see any flickering of map, which was a recent gtk+ regression. > > Since it seems only two people have tested this so far and it doesn't work for > > one of them, resolving this bug as FIXED isn't a good idea. :) Not that I wont > > be the happiest person on this planet when it does get fixed. :) > > Alex D tested it as well as least. I would hope Jonas tested it as well. Surely you see me correctly myself about that in the later comment? :)
(In reply to comment #193) > (In reply to comment #190) > > (In reply to comment #189) > > > This totally doesn't work against latest of deps from git, at least for me. I'd > > > suggest you use jhbuild for testing git master of gnome-maps so all new > > > features get tested against git master of all deps as next version of Maps will > > > most likely be used with next version of the deps, Maps being part of GNOME > > > release cycle. > > > > > > Since it seems only two people have tested this so far and it doesn't work for > > > one of them, resolving this bug as FIXED isn't a good idea. :) > > > > Apparently, I wasn't completely correct here. Firstly other people have tested > > routing and it does work for them. It also works for me but its just that it > > doesn't work for all the places I have tried previously. E.g 'London to > > Brighton' works but 'Brixton to Tulse Hill' doesn't work. From this video, I > > see that I'm not alone in that: > > > > https://www.youtube.com/watch?v=ByEoe2vLubk > > Brixton to Tulse Hill seems to work fine for me. As I told you later in chat (sorry, forgot to update this bug), it works for me too. If you recall, we already figured what the issue was: Selection of search results through mouse doesn't work reliably. What makes it worse is that the text field does seem to get updated but place isn't actually selected somehow.