Bug 54277

Summary: Change scaling behaviour in spice-gtk
Product: Spice Reporter: Alexander Larsson <alexl>
Component: spice-gtkAssignee: Spice Bug List <spice-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Centralize scaling handling
Add only_downscale property
Fix the flickering
Fix build with gtk2
Fix leak

Description Alexander Larsson 2012-08-30 17:16:19 UTC
I want to have a new scaling option where spice-gtk never scales up a display, as that kind of magnification is rarely what you want. However, it is very useful to scale down when a window is to small (so that you can see the entire vm in a smaller window).

For instance, you might have the vm be in the same resolution as your monitor so that fullscreen is 1:1, but then when you go out of fullscreen you see everything scaled down.

Also, I don't think it ever makes sense to scale a display without keeping the aspect ratio, that just makes things look weird.

Attaching a patch to implement this.
Comment 1 Alexander Larsson 2012-08-30 17:42:18 UTC
Created attachment 66355 [details] [review]
Centralize scaling handling
Comment 2 Alexander Larsson 2012-08-30 17:42:43 UTC
Created attachment 66356 [details] [review]
Add only_downscale property
Comment 3 Marc-Andre Lureau 2012-08-30 19:24:56 UTC
Comment on attachment 66356 [details] [review]
Add only_downscale property

Review of attachment 66356 [details] [review]:
-----------------------------------------------------------------

looks good

::: gtk/spice-widget.c
@@ +1639,5 @@
> +                              G_PARAM_READWRITE |
> +                              G_PARAM_CONSTRUCT |
> +                              G_PARAM_STATIC_STRINGS));
> +
> +    g_object_class_install_property

it would be nice to add the same doc blurb, with a "Since: 0.14"
Comment 4 Marc-Andre Lureau 2012-08-30 19:27:41 UTC
Comment on attachment 66355 [details] [review]
Centralize scaling handling

Review of attachment 66355 [details] [review]:
-----------------------------------------------------------------

nice clean-up, and seems to work fine in all situations (virt-viewer zooming, MonitorsConfig area, server mouse-mode etc..)! thanks a bunch

I guess we need a new stable release for GNOME Boxes with those patches.
Comment 5 Alexander Larsson 2012-08-30 20:09:09 UTC
pushed.
Comment 6 Alexander Larsson 2012-08-31 07:51:32 UTC
For some reason the background clearing is flickering on one of my machines after this, reopening.
Comment 7 Alexander Larsson 2012-08-31 07:52:19 UTC
Created attachment 66387 [details] [review]
Fix the flickering
Comment 8 Marc-Andre Lureau 2012-08-31 08:21:57 UTC
(In reply to comment #7)
> Created attachment 66387 [details] [review] [review]
> Fix the flickering

I don't have this flickering. The patch is apparently harmless for me, but I don't understand why it would work better. ack if it fixes your issue.
Comment 9 Marc-Andre Lureau 2012-08-31 09:01:58 UTC
sadly, it broke the build on mingw/gtk2

spice-widget-cairo.c: In function 'spicex_draw_event':
spice-widget-cairo.c:109:5: error: passing argument 2 of 'gdk_cairo_region' from incompatible pointer type [-Werror]
In file included from /usr/i686-w64-mingw32/sys-root/mingw/include/gtk-2.0/gdk/gdk.h:33:0,
                 from /usr/i686-w64-mingw32/sys-root/mingw/include/gtk-2.0/gtk/gtk.h:32,
                 from spice-widget.h:23,
                 from spice-widget-cairo.c:18:
/usr/i686-w64-mingw32/sys-root/mingw/include/gtk-2.0/gdk/gdkcairo.h:54:10: note: expected 'const struct GdkRegion *' but argument is of type 'struct cairo_region_t *'

But also with older version of cairo (on rhel6):

../../gtk/spice-widget-cairo.c: In function ‘spicex_draw_event’:
../../gtk/spice-widget-cairo.c:80: error: ‘cairo_rectangle_int_t’ undeclared (first use in this function)
../../gtk/spice-widget-cairo.c:80: error: (Each undeclared identifier is reported only once
../../gtk/spice-widget-cairo.c:80: error: for each function it appears in.)
../../gtk/spice-widget-cairo.c:80: error: expected ‘;’ before ‘rect’
../../gtk/spice-widget-cairo.c:81: error: ‘cairo_region_t’ undeclared (first use in this function)
../../gtk/spice-widget-cairo.c:81: error: ‘region’ undeclared (first use in this function)
../../gtk/spice-widget-cairo.c:92: error: ‘rect’ undeclared (first use in this function)
../../gtk/spice-widget-cairo.c:96: warning: implicit declaration of function ‘cairo_region_create_rectangle’ [-Wimplicit-function-declaration]
../../gtk/spice-widget-cairo.c:96: warning: nested extern declaration of ‘cairo_region_create_rectangle’ [-Wnested-externs]
../../gtk/spice-widget-cairo.c:106: warning: implicit declaration of function ‘cairo_region_subtract_rectangle’ [-Wimplicit-function-declaration]
../../gtk/spice-widget-cairo.c:106: warning: nested extern declaration of ‘cairo_region_subtract_rectangle’ [-Wnested-externs]
../../gtk/spice-widget-cairo.c:110: warning: implicit declaration of function ‘cairo_region_destroy’ [-Wimplicit-function-declaration]
../../gtk/spice-widget-cairo.c:110: warning: nested extern declaration of ‘cairo_region_destroy’ [-Wnested-externs]
Comment 10 Alexander Larsson 2012-08-31 14:48:20 UTC
Ugh, i guess we need to have fallbacks for the GdkRegion using case too. Should be simple.
Comment 11 Alexander Larsson 2012-08-31 15:04:27 UTC
Created attachment 66410 [details] [review]
Fix build with gtk2
Comment 12 Marc-Andre Lureau 2012-08-31 16:32:42 UTC
(In reply to comment #11)
> Created attachment 66410 [details] [review] [review]
> Fix build with gtk2

that fixes the build indeed, ack
Comment 13 Marc-Andre Lureau 2012-08-31 16:47:22 UTC
pushed gtk2 fix
Comment 14 Alexander Larsson 2012-09-02 17:42:49 UTC
Comment on attachment 66410 [details] [review]
Fix build with gtk2

Review of attachment 66410 [details] [review]:
-----------------------------------------------------------------

::: gtk/spice-widget-cairo.c
@@ +76,5 @@
> +#if !GTK_CHECK_VERSION (3, 0, 0)
> +#define cairo_rectangle_int_t GdkRectangle
> +#define cairo_region_t GdkRegion
> +#define cairo_region_create_rectangle gdk_region_rectangle
> +#define cairo_region_subtract_rectangle(_dest,_rect) { GdkRegion *_region = gdk_region_rectangle (_rect); gdk_region_subtract (_dest, _region); }

Ugh, this leaks _region.
Comment 15 Christophe Fergeau 2012-09-03 07:31:23 UTC
Comment on attachment 66410 [details] [review]
Fix build with gtk2

Review of attachment 66410 [details] [review]:
-----------------------------------------------------------------

::: gtk/spice-widget-cairo.c
@@ +76,5 @@
> +#if !GTK_CHECK_VERSION (3, 0, 0)
> +#define cairo_rectangle_int_t GdkRectangle
> +#define cairo_region_t GdkRegion
> +#define cairo_region_create_rectangle gdk_region_rectangle
> +#define cairo_region_subtract_rectangle(_dest,_rect) { GdkRegion *_region = gdk_region_rectangle (_rect); gdk_region_subtract (_dest, _region); }

This could also use G_STMT_{START,END}
Comment 16 Alexander Larsson 2012-09-03 08:14:15 UTC
Created attachment 66532 [details] [review]
Fix leak
Comment 17 Marc-Andre Lureau 2012-09-03 08:18:35 UTC
Comment on attachment 66532 [details] [review]
Fix leak

Review of attachment 66532 [details] [review]:
-----------------------------------------------------------------

ack (I guess christophe would prefer G_STMT_{START,END} usage)
Comment 18 Alexander Larsson 2012-09-03 08:23:12 UTC
Well, its just used in one place and is a backwards compat define. I don't think making it "nicer" is all that important.
Comment 19 Alexander Larsson 2012-09-03 08:24:51 UTC
(I.E. there are reasons to use G_STMT_{START,END} for macros, but they don't affect the one use of the macro here.)

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.