GNOME Bugzilla – Bug 725851
Improve the zoom control
Last modified: 2014-10-10 18:19:18 UTC
The zoom-control we have now was created with transparency in mind. Since we are not able to have transparency over a Clutter surface we might want to rethink it. Also at the moment it does not have a "pressed" state and does not behave much as buttons. A new design and a improved implementation might be in order.
I think it makes sense to just move the zoom control back to the clutter canvas again actually. We still want the overlay for popovers and everything, but markers, location-bubbles and the zoom control probably can live in the mapview. What do you think?
*** Bug 706296 has been marked as a duplicate of this bug. ***
I'm not sure that transparency for the zoom control is needed or even pretty. I would like a nice zoom control for 3.14. Mockups requested!
Created attachment 287655 [details] [review] improve the zoom-control
Review of attachment 287655 [details] [review]: Hi! Thanks for the patch! I think that if we are going the route of using standard buttons with the "linked" class then we do not want any css or images. So I think you should remove all css that applies to these buttons and all the images as well. And instead set the label of the buttons to +/-. Maybe even the unicode ones? Please also remove the image files and the reference to them in all resource files and make files.
Created attachment 287691 [details] [review] improve the zoom-control
Review of attachment 287691 [details] [review]: Thanks! You removed the images, great! But you forgot to remove them from src/gnome-maps.data.gresource.xml, this gave me a build-error. Please remove them from there in the next revision. I think this can be pretty. But it needs some more work. I think the +/- signs need to be bigger. And possibly bold. And the buttons look kind of bloated to me. Maybe you can remove the width/height requests? And see if you can make the buttons look good. Also when you are in the ui file, could you convert it to using spaces only as indentation? Thanks!
screen shot of changed zoom control http://ctrlv.in/443999
I agree that the buttons look very fat. I would suggest making them equally high as wide. There is also some fuzzy antialias going on around the borders. Might need to make sure it matches the pixel grid by being positioned properly.
Yeah, I'd say make the buttons as big as the headerbar buttons (style class "imagebutton"), and make them be horizontally aligned with the leftmost button of the headerbar.
You might need to play with the halign/valign properties of the GtkButton, and also maybe the packing properties, to sett "fill" to false. Try going ctrl-shift-i when maps is open, you will get the GtkInspector and can inspect the different properties and child properties of the widgets. And see what happens when you change them. http://blogs.gnome.org/mclasen/2014/05/15/introducing-gtkinspector/
screenshot of improved zoom control with height and width of zoom in and zoom out button same as that of headerbar buttons (34 X 34) http://ctrlv.in/445292
Looks nice! Maybe it also should be aligned with the button in the header bar? The placement of the zoom control is controlled by the values in zoomControl.js, the halign/valing sent to this.parent. Please post the patch as well next time?
Sorry, I mean the margin_top and the margin_start.
(In reply to comment #12) > screenshot of improved zoom control with height and width of zoom in and zoom > out button same as that of headerbar buttons (34 X 34) > > http://ctrlv.in/445292 That looks a lot better. Thanks!
Created attachment 288157 [details] [review] improve the zoom-control
Review of attachment 288157 [details] [review]: Thanks! It looks nice! Have some comments and questions below. Also, could you also do a patch that converts the zoom-control.ui to just use spaces for indentation? And have that patch come before this one? ::: src/zoom-control.ui @@ +21,3 @@ + <property name="can_focus">False</property> + <property name="margin">0</property> + <property name="label" translatable="yes">_+</property> This does not need to be translatable. Why does this have an underscore before the '+'? @@ +22,3 @@ + <property name="margin">0</property> + <property name="label" translatable="yes">_+</property> + <property name="use_underline">True</property> Why to we need to use underline? @@ +39,3 @@ + <property name="can_focus">False</property> + <property name="margin">0</property> + <property name="label" translatable="yes">_-</property> This does not need to be translatab.e ::: src/zoomControl.js @@ +32,3 @@ this.parent({ halign: Gtk.Align.START, valign: Gtk.Align.START, margin_top: 20, I think I might prefer 6 on both of these, Andreas?
Created attachment 288185 [details] [review] using only space for indentation
Review of attachment 288185 [details] [review]: Thanks! Awesome. So the topic of this will be the commit shortlog. I think we can do better! How about: "zoom-control: Use space for indentation" ? Also, could you change the indentation to use two spaces only? Like the other ui files?
Created attachment 288186 [details] [review] Use space for indentation
Review of attachment 288186 [details] [review]: Awesome!
Created attachment 288187 [details] [review] improve the zoom-control
Created attachment 288188 [details] [review] improve the zoom-control
This doesn't really apply for me, is there some kind of rebase mishap?
Created attachment 288222 [details] [review] improve the zoom-control
committed with modifications @288222 - improve the zoom-control - none
Is the patch getting applied now?