GNOME Bugzilla – Bug 736111
Make sure the default transportation mode gets notified.
Last modified: 2014-09-05 18:38:05 UTC
I did a route search on a common walking path in my town. It's ~1km and the estimated time was 1 min. This felt weird. And when digging I noticed that the search was done with transportation = CAR. Even tho the default setting of the mode chooser is PEDESTRIAN. This is because there is no notification from when the transportation mode gets reset. Which also happens at init.
Created attachment 285467 [details] [review] routeQuery: Notify transportation on reset
Review of attachment 285467 [details] [review]: Nice catch! Actually, most maps applications use the Car mode by default for routing (Bing Maps and Apple Maps at least, latest versions of Google Maps select the best transportation mode depending on the route, older versions used Car), I think we need to use CAR by default too. Also this patch won't work for the FIRST sidebar reveal, I was trying it and changed the default mode to CAR and the active button on first reveal was PEDESTRIAN, this is because the first query reset is done before initializing the sidebar, I think we need to reset the query on Sidebar::_init or manually call the query.connect('notify::transportation') transportation callback in Sidebar::_initTransportationToggles, or re-hardcode the default active button if you agree that CAR mode should be the default. ::: src/routeQuery.js @@ +43,3 @@ }; +const DefaultTransportation = Transportation.PEDESTRIAN; Isn't this constant too much? (also, shouldn't be DEFAULT_TRANSPORTATION?)
(In reply to comment #2) > Review of attachment 285467 [details] [review]: > > Nice catch! > > Actually, most maps applications use the Car mode by default for routing (Bing > Maps and Apple Maps at least, latest versions of Google Maps select the best > transportation mode depending on the route, older versions used Car), I think > we need to use CAR by default too. I agree. We can re-evaluate this still when we have public transportation as well.
(In reply to comment #2) > Review of attachment 285467 [details] [review]: > > Nice catch! > > Actually, most maps applications use the Car mode by default for routing (Bing > Maps and Apple Maps at least, latest versions of Google Maps select the best > transportation mode depending on the route, older versions used Car), I think > we need to use CAR by default too. > > Also this patch won't work for the FIRST sidebar reveal, I was trying it and > changed the default mode to CAR and the active button on first reveal was > PEDESTRIAN, this is because the first query reset is done before initializing > the sidebar, I think we need to reset the query on Sidebar::_init or manually > call the query.connect('notify::transportation') transportation callback in > Sidebar::_initTransportationToggles, or re-hardcode the default active button > if you agree that CAR mode should be the default. > > ::: src/routeQuery.js > @@ +43,3 @@ > }; > > +const DefaultTransportation = Transportation.PEDESTRIAN; > > Isn't this constant too much? (also, shouldn't be DEFAULT_TRANSPORTATION?) Yeah agreem it could go away.
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 285467 [details] [review] [details]: > > > > Nice catch! > > > > Actually, most maps applications use the Car mode by default for routing (Bing > > Maps and Apple Maps at least, latest versions of Google Maps select the best > > transportation mode depending on the route, older versions used Car), I think > > we need to use CAR by default too. > > I agree. We can re-evaluate this still when we have public transportation as > well. So I didn't get away with my anti car agenda :/
Created attachment 285515 [details] [review] routeQuery: Notify transportation on reset Make sure that the transporation mode is notified on init. This fixes a bug where the sidebar showed PEDESTRIAN as selected but the search was actually done with CAR.
Review of attachment 285515 [details] [review]: Looks good for me.
Attachment 285515 [details] pushed as e87c688 - routeQuery: Notify transportation on reset