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. > > > 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@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.. I will send v2. Thanks, Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
