GNOME Bugzilla – Bug 708000
Align better the buttons in the headerbar
Last modified: 2013-09-28 19:50:47 UTC
The buttons need to be same height, also a buttons with text. See the patches.
Created attachment 254826 [details] [review] shell: Make the lock button align better
Created attachment 254827 [details] [review] shell: Align better the buttons in the headerbar
do you have before/after screenshots showing the problem and its fix ?
Created attachment 254831 [details] Screenshot - before
Created attachment 254832 [details] Screenshot - after Note: The difference is very small.
I think that both patches should be merged into one, and the sizegroup reused.
Comment on attachment 254826 [details] [review] shell: Make the lock button align better Marking as needs-work as per comment
Comment on attachment 254827 [details] [review] shell: Align better the buttons in the headerbar Marking as needs-work as per comment
Created attachment 255088 [details] [review] shell: Align better the buttons in the headerbar
Created attachment 255089 [details] [review] mouse: Make the test button align better
Review of attachment 255088 [details] [review]: In addition to my nitpicks down below, rtcm pointed out on IRC that the patch isn't including the close button in the size group. ::: shell/cc-window.c @@ +89,3 @@ GQueue *previous_panels; + GtkSizeGroup *size; 'size' is a bit ambiguous, might be better to call the variable 'header_sizegroup' or similar. @@ +955,3 @@ + + gtk_size_group_add_widget (GTK_SIZE_GROUP (priv->size), priv->previous_button); + gtk_size_group_add_widget (GTK_SIZE_GROUP (priv->size), widget); Is priv->previous_button supposed to get added to the size group here, in _shell_embed_widget_in_header() ? I would move it below to create_header() where the previous_button is created and where you add the rest of the widgets to the size group. Also, the typecasts are unnecessary here -- the variable is already a GtkSizeGroup * @@ +1509,3 @@ + + gtk_size_group_add_widget (GTK_SIZE_GROUP (priv->size), priv->search_button); + gtk_size_group_add_widget (GTK_SIZE_GROUP (priv->size), priv->lock_button); Unnecessary typecasts.
Created attachment 255319 [details] [review] shell: Align better the buttons in the headerbar
Created attachment 255320 [details] [review] shell: Align better the buttons in the headerbar
Review of attachment 255320 [details] [review]: Looks good to me now, thanks. I wonder if headerbar should do this automatically. Seems like a common use case to ensure all children are of the same height. A "homogeneous" property maybe, for GTK+ 3.12?
(In reply to comment #14) > Review of attachment 255320 [details] [review]: > > Looks good to me now, thanks. > > I wonder if headerbar should do this automatically. Seems like a common use > case to ensure all children are of the same height. A "homogeneous" property > maybe, for GTK+ 3.12? Can you please file a bug about it?
Review of attachment 255320 [details] [review]: pushed as 11a074525ce4efae3daa55111f07421851a9dc89 - shell: Align better the buttons in the headerbar
Review of attachment 255089 [details] [review]: Looks good to me, just one minor issue below. Also, would be nice to add an additional paragraph to the commit message saying that this patch removes unwanted 2 pixels of spacing around the button. ::: panels/mouse/cc-mouse-panel.c @@ -108,3 @@ - gtk_widget_set_visible (box, TRUE); - - priv->shell_header = g_object_ref (box); Please remove priv->shell_header now that it's unused.
Created attachment 255989 [details] [review] mouse: Make the test button align better
Review of attachment 255989 [details] [review]: Looks good to me now, thanks!
Review of attachment 255989 [details] [review]: pushed as 50488b1ea07be40bbcefa1663fe4e3bd86ac180f - mouse: Make the test button align better