Hi, thanks for the review. > > On Mon, Mar 30, 2015 at 10:17 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 > > --- > > v2: > > - treatment of zoom level moved to _window_set_zoom_level() in > > order to make it work in all cases (ie. with the command line > > option '-z') > > - _get_minimal_zoom_level() instead of _can_zoom_out() > > - more debug logs > > --- > > src/virt-viewer-window.c | 70 > > ++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 68 insertions(+), 2 deletions(-) > > > > diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c > > index 799adce..48bb543 100644 > > --- a/src/virt-viewer-window.c > > +++ b/src/virt-viewer-window.c > > @@ -34,9 +34,11 @@ > > #include <locale.h> > > #include <glib/gprintf.h> > > #include <glib/gi18n.h> > > +#include <math.h> > > > > #include "virt-gtk-compat.h" > > #include "virt-viewer-window.h" > > +#include "virt-viewer-display.h" > > #include "virt-viewer-session.h" > > #include "virt-viewer-app.h" > > #include "virt-viewer-util.h" > > @@ -67,6 +69,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 gint > > virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self); > > > > G_DEFINE_TYPE (VirtViewerWindow, virt_viewer_window, > > G_TYPE_OBJECT) > > > > @@ -1306,7 +1310,7 @@ > > virt_viewer_window_set_display(VirtViewerWindow *self, > > VirtViewerDisplay *displa > > if (display != NULL) { > > priv->display = g_object_ref(display); > > > > - > > virt_viewer_display_set_zoom_level(VIRT_VIEWER_DISPLAY(priv->display), > > priv->zoomlevel); > > + virt_viewer_window_set_zoom_level(self, priv->zoomlevel); > > virt_viewer_display_set_monitor(VIRT_VIEWER_DISPLAY(priv->display), > > priv->fullscreen_monitor); > > virt_viewer_display_set_fullscreen(VIRT_VIEWER_DISPLAY(priv->display), > > priv->fullscreen); > > > > @@ -1410,8 +1414,20 @@ > > virt_viewer_window_set_zoom_level(VirtViewerWindow *self, gint > > zoom_level) > > zoom_level = MIN_ZOOM_LEVEL; > > if (zoom_level > MAX_ZOOM_LEVEL) > > zoom_level = MAX_ZOOM_LEVEL; > > - priv->zoomlevel = zoom_level; > > > > + if (priv->display != NULL) { > > + guint min_zoom = > > virt_viewer_window_get_minimal_zoom_level(self); > > + if (min_zoom > zoom_level) { > > + g_debug("Cannot set zoom level %d, using %d", > > zoom_level, min_zoom); > > + zoom_level = min_zoom; > > + } > > + } > > Hmm. Right below you're checking again if display is NULL and > returning in case it is ... > And before returning 'priv->zoomlevel = zoom_level; ' It is necessary to set priv->zoomlevel. > > + > > + if (priv->zoomlevel == zoom_level) { > > + g_debug("Maximum/Minimum zoom level reached"); > > Actually, this is reached when the zoom is not changed ... > > > + } > > + > > You can simply return here, right? > > > + priv->zoomlevel = zoom_level; > > if (!priv->display) > > return; > > > In this whole function I'd go for something like: > > if (!priv->display) > return; > > if (zoom_level < MIN...) > if (zoom_level > MAX...) > > min_zoom = virt_viewer_window_get_minimal_zoom_level(self); > if (min_zoom > zoom_level) { > g_debug("Cannot set zoom level %d, using %d", zoom_level, > min_zoom); > zoom_level = min_zoom; > } > > if (priv->zoomlevel == zoom_level) { > g_debug("Zoom level not changed, using: %d", priv->zoomlevel) > return; > } > > > > > @@ -1467,6 +1483,56 @@ > > 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; > > + > > + 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 > > + /* minimal dimensions of the window are the maximum of > > dimensions of the top-menu > > + * and minimal dimension of the display > > + */ > > + *height = MAX(MIN_DISPLAY_HEIGHT, req.height); > > + *width = MAX(MIN_DISPLAY_WIDTH, req.width); > > +} > > + > > +/** > > + * virt_viewer_window_get_minimal_zoom_level: > > + * @self: a #VirtViewerWindow > > + * > > + * Calculates the zoom level with respect to the desktop > > dimensions > > + * > > + * Returns: minimal possible zoom level (multiple of ZOOM_STEP) > > + */ > > +static gint > > +virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self) > > +{ > > + guint min_width, min_height, > > + width, height; /* desktop dimensions */ > > + gint zoom; > > + double width_zoom, height_zoom; /* zoom percentage */ > > + > > + g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(self) && > > + self->priv->display != NULL, > > MIN_ZOOM_LEVEL); > > + > > + virt_viewer_window_get_minimal_dimensions(self, &min_width, > > &min_height); > > + > > virt_viewer_display_get_desktop_size(virt_viewer_window_get_display(self), > > &width, &height); > > + > > + width_zoom = (double) min_width / width; > > + height_zoom = (double) min_height / height; > > + zoom = ceil(10 * MAX(width_zoom, height_zoom)); > > Hmm. I didn't understand this part ... > It is just rounding. I will try to use better names and add some comments: static gint virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self) { guint min_width, min_height, width, height; /* desktop dimensions */ gint zoom; double width_ratio, height_ratio; g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(self) && self->priv->display != NULL, MIN_ZOOM_LEVEL); virt_viewer_window_get_minimal_dimensions(self, &min_width, &min_height); virt_viewer_display_get_desktop_size(virt_viewer_window_get_display(self), &width, &height); /* e.g. minimal width = 200, desktop width = 550 => width ratio = 0.36 * which means that the minimal zoom level is 40 (4 * ZOOM_STEP) */ width_ratio = (double) min_width / width; height_ratio = (double) min_height / height; zoom = ceil(10 * MAX(width_ratio, height_ratio)); return MAX(MIN_ZOOM_LEVEL, zoom * ZOOM_STEP); } Thanks, Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
