GNOME Bugzilla – Bug 753626
Add minimize button to the fullscreen toolbar
Last modified: 2015-11-18 17:38:34 UTC
Created attachment 309266 [details] [review] Add minimize button to the fullscreen toolbar It would be useful to have a button which minimizes Vinagre in the fullscreen toolbar. This feature was already in Vinagre before (see https://bugzilla.gnome.org/show_bug.cgi?id=558596) but has been removed (see https://git.gnome.org/browse/vinagre/commit/?id=28307995dc56ca82e6e8e1ef512d6668f51cc2cb). Attached patch implements it.
Review of attachment 309266 [details] [review]: ::: configure.ac @@ +35,3 @@ GTK_VNC_DEPS="gtk-vnc-2.0 >= 0.4.3" XML2_DEPS="libxml-2.0 >= 2.6.31" +ICON_THEME_DEPS="adwaita-icon-theme >= 3.13.1" With recent versions of GTK+, a new enough icon theme should already be installed, so is an explicit icon theme dependency necessary? ::: vinagre/vinagre-tab.c @@ +366,3 @@ + /* Minimize window */ + button = GTK_WIDGET (gtk_tool_button_new (NULL, NULL)); + gtk_tool_button_set_icon_name (GTK_TOOL_BUTTON (button), "window-minimize-symbolic"); Should this be a symbolic icon? The other icons are not symbolic, so this should probably match. Having said that, it seems that the only window-minimize icon is the symbolic variant. Does just using "window-minimize" succeed with the symoblic variant?
(In reply to David King from comment #1) > Review of attachment 309266 [details] [review] [review]: > > ::: configure.ac > @@ +35,3 @@ > GTK_VNC_DEPS="gtk-vnc-2.0 >= 0.4.3" > XML2_DEPS="libxml-2.0 >= 2.6.31" > +ICON_THEME_DEPS="adwaita-icon-theme >= 3.13.1" > > With recent versions of GTK+, a new enough icon theme should already be > installed, so is an explicit icon theme dependency necessary? Probably you are right I just wanted to be sure that the theme is installed. > ::: vinagre/vinagre-tab.c > @@ +366,3 @@ > + /* Minimize window */ > + button = GTK_WIDGET (gtk_tool_button_new (NULL, NULL)); > + gtk_tool_button_set_icon_name (GTK_TOOL_BUTTON (button), > "window-minimize-symbolic"); > > Should this be a symbolic icon? The other icons are not symbolic, so this > should probably match. Having said that, it seems that the only > window-minimize icon is the symbolic variant. Does just using > "window-minimize" succeed with the symoblic variant? I've tried to use just the "window-minimize" and it doesn't work with standard theme. There is "window-minimize" icon in High Contrast theme but it was not shown.
(In reply to Marek Kašík from comment #2) > (In reply to David King from comment #1) > > With recent versions of GTK+, a new enough icon theme should already be > > installed, so is an explicit icon theme dependency necessary? > > Probably you are right I just wanted to be sure that the theme is installed. I think this can be safely ignored. It would have to be a runtime check anyway (and GTK+ will provide a suitable "icon missing" image if the icon theme is not installed). > > Should this be a symbolic icon? The other icons are not symbolic, so this > > should probably match. Having said that, it seems that the only > > window-minimize icon is the symbolic variant. Does just using > > "window-minimize" succeed with the symoblic variant? > > I've tried to use just the "window-minimize" and it doesn't work with > standard theme. There is "window-minimize" icon in High Contrast theme but > it was not shown. Thanks! The symbolic variant should be fine then.
Created attachment 310986 [details] [review] Add minimize button to the fullscreen toolbar I've removed the requirements for the icon theme. Btw, shouldn't we try to move the icons for minimization and for disconnection to the right side so they are last icons in the toolbar?
Review of attachment 310986 [details] [review]: Looks good, this can get merged as soon as the freeze is over. I'm not too fussed about the arrangement of the buttons inside the toolbar.
Thank you for the reviews. I've pushed the patch to master.