[virt-tools-list] [PATCH virt-viewer] virt-viewer-window: Change zoom of the display only when it is possible
Pavel Grunt
pgrunt at redhat.com
Sat Mar 28 17:24:56 UTC 2015
Hi, thanks for the review!
>
> 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 ...
The main difference is that functions virt_viewer_window_menu_view_zoom_out/in()
are only invoked by the user. I wanted to avoid calling virt_viewer_display_set_zoom_level()
and virt_viewer_window_queue_resize() when it is not necessary.
>
> > 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?
Probably it is not necessary, it is just avoiding _queue_resize()
> if (zoom_level > 400)
> zoom_level = 400;
>
> What, btw, should use MAX_ZOOM_LEVEL instead of 400 ...
>
Looking at the code there is more zoom related magic constants - maybe I should also introduce something like ZOOM_STEP, NORMAL_ZOOM_LEVEL ?
>
> 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;
> }
>
Sure
>
> > }
> >
> > 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
>
Sure, or also remove magical constants ?
>
> > +}
> > +
> > +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 ...
>
ok, I was thinking like giving the "complete" answer whether is possible to zoom out.
>
> > +
> > + 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
yes
> ...
>
> > + 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?
>
I wanted to avoid extra display_queue_resize(). And I was afraid of "the command line"
It will be possible to set the small zoom level from command line but not really setting it
because we return from window_set_zoom_level(). I have to try it..
I will send v2.
Thanks,
Pavel
More information about the virt-tools-list
mailing list