Bug 54277

Created attachment 66355 [details] [review]
Centralize scaling handling

Created attachment 66356 [details] [review]
Add only_downscale property

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 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.

pushed.

For some reason the background clearing is flickering on one of my machines after this, reopening.

Created attachment 66387 [details] [review]
Fix the flickering

(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.

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]

Ugh, i guess we need to have fallbacks for the GdkRegion using case too. Should be simple.

Created attachment 66410 [details] [review]
Fix build with gtk2

(In reply to comment #11)
> Created attachment 66410 [details] [review] [review]
> Fix build with gtk2

that fixes the build indeed, ack

pushed gtk2 fix

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 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}

Created attachment 66532 [details] [review]
Fix leak

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)

Well, its just used in one place and is a backwards compat define. I don't think making it "nicer" is all that important.

(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.)