GNOME Bugzilla – Bug 736347
don't forget route
Last modified: 2014-09-12 18:23:20 UTC
just hiding and reshowing the side-bar wipes it clean - all my carefully entered route information is lost :-( that shouldn't happen. Even better would be to keep a history of past routes, and letting me go back to previously entered ones.
Agree, actually this was pointed in [1]. About keep a history of past routes, how would you implement that, in the entry autocompletion or extra UI components exclusive for this? Maybe we need designer's voice on this. [1] https://bugzilla.gnome.org/show_bug.cgi?id=731068#c312
Yeah a history of routes would be nice. Meanwhile, I think the solution is to keep the last route search in memory and when we re-open the route-sidebar just re-show the route. What do you think?
Created attachment 285790 [details] [review] Do not forget route on sidebar hide
Created attachment 285791 [details] [review] Hide route on invalid query Without this the old route information and route will be present even tho we have removed parts of the route in the entries.
Review of attachment 285790 [details] [review]: ::: src/mapView.js @@ +65,3 @@ + + get routeVisible() { + this._routeLayer.visible && this._instructionMarkerLayer.visible; Should probably be ||
Review of attachment 285790 [details] [review]: That was easier than I thought. Nice! ::: src/mapView.js @@ +62,3 @@ + GObject.ParamFlags.READWRITE, + false) + }, I think it's custom to use dash-separated names here, like 'route-visible'. If that for some reason doesn't work, make it 'routeVisible' in both places instead. :) @@ +65,3 @@ + + get routeVisible() { + this._routeLayer.visible && this._instructionMarkerLayer.visible; Sure. No strong opinion here. :)
Review of attachment 285790 [details] [review]: Thanks for review! ::: src/mapView.js @@ +62,3 @@ + GObject.ParamFlags.READWRITE, + false) + }, I was going for routeVisible in both places actually :) The routevisible is a typo. If I name it 'route-visible' do you think it is still ok to have the getter as routeVisible? And the notify should be 'route-visible' I guess?
Review of attachment 285791 [details] [review]: Nice! ::: src/mapView.js @@ +141,3 @@ + query.connect('notify', (function() { + if (!query.isValid()) + this.routeVisible = false; So many negations that my head spins. Would > this.routeVisible = query.isValid(); work?
Created attachment 285810 [details] [review] Do not forget route on sidebar hide Updated after review.
Created attachment 285811 [details] [review] Hide route on invalid query Updated after review.
Review of attachment 285790 [details] [review]: ::: src/mapView.js @@ +62,3 @@ + GObject.ParamFlags.READWRITE, + false) + }, Hm, I don't know actually. I think you could just go with routeVisible then.
(In reply to comment #11) > Review of attachment 285790 [details] [review]: > > ::: src/mapView.js > @@ +62,3 @@ > + > GObject.ParamFlags.READWRITE, > + false) > + }, > > Hm, I don't know actually. I think you could just go with routeVisible then. I did in the updated patch. Having the name 'route-visible' means we need a getter called route_visible. And that clashes with our naming style.
Review of attachment 285810 [details] [review]: ACK
Review of attachment 285811 [details] [review]: ACK (except for tiny issue) ::: src/mapView.js @@ +140,3 @@ + query.connect('notify', (function() { + this.routeVisible = query.isValid(); Indent -4 here.
Review of attachment 285810 [details] [review]: ::: src/mainWindow.js @@ +71,3 @@ + this._sidebar.bind_property('reveal-child', + this.mapView, 'routeVisible', + GObject.BindingFlags.DEFAULT); If your query is invalid, you will show the route anyways, the steps to reproduce this are the following: 1) Startup Maps, open sidebar and search a valid route with, for example, two points. 2) Clear the second route point, so now the query is invalid. 3) Hide the sidebar. 4) Show the sidebar again, the route of item (1) is showed. Maybe an easy way to fix it is to validate the query in routeVisible setter.
(In reply to comment #15) > Review of attachment 285810 [details] [review]: > > ::: src/mainWindow.js > @@ +71,3 @@ > + this._sidebar.bind_property('reveal-child', > + this.mapView, 'routeVisible', > + GObject.BindingFlags.DEFAULT); > > If your query is invalid, you will show the route anyways, the steps to > reproduce this are the following: > > 1) Startup Maps, open sidebar and search a valid route with, for example, two > points. > 2) Clear the second route point, so now the query is invalid. > 3) Hide the sidebar. > 4) Show the sidebar again, the route of item (1) is showed. > > Maybe an easy way to fix it is to validate the query in routeVisible setter. Good catch, thanks!
Created attachment 285943 [details] [review] Do not forget route on sidebar hide
Review of attachment 285943 [details] [review]: ::: src/mapView.js @@ +65,3 @@ + + get routeVisible() { + this._routeLayer.visible || this._instructionMarkerLayer.visible; Missing return?
Review of attachment 285943 [details] [review]: ::: src/mapView.js @@ +65,3 @@ + + get routeVisible() { + this._routeLayer.visible || this._instructionMarkerLayer.visible; Yeah, I will fix it up in commit.
Attachment 285811 [details] pushed as 5ac8c23 - Hide route on invalid query Attachment 285943 [details] pushed as ddcf913 - Do not forget route on sidebar hide
Cool. Create another bug for the last part of comment#0. https://bugzilla.gnome.org/show_bug.cgi?id=736582