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

[Fabiano Fidêncio]

On Sat, Mar 28, 2015 at 1:24 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
> ---
>  src/virt-viewer-window.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> index d668f74..205a09a 100644
> --- a/src/virt-viewer-window.c
> +++ b/src/virt-viewer-window.c
> @@ -67,6 +67,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 gboolean virt_viewer_window_can_zoom_out(VirtViewerWindow *self);
>
>  G_DEFINE_TYPE (VirtViewerWindow, virt_viewer_window, G_TYPE_OBJECT)
>
> @@ -366,14 +368,16 @@ G_MODULE_EXPORT void
>  virt_viewer_window_menu_view_zoom_out(GtkWidget *menu G_GNUC_UNUSED,
>                                        VirtViewerWindow *self)
>  {
> -    virt_viewer_window_set_zoom_level(self, self->priv->zoomlevel - 10);
> +    if (virt_viewer_window_can_zoom_out(self))
> +        virt_viewer_window_set_zoom_level(self, self->priv->zoomlevel - 10);
>  }
>

I'm not sure if I prefer this treatment being done here or inside
_set_zoom_level(), where you can know if you're zooming in or zooming
out as well ... OTOH, I can agree with this here and with some other
treatment being moved from _set_zoom_level() specifically to
_zoom_in/out() functions ...

>  G_MODULE_EXPORT void
>  virt_viewer_window_menu_view_zoom_in(GtkWidget *menu G_GNUC_UNUSED,
>                                       VirtViewerWindow *self)
>  {
> -    virt_viewer_window_set_zoom_level(self, self->priv->zoomlevel + 10);
> +    if (self->priv->zoomlevel < MAX_ZOOM_LEVEL)
> +        virt_viewer_window_set_zoom_level(self, self->priv->zoomlevel + 10);

This check is already done on set_zoom_level() ... no?
if (zoom_level > 400)
    zoom_level = 400;

What, btw, should use MAX_ZOOM_LEVEL instead of 400 ...

Actually, I know it is not related, but on set_zoom_level() I'd like
to see some treatment as (with a better debug message):
if (priv->zoomlevel == zoom_level) {
    g_debug ("Maximum/Minimum zoom level reached");
    return;
}

>  }
>
>  G_MODULE_EXPORT void
> @@ -1467,6 +1471,44 @@ 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;
> +    g_return_if_fail(VIRT_VIEWER_IS_WINDOW(self));
> +    g_return_if_fail(width != NULL);
> +    g_return_if_fail(height != NULL);
> +
> +    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
> +    *height = 50;
> +    *width = req.width;

I would appreciate a comment here explaining that the minimum size is
set to 50x50 (an arbitrary small value) but in the end it will be the
top menu's width x 50

> +}
> +
> +static gboolean
> +virt_viewer_window_can_zoom_out(VirtViewerWindow *self)
> +{
> +    double zoom_level;
> +    guint min_width = 0, min_height = 0, act_width = 0, act_height = 0;
> +
> +    g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(self), FALSE);
> +    if (self->priv->zoomlevel <= MIN_ZOOM_LEVEL || self->priv->display == NULL)
> +        return FALSE;

This treatment is already done on _set_zoom_level() ... maybe we
really should move the whole treatment to that function ...

> +
> +    virt_viewer_window_get_minimal_dimensions(self, &min_width, &min_height);
> +    virt_viewer_display_get_desktop_size(virt_viewer_window_get_display(self), &act_width, &act_height);

What does act stands for? actual? I'd use just width and height here ...

> +    zoom_level = (self->priv->zoomlevel - 10) / 100.0;
> +
> +    return zoom_level * act_width >= min_width && zoom_level *act_height >= min_height;
> +}
> +
>  /*
>   * Local variables:
>   *  c-indent-level: 4
> --
> 2.3.4
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


Let me know your thoughts about moving the _can_zoom_out() and the
other checks you have added to the _set_zoom_level() function ... Or
would it be a problem when starting virt-viewer setting a zoom-level
by the command line?

Best Regards,
-- 
Fabiano Fidêncio