[virt-tools-list] [PATCH virt-viewer] virt-viewer-window: Change zoom of the display only when it is possible
Fabiano Fidêncio
fabiano at fidencio.org
Sun Mar 29 01:16:52 UTC 2015
On Sat, Mar 28, 2015 at 6:24 PM, Pavel Grunt <pgrunt at redhat.com> wrote:
> 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.
The main problem with your approach is that it won't work when passing
the zoom-level from command line.
Having a window as 250x250 and opening the virt-viewer passing --zoom
10 out trigger the queue_resize you would like to avoid.
Returning earlier in _set_zoom_level() seem the way to avoid it.
>
>>
>> > 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 ?
I would go for that, instead of cryptic numbers all around the code :-)
>
>>
>> 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 ?
>>
Both, if possible.
>> > +}
>> > +
>> > +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.
Not sure if I understand what you meant bu "because we return from
_set_zoom_level().
>
> I will send v2.
Looking forward for a v2.
>
> Thanks,
>
> Pavel
Best Regards,
--
Fabiano Fidêncio
More information about the virt-tools-list
mailing list