GNOME Bugzilla – Bug 725352
Add a nice layer popover menu
Last modified: 2014-03-27 07:31:44 UTC
Me and Andreas did some design work in January for the Headerbar (see here: https://raw.github.com/gnome-design-team/gnome-mockups/master/maps/headerbar-and-popup-ideas.png). It would be nice to have an implementation for the new layer menu seen in picture two from the top in maps.
Start working on this now. =)
(In reply to comment #1) > Start working on this now. =) Great!
Created attachment 270657 [details] street maptype image
Created attachment 270658 [details] satellite maptype image
Created attachment 270660 [details] layers icon
Created attachment 270957 [details] [review] Implemented layers popover menu.
Review of attachment 270957 [details] [review]: Great work! You'll need to add some info about the patch on the third line. Look in the history if you're unsure as to what we want there. Don't feel bad about the many comments below btw. Click review to actually check them out and to reply to the comments. Thanks for doing this! ::: src/gnome-maps.data.gresource.xml @@ +15,3 @@ + <file alias="maptype-satellite.png">../data/media/maptype-satellite.png</file> + <file alias="maptype-street.png">../data/media/maptype-street.png</file> + <file alias="layers.svg">../data/media/layers.svg</file> Call this layers-button.svg and put it in ../data/icons/ instead. ::: src/layers-popup.ui @@ +2,3 @@ +<interface> + <!-- interface-requires gtk+ 3.10 --> + <object class="GtkGrid" id="grid-box"> Just call this grid since there's both GtkGrid and GtkBox. "-box" just adds confusion IMO. Not very important though. @@ +34,3 @@ + </child> + <child> + <object class="GtkRadioButton" id="satellite-layer-button"> Let's call this aerial-layer-button (since the action-target is AERIAL too). ::: src/layersPopup.js @@ +27,3 @@ + ICON: 0, + TEXT: 1 +}; What are this for? Is it left overs from a copy-n-paste? If it's not used it should be removed. :) @@ +33,3 @@ + Extends: Gtk.Popover, + + _init: function(relativeTo) { This parameter isn't needed, see below. @@ +39,3 @@ + 'street-layer-image', + 'satellite-layer-button', + 'satellite-layer-image', These two *-image objects aren't used in the code below, can they be removed? @@ +44,3 @@ + this._gridBox = ui.gridBox; + + this.parent({ relative_to: relativeTo, This shouldn't be needed since you do this in mainwindow: this._layersButton.popover = this._layersPopover; When you set that property, the relative_to thing is set automatically. @@ +47,3 @@ + width_request: 200, + no_show_all: true, + visible: true }); If you initialize this to false here the this.hide() below isn't needed. @@ +51,3 @@ + this._streetLayerButton = ui.streetLayerButton; + this._satelliteLayerButton = ui.satelliteLayerButton; + this._satelliteLayerButton.join_group(this._streetLayerButton); Why did you get these warnings when doing the group joining the UI-file? I'd like to have this in the UI-file if possible. @@ +55,3 @@ + this.get_style_context().add_class('maps-popover'); + this.add(this._gridBox); + this.hide(); Shouldn't be needed. See above. ::: src/main-window.ui @@ +72,3 @@ </style> <child> + <object class="GtkImage" id="layers-image"> I think layers-button-image is better. ::: src/mainWindow.js @@ +66,3 @@ + 'search-completion', + 'layers-button']); + this._layersButton = ui.layersButton; I think you can skip the initLayersWidgets method below and just do this instead: let layersPopover = new LayersPopup.LayersPopup(ui.layersButton); ui.layersButton.popover = layersPopover;
(In reply to comment #6) > Created an attachment (id=270957) [details] [review] > Implemented layers popover menu. You want to use 'present rather than past tense': Implemented -> implement. As Mattias pointed out, the commit log needs some love: https://wiki.gnome.org/Git/CommitMessages
Created attachment 270970 [details] [review] Fix bug #725352 - add a new layers popover menu. This patch implement the new layers popover menu. The new module is an extension of Gtk.Popover. The new menu provides a gtk_radio_button for selecting between the aerial and the street view. Fixed all the troubles coming from the review, except one. Setting the radio button properties 'group' in the ui gives a warning. For this reason the radio button grouping is setup in the js file.
Review of attachment 270970 [details] [review]: In this patch an icon is missing.
Created attachment 270971 [details] [review] Fix bug #725352 - add a new layers popover menu. This patch implement the new layers popover menu. The new module is an extension of Gtk.Popover. The new menu provides a gtk_radio_button for selecting between the aerial and the street view. Fixed all the troubles coming from the review, except one. Setting the radio button properties 'group' in the ui gives a warning. For this reason the radio button grouping is setup in the js file. -- Added the missing icon.
Review of attachment 270970 [details] [review]: The commit log doesn't say anything about the change. :( Please follow these guidelines: https://wiki.gnome.org/Git/CommitMessages (there is even a checklist in there now). Oh and for bug referencing, we use the default format of git-bz: Put a link to the bug as the last line with an empty line above it.
Review of attachment 270971 [details] [review]: I commented on the obsolete patch it seems (please mark them as obsolete btw, `git-bz attach -e` makes that super easy). Anyway, same comment applies here.
Comment on attachment 270971 [details] [review] Fix bug #725352 - add a new layers popover menu. This patch implements the new layers popover menu. The new module is an extension of Gtk.Popover. The new menu provides a gtk_radio_button for selecting between the aerial and the street view. There is just a thing to fix. Setting the radio button properties 'group' in the ui gives a warning. For this reason the radio button grouping is setup in the js file.
Review of attachment 270971 [details] [review]: Hi, thanks for the patch! The icons included are all routing icons. I do not see the icons actually referenced (maptype-*.png) in the patch included. Something went wrong? Comments below! ::: src/layersPopup.js @@ +26,3 @@ +const LayersPopup = new Lang.Class({ + Name: 'LayersPopup', + Extends: Gtk.Popover, Maybe LayersPopover is a better name for this class? @@ +44,3 @@ + this._aerialLayerButton = ui.aerialLayerButton; + this._aerialLayerButton.join_group(this._streetLayerButton); + Are we sure this is needed? And can't be done in ui file? Have you checked how it is done in the gtk3+ example? It can be found under the gtk+ checkout dir at: demos/widget-factory/widget-factory.ui ::: src/mainWindow.js @@ +80,3 @@ + + let layersPopover = new LayersPopup.LayersPopup(ui.layersButton); + ui.layersButton.popover = layersPopover; The LayersPopup class does no longer take a button as an argument, right? @@ +121,2 @@ this._searchEntry.connect('changed', this._searchPopup.hide.bind(this._searchPopup)); This line removal should not be included in the patch.
Created attachment 270987 [details] [review] New layers popover menu This patch implements the new layers popover menu. The new module is an extension of Gtk.Popover. Inside the menu there is a radio button group that allows the user to select a view (aerial or street). Note that setting the radio button properties 'group' in the ui file gives a warning and for this reason the radio button grouping is coded in the js file.
Review of attachment 270987 [details] [review]: Thanks! My comments from the last patch attachments are still valid on this one!
(In reply to comment #15) > Review of attachment 270971 [details] [review]: > > Hi, thanks for the patch! > > The icons included are all routing icons. I do not see the icons actually > referenced (maptype-*.png) in the patch included. Something went wrong? For now the routing icons are not needed the routing is not complete. I just used layers-button.svg and the maptypes. If you consider not right to put them in this patch, I could remove them. > Comments below! > > ::: src/layersPopup.js > @@ +26,3 @@ > +const LayersPopup = new Lang.Class({ > + Name: 'LayersPopup', > + Extends: Gtk.Popover, > > Maybe LayersPopover is a better name for this class? Fixed in the next patch. > @@ +44,3 @@ > + this._aerialLayerButton = ui.aerialLayerButton; > + this._aerialLayerButton.join_group(this._streetLayerButton); > + > > Are we sure this is needed? And can't be done in ui file? Have you checked how > it is done in the gtk3+ example? It can be found under the gtk+ checkout dir > at: demos/widget-factory/widget-factory.ui If I don't do in this way the radio button continue to work but I have this warning: (gnome-maps:5350): Gtk-CRITICAL **: gtk_radio_button_set_group: assertion '!g_slist_find (group, radio_button)' failed > ::: src/mainWindow.js > @@ +80,3 @@ > + > + let layersPopover = new LayersPopup.LayersPopup(ui.layersButton); > + ui.layersButton.popover = layersPopover; > > The LayersPopup class does no longer take a button as an argument, right? Fixed in the next patch. > @@ +121,2 @@ > this._searchEntry.connect('changed', > > this._searchPopup.hide.bind(this._searchPopup)); > > This line removal should not be included in the patch. Sorry :-)
(In reply to comment #18) > > (In reply to comment #15) > > Review of attachment 270971 [details] [review] [details]: > > > > Hi, thanks for the patch! > > > > The icons included are all routing icons. I do not see the icons actually > > referenced (maptype-*.png) in the patch included. Something went wrong? > > For now the routing icons are not needed the routing is not complete. > I just used layers-button.svg and the maptypes. If you consider not right to > put them in this patch, I could remove them. > > Yes please, the routing icons should not be added in this patch. And also make sure that the maptype icons are included they were not included in the last patch. > > Comments below! > > > > ::: src/layersPopup.js > > @@ +26,3 @@ > > +const LayersPopup = new Lang.Class({ > > + Name: 'LayersPopup', > > + Extends: Gtk.Popover, > > > > Maybe LayersPopover is a better name for this class? > > Fixed in the next patch. > > > @@ +44,3 @@ > > + this._aerialLayerButton = ui.aerialLayerButton; > > + this._aerialLayerButton.join_group(this._streetLayerButton); > > + > > > > Are we sure this is needed? And can't be done in ui file? Have you checked how > > it is done in the gtk3+ example? It can be found under the gtk+ checkout dir > > at: demos/widget-factory/widget-factory.ui > > If I don't do in this way the radio button continue to work but I have this > warning: > > (gnome-maps:5350): Gtk-CRITICAL **: gtk_radio_button_set_group: assertion > '!g_slist_find (group, radio_button)' failed > > > > ::: src/mainWindow.js > > @@ +80,3 @@ > > + > > + let layersPopover = new LayersPopup.LayersPopup(ui.layersButton); > > + ui.layersButton.popover = layersPopover; > > > > The LayersPopup class does no longer take a button as an argument, right? > > Fixed in the next patch. > > > @@ +121,2 @@ > > this._searchEntry.connect('changed', > > > > this._searchPopup.hide.bind(this._searchPopup)); > > > > This line removal should not be included in the patch. > > Sorry :-) Also again, make sure that the patch applies cleanly to master and that all files that should be included are included. Thanks!
Created attachment 271004 [details] [review] New layers popover menu This patch implements the new layers popover menu. The new module is an extension of Gtk.Popover. Inside the menu there is a radio button group that allows the user to select a view (aerial or street). Note that setting the radio button properties 'group' in the ui file gives a warning and for this reason the radio button grouping is coded in the js file.
Review of attachment 271004 [details] [review]: Thanks! I can now apply the patch using git bz apply. I would like some desinger, Andreas maybe?, to look at how this look before we commit this. The arrow pointing thing is not in the center of the box for instance, could that be fixed? Should it be fixed? Some additional comments below about whitespace damage. It might be wise to see if your editor has some way to show whitespace. ::: src/layersPopover.js @@ +27,3 @@ + Name: 'LayersPopover', + Extends: Gtk.Popover, + Some whitespace damage here. @@ +34,3 @@ + 'aerial-layer-button', + ]); + And here as well. @@ +40,3 @@ + no_show_all: true, + visible: false }); + And whitespace damage here too. ::: src/mainWindow.js @@ +78,2 @@ this._contextMenu = new ContextMenu.ContextMenu(this.mapView); + Whitespace damage here.
Created attachment 271078 [details] visual mockup > I would like some desinger, Andreas maybe?, to look at how this look before we > commit this. The arrow pointing thing is not in the center of the box for > instance, could that be fixed? Should it be fixed? I think it's a very good start! Visually, I envisioned something like the attached mockup. So no button appearance, and just a border and then blue border to indicate the selected map type.
Created attachment 271416 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street).
Review of attachment 271416 [details] [review]: Thanks, looks great. Is there anyway of making this keyboard accessible? So that one could use the keyboard to choose the layer style. Hopefully Andreas or Mattias can comment on how this looks ui-wise. I have some nit comments below. ::: src/layers-popover.ui @@ +13,3 @@ + <property name="action-target">"STREET"</property> + <property name="draw_indicator">False</property> + <property name="receives_default">True</property> You mix using '-' and '_' to separate the names of the properties, might want to stick to one. Also: why does it need receives_default? @@ +39,3 @@ + <property name="action-target">"AERIAL"</property> + <property name="draw_indicator">False</property> + <property name="receives_default">True</property> Same question about default here. ::: src/layersPopover.js @@ +29,3 @@ + + _init: function() { + This blank line should be removed. @@ +39,3 @@ + visible: false }); + + ui.aerialLayerButton.join_group(ui.streetLayerButton); This still bugs me :) But I don't know how to fix it. The funny thing is that when you run the gtk3-widget-factory demo, found in the demo directory of gtk+ you don't get this warning. But I can't see what the difference between your ui-file and the gtk one is. Would like to fix it, but can live with having it in js-code as well if a fix is not easily found. Maybe a bug should be placed towards gtk+. @@ +43,3 @@ + ui.streetLayerButton.get_style_context().add_class('layer-radio-button'); + ui.aerialLayerButton.get_style_context().add_class('layer-radio-button'); + These can be set from the ui-file. <class name="layer-radio-button"/> same way you set the "image-button" class.
Created attachment 271426 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street). The keyboard navigation is still not implemented. Waiting for comments :-)
Review of attachment 271426 [details] [review]: Thanks! Some more nits below :) ::: src/layersPopover.js @@ +32,3 @@ + 'street-layer-button', + 'aerial-layer-button', + ]); These does not appear to be aligned properly. ::: src/mainWindow.js @@ +70,3 @@ + let layersPopover = new LayersPopover.LayersPopover(); + ui.layersButton.popover = layersPopover; Thinking about it, we don't really need the layersPopover temp variable here, right? We could go ui.layersButton.popover = new LayersPopover.LayersPopover();
Review of attachment 271426 [details] [review]: Also, the commit message does not follow guidelines atm, the body has too long line, the limit is 74 iirc.
Created attachment 271427 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street). The keyboard navigation is still not implemented. Waiting for comments :-)
Review of attachment 271427 [details] [review]: Thank you! What comments are you waiting for regarding keyboard accessibility? ::: src/layersPopover.js @@ +31,3 @@ + let ui = Utils.getUIObject('layers-popover', ['grid', + 'street-layer-button', + 'aerial-layer-button', Still not aligned :)
Review of attachment 271427 [details] [review]: Waiting for comments by Andreas or Mattias.
Created attachment 271431 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street).
Created attachment 271449 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street).
Jonas the menu now is keyboard accessible. :-)
Review of attachment 271449 [details] [review]: Great, thanks! It's a bit hard to see the focus thingie, but it might be ok. Andreas, Mattias, what do you say is the UI ok to go (including the focus ring while using the kb)?
(In reply to comment #34) > Review of attachment 271449 [details] [review]: > Andreas, Mattias, what do you say is the UI ok to go (including the focus ring > while using the kb)? I'm all right with the focus look of things as it seems it's the only way to indicate keyboard accessibility.
I don't have time to test this until tomorrow evening, but I trust Andreas' judgment. :)
Review of attachment 271449 [details] [review]: I still want Andreas or Mattias to give a final "this UI is good to go". But it's no rush. GNOME is in ui freeze so this will not be commited until after 3.12. I found some more nits. When they are cleared up and I get an ok for the UI I will mark this as commit_after_freeze and you can reference that in your application. ::: data/gnome-maps.css @@ +28,3 @@ } +.layer-radio-button{ Insert a space before the curly bracket. @@ +39,3 @@ +} + +.layer-radio-button:active{ Insert a space before the curly bracket. @@ +42,3 @@ + border: 2px solid @suggested_action_button_a; +} + This lands in between different styling for the zoom-control, maybe it could be moved above the zoom related css? ::: src/layersPopover.js @@ +31,3 @@ + let ui = Utils.getUIObject('layers-popover', [ 'grid', + 'street-layer-button', + 'aerial-layer-button']); Needs a space before the end bracket.
Review of attachment 271449 [details] [review]: It's a trade off if the focus is more visible than we have more space between the border and the image.
Review of attachment 271449 [details] [review]: Going to have these small changes right now and upload soon the new patch.
Created attachment 272001 [details] [review] New layers popover menu Replace the map-type-menu with a new popover menu for switching between views (aerial and street).
Review of attachment 272001 [details] [review]: Thanks a lot Dario! I will mark this with commit_after_freeze and it will be committed to master after the 3.12 release is over.
Attachment 272001 [details] pushed as 93710ef - New layers popover menu