[Spice-devel] [PATCH vdagent 0/2] clipboard system redesign

[Victor Toso]

Hey,

Sorry for taking some time to review your patches!

On Sun, Jan 21, 2018 at 09:03:12PM +0100, Jakub Janků wrote:
> Hi,
> these two patches replace the current clipboard handling code,
> which uses Xlib, with code utilizing GTK+. The code was moved
> from x11.c to clipboard.c
>
> Although GTK+ can work on Wayland natively, this code currently
> works only under X. gdk_set_allowed_backends("x11") in
> vdagent.c is called to force GTK+ to always use X11 backend.
> 
> The implementation is partially based on the code in spice-gtk
> (spice-gtk-session.c).
> 
> Cheers,

Awesome work. Tested and seems to work as expected on F26 guest.
I'll be reviewing this inline but I'd like to suggest a different
approach to the patches.

For the clipboard, GTK+ is going to replace x11 fairly well and I
don't expect major problems but we might have some, we never
know. I would like to do one last vdagent release with x11
clipboard to explicit say that we will replace x11 with gtk+
after that. I think we can do a release as soon as this gets in.

So, removing x11 calls would be a problem.

Another issue is that, it is hard to review/compare when one
patch removes the feature and the other add it back besides the
fact that bisecting would be broken to that feature (although its
not a big problem in this case, I think).

My suggestion is to have a ./configure --with-gtk or --enable-gtk
to wrap new code around #ifdef GTK_ENABLED for the new code
otherwise it calls the x11 code as before.

I think the clipboard function functions exported by x11.h should
be moved to clipboard.[ch] and together with the proposed
VDAgentClipboards (interface) changes.

What do you think?
Cheers,
        toso

> 
>  Makefile.am             |    2 +
>  src/vdagent/clipboard.c |  401 ++++++++++++++++++
>  src/vdagent/clipboard.h |   42 ++
>  src/vdagent/vdagent.c   |   38 +-
>  src/vdagent/x11-priv.h  |   91 ----
>  src/vdagent/x11.c       | 1074 +----------------------------------------------
>  src/vdagent/x11.h       |   10 -
>  7 files changed, 472 insertions(+), 1186 deletions(-)
>  create mode 100644 src/vdagent/clipboard.c
>  create mode 100644 src/vdagent/clipboard.h
> 
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180212/e5d15aa7/attachment.sig>