GNOME Bugzilla – Bug 759574
Provide keyboard shortcuts to switch between street-view and aerial-view
Last modified: 2017-06-19 19:54:21 UTC
What should they be though? <Primary>S for street? <Primary>A for aerial? This should probably be done in mainWindow.js search for accels to see were other keyboard shortcuts are added.
mainwindow.js doesn't seem to recognize the accel when I put it in 'map-type' util action or 'map-type-menu'.Do you have any suggestions?
The map-type action takes a parameter s, that determines if the map-type is street or aerial, so a simple accel will not work in that case I guess. So either we need to do this with other key-bindings or you need to re-factor to have one action for street and one for aerial. Mattias, thoughts?
Ah okay, I understood the refactoring part,but what are the other key bindings you're referring to?
(In reply to Karanbir Chahal from comment #3) > Ah okay, I understood the refactoring part,but what are the other key > bindings you're referring to? Usingthe key-press-event: https://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget-key-press-event
Thank you :)
Created attachment 323551 [details] Keyboard shortcuts : Aerial view and Street view Provided keyboard shortcuts to switch between street-view and aerial-view. The shortcuts being <Primary>S for street view and <Primary>A for aerial view.
Can we have a keyboard shortcut which toggles between the two views, it will be easier to remember and since we only have only two values so a toggle button should be fine.
Review of attachment 323551 [details]: Thanks! Primary-S is right now used to toggle the scale ruler as can be seen here: https://git.gnome.org/browse/gnome-maps/tree/src/mainWindow.js#n201 And I also wonder, could we tie this someway to the custom-layers? Or does that not make sense? I like the Alt-1,2,3,4,5 suggestion to toggle between layers. I think we need a designers input here! Andreas?
Review of attachment 323551 [details]: Also, whenever we update our keyboard shortcuts we need to update the help-overlay (https://git.gnome.org/browse/gnome-maps/tree/data/ui/help-overlay.ui) as well! In data/ui/help-overlay.ui, and make sure it is under the appropriate section!
(In reply to Nayan Deshmukh from comment #7) > Can we have a keyboard shortcut which toggles between the two views, it will > be easier to remember and since we only have only two values so a toggle > button should be fine. Yes, thins might b e an option as well! Maybe one binding that toggles between aerial/street and then alt-1,2,3,4 to toggle between custom layers? Many options!
(In reply to Jonas Danielsson from comment #8) > I like the Alt-1,2,3,4,5 suggestion to toggle between layers. I don't feel super-confident giving advice on keyboard shortcuts, but yeah, this might work.
So Alt+1 for Street view and Alt + 2 for Aerial view ? Or should I use a seperate key for the toggling the street/aerial view ? In that case , which key should be used for that ?
Created attachment 323668 [details] [review] Keyboard shortcuts : Street view and Aerial view Provided keyboard shortcuts to switch between street-view and aerial-view. The shortcuts being Alt+1 for street view and Alt+2 for aerial view. Also the help-overlay has been updated to show the information about the shortcuts under the appropriate section.
Review of attachment 323668 [details] [review]: Thanks! Please obsolete patches that are no longer needed. Otherwise it is hard to use git bz with this. And I will be reluctant to review. This patch does not apply on its own against master. And I do not like using Alt-1|2 for this, that feels like something we could add to custom layers. But not this. If anything I think we should have something that toggles between street and aerial.
Review of attachment 323551 [details]: obsolete this!
Created attachment 323773 [details] [review] Keyboard shortcut : Toggle street/aerial view Provided keyboard shortcut to toggle between street-view and aerial-view . The shortcut being Alt+M . M as in for "Map Type" and also dropped the idea of "Alt+V" so that user doesnt mistake it for "Ctrl+V". Since the custom layers would be bound to "Alt 1-5" , hence Alt makes sense for this toggle as well . Also the help-overlay has been updated to show the information about the shortcut under the appropriate section. https://bugzilla.gnome,org/show_bug.cgi?id=759574
Review of attachment 323773 [details] [review]: Thanks! This patch is based on what? It seems to be based on your previous patch? Always base your patches against master! Do not write anything about custom layers in the commit message. That might change, it might never get commited. Keep the message only relevant to the change in hand. The short log could maybe be something like: "Add keyboard shortcut for toggling map type" ? UX-question: What will happen if you toggle this while having the layers popover open? Will it reflect the correct state? ::: data/ui/help-overlay.ui @@ +82,3 @@ <object class="GtkShortcutsShortcut"> <property name="visible">1</property> + <property name="title" translatable="yes" context="shortcut window">Toggle street/aerial view</property> Toggle Map Type? ::: src/mainWindow.js @@ +182,3 @@ onActivate: this._onMapTypeActivate.bind(this) }, + 'toggle-view': { toggle view seems to generic? toggle-map-type? @@ -407,6 +404,6 @@ - _onMapTypeActivateShortAerial: function(action) { - this._mapView.setMapType(MapView.MapType.AERIAL); - }, ... 3 more ... + _onMapTypeActivateToggleView: function(action) { + let state = action.get_state().get_boolean(); + action.set_state(GLib.Variant.new('b', !state)); ... 3 more ... indent seems off here?
Thanks a lot for the feedback :) I will correct these as soon as possible .
@Jonas : As of now if we toggle this while having the layers-popover open it doesn't reflect the correct state in the layers-popover . How should we approach this ?
(In reply to Kaustav Biswas from comment #19) > @Jonas : As of now if we toggle this while having the layers-popover open it > doesn't reflect the correct state in the layers-popover . How should we > approach this ? Do you have any ideas?
Yeah , I was thinking of binding the key press to the actual click event somehow. Is this feasible ?
(In reply to Kaustav Biswas from comment #21) > Yeah , I was thinking of binding the key press to the actual click event > somehow. > Is this feasible ? I do not think so, maybe looking in to binding properties is a better way? What determines today which layer is selected in the layers-popover? Could we bind a property to that? Could we set some property when the keybinding is activated? Check how the connected property on application is used and bound to other stuff. Look for bind_property in the code. I do not know if this is the best way, but it could be one way. Maybe a boolean property called "streetView"?
Alright I will look into it .
Nautilus uses ctrl+1 and ctrl+2 to swich view modes (list and icon), maybe using those same shortcuts makes sense? (not exactly equivalent, but still…)
(In reply to Marcus Lundblad from comment #24) > Nautilus uses ctrl+1 and ctrl+2 to swich view modes (list and icon), maybe > using those same shortcuts makes sense? (not exactly equivalent, but still…) Seems fine to me, and then 3,4,5,...,n would be custom layers?
(In reply to Jonas Danielsson from comment #25) > (In reply to Marcus Lundblad from comment #24) > > Nautilus uses ctrl+1 and ctrl+2 to swich view modes (list and icon), maybe > > using those same shortcuts makes sense? (not exactly equivalent, but still…) > > Seems fine to me, and then 3,4,5,...,n would be custom layers? Yeah, I think that would make sense.
Created attachment 350028 [details] [review] mainWindow: Add keyboard shortcuts to switch map mode Added shortcuts to switch between street and aerial views. Manually update the state of the layer switcher buttons to avoid recursive signal emissions.
Created attachment 350029 [details] [review] helpOverlay: Add shortcuts to switch map mode
I didn't manage to get separate actions for street and aerial working correctly when set up as action handlers on the layers buttons (got some weird race condition-like effect where it toggled between them in an infinite loop). Also tried refactoring to make MapView.setMapType emit a signal and connect to that in LayersPopover, but in this case got a recursive emission problem (probably because setting the GtkRadioButton.active = true causes the click handler being triggered). So rather I added an entry point in LayersPopover to update the map type selection and have the action in MainWindow taking care of that.
Created attachment 350164 [details] [review] mainWindow,mapView: Add keyboard shortcuts to switch map mode Added shortcuts to switch between street and aerial views. Manually update the state of the layer switcher buttons to avoid recursive signal emissions.
Attachment 350029 [details] pushed as 35e69f0 - helpOverlay: Add shortcuts to switch map mode Attachment 350164 [details] pushed as e890bd9 - mainWindow,mapView: Add keyboard shortcuts to switch map mode