On Sat, Mar 28, 2015 at 6:24 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote: > Hi, thanks for the review! > >> >> On Sat, Mar 28, 2015 at 1:24 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> >> 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@xxxxxxxxxx >> > 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
