[virt-tools-list] [PATCH virt-viewer 4/4 v2] virt-viewer-window: Change zoom of the display only when it is possible

[Pavel Grunt]

Hi, thanks for the review.

>
> On Mon, Mar 30, 2015 at 10:17 PM, Pavel Grunt <pgrunt at redhat.com>
> wrote:
> > Do not allow to zoom out if it is not possible due to the width of
> > the top menu. It avoids emitting size allocation events that will
> > change the display resolution of the spice guest.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1206460
> > ---
> > v2:
> >  - treatment of zoom level moved to _window_set_zoom_level() in
> >  order to make it work in all cases (ie. with the command line
> >  option '-z')
> >  - _get_minimal_zoom_level() instead of _can_zoom_out()
> >  - more debug logs
> > ---
> >  src/virt-viewer-window.c | 70
> >  ++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> > index 799adce..48bb543 100644
> > --- a/src/virt-viewer-window.c
> > +++ b/src/virt-viewer-window.c
> > @@ -34,9 +34,11 @@
> >  #include <locale.h>
> >  #include <glib/gprintf.h>
> >  #include <glib/gi18n.h>
> > +#include <math.h>
> >
> >  #include "virt-gtk-compat.h"
> >  #include "virt-viewer-window.h"
> > +#include "virt-viewer-display.h"
> >  #include "virt-viewer-session.h"
> >  #include "virt-viewer-app.h"
> >  #include "virt-viewer-util.h"
> > @@ -67,6 +69,8 @@ static void
> > virt_viewer_window_disable_modifiers(VirtViewerWindow *self);
> >  static void virt_viewer_window_resize(VirtViewerWindow *self,
> >  gboolean keep_win_size);
> >  static void virt_viewer_window_toolbar_setup(VirtViewerWindow
> >  *self);
> >  static GtkMenu*
> >  virt_viewer_window_get_keycombo_menu(VirtViewerWindow *self);
> > +static void
> > virt_viewer_window_get_minimal_dimensions(VirtViewerWindow *self,
> > guint *width, guint *height);
> > +static gint
> > virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self);
> >
> >  G_DEFINE_TYPE (VirtViewerWindow, virt_viewer_window,
> >  G_TYPE_OBJECT)
> >
> > @@ -1306,7 +1310,7 @@
> > virt_viewer_window_set_display(VirtViewerWindow *self,
> > VirtViewerDisplay *displa
> >      if (display != NULL) {
> >          priv->display = g_object_ref(display);
> >
> > -
> >        virt_viewer_display_set_zoom_level(VIRT_VIEWER_DISPLAY(priv->display),
> > priv->zoomlevel);
> > +        virt_viewer_window_set_zoom_level(self, priv->zoomlevel);
> >          virt_viewer_display_set_monitor(VIRT_VIEWER_DISPLAY(priv->display),
> >          priv->fullscreen_monitor);
> >          virt_viewer_display_set_fullscreen(VIRT_VIEWER_DISPLAY(priv->display),
> >          priv->fullscreen);
> >
> > @@ -1410,8 +1414,20 @@
> > virt_viewer_window_set_zoom_level(VirtViewerWindow *self, gint
> > zoom_level)
> >          zoom_level = MIN_ZOOM_LEVEL;
> >      if (zoom_level > MAX_ZOOM_LEVEL)
> >          zoom_level = MAX_ZOOM_LEVEL;
> > -    priv->zoomlevel = zoom_level;
> >
> > +    if (priv->display != NULL) {
> > +        guint min_zoom =
> > virt_viewer_window_get_minimal_zoom_level(self);
> > +        if (min_zoom > zoom_level) {
> > +            g_debug("Cannot set zoom level %d, using %d",
> > zoom_level, min_zoom);
> > +            zoom_level = min_zoom;
> > +        }
> > +    }
> 
> Hmm. Right below you're checking again if display is NULL and
> returning in case it is ...
> 

And before returning 'priv->zoomlevel = zoom_level; ' It is necessary to set priv->zoomlevel.

> > +
> > +    if (priv->zoomlevel == zoom_level) {
> > +        g_debug("Maximum/Minimum zoom level reached");
> 
> Actually, this is reached when the zoom is not changed ...
> 
> > +    }
> > +
> 
> You can simply return here, right?
> 
> > +    priv->zoomlevel = zoom_level;
> >      if (!priv->display)
> >          return;
> 
> 
> In this whole function I'd go for something like:
> 
> if (!priv->display)
>     return;
> 
> if (zoom_level < MIN...)
> if (zoom_level > MAX...)
> 
> min_zoom = virt_viewer_window_get_minimal_zoom_level(self);
> if (min_zoom > zoom_level) {
>     g_debug("Cannot set zoom level %d, using %d", zoom_level,
>     min_zoom);
>     zoom_level = min_zoom;
> }
> 
> if (priv->zoomlevel == zoom_level) {
>     g_debug("Zoom level not changed, using: %d", priv->zoomlevel)
>     return;
> }
> 
> >
> > @@ -1467,6 +1483,56 @@
> > virt_viewer_window_set_kiosk(VirtViewerWindow *self, gboolean
> > enabled)
> >          g_debug("disabling kiosk not implemented yet");
> >  }
> >
> > +static void
> > +virt_viewer_window_get_minimal_dimensions(VirtViewerWindow *self,
> > +                                          guint *width,
> > +                                          guint *height)
> > +{
> > +    GtkRequisition req;
> > +    GtkWidget *top_menu;
> > +
> > +    top_menu =
> > GTK_WIDGET(gtk_builder_get_object(virt_viewer_window_get_builder(self),
> > "top-menu"));
> > +#if !GTK_CHECK_VERSION(3, 0, 0)
> > +    gtk_widget_get_child_requisition(top_menu, &req);
> > +#else
> > +    gtk_widget_get_preferred_size(top_menu, &req, NULL);
> > +#endif
> > +    /* minimal dimensions of the window are the maximum of
> > dimensions of the top-menu
> > +     * and minimal dimension of the display
> > +     */
> > +    *height = MAX(MIN_DISPLAY_HEIGHT, req.height);
> > +    *width = MAX(MIN_DISPLAY_WIDTH, req.width);
> > +}
> > +
> > +/**
> > + * virt_viewer_window_get_minimal_zoom_level:
> > + * @self: a #VirtViewerWindow
> > + *
> > + * Calculates the zoom level with respect to the desktop
> > dimensions
> > + *
> > + * Returns: minimal possible zoom level (multiple of ZOOM_STEP)
> > + */
> > +static gint
> > +virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self)
> > +{
> > +    guint min_width, min_height,
> > +          width, height; /* desktop dimensions */
> > +    gint zoom;
> > +    double width_zoom, height_zoom; /* zoom percentage */
> > +
> > +    g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(self) &&
> > +                         self->priv->display != NULL,
> > MIN_ZOOM_LEVEL);
> > +
> > +    virt_viewer_window_get_minimal_dimensions(self, &min_width,
> > &min_height);
> > +
> >    virt_viewer_display_get_desktop_size(virt_viewer_window_get_display(self),
> > &width, &height);
> > +
> > +    width_zoom = (double) min_width / width;
> > +    height_zoom = (double) min_height / height;
> > +    zoom = ceil(10 * MAX(width_zoom, height_zoom));
> 
> Hmm. I didn't understand this part ...
> 
It is just rounding. I will try to use better names and add some comments:

static gint
virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self)
{
    guint min_width, min_height,
          width, height; /* desktop dimensions */
    gint zoom;
    double width_ratio, height_ratio;

    g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(self) &&
                         self->priv->display != NULL, MIN_ZOOM_LEVEL);

    virt_viewer_window_get_minimal_dimensions(self, &min_width, &min_height);
    virt_viewer_display_get_desktop_size(virt_viewer_window_get_display(self), &width, &height);

    /* e.g. minimal width = 200, desktop width = 550 => width ratio = 0.36
     * which means that the minimal zoom level is 40 (4 * ZOOM_STEP)
     */
    width_ratio = (double) min_width / width;
    height_ratio = (double) min_height / height;
    zoom = ceil(10 * MAX(width_ratio, height_ratio));

    return MAX(MIN_ZOOM_LEVEL, zoom * ZOOM_STEP);
}

Thanks,

Pavel