[Spice-devel] [PATCH spice-gtk 4/4] gtk: handle clipboard CRLF conversion

[Hans de Goede]

Hi,

On 08/24/2013 02:18 PM, Marc-André Lureau wrote:
> On Sat, Aug 24, 2013 at 12:27 PM, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>>
>> On 08/23/2013 10:25 PM, Marc-André Lureau wrote:
>>>
>>> gtk+ internal text/utf8 is using LF conversion, on all platforms.
>>> Even though the toolkit may only handle a single line ending type, we
>>> may want to avoid the conversion for Spice use cases, gtk+ could learn a
>>> new native utf8 target type, see also:
>>> https://bugzilla.gnome.org/show_bug.cgi?id=706657
>>>
>>> In the meantime, the only thing we need to convert, is to/from crlf
>>> guest (from/to lf). This is what this change is about.
>>>
>>> It has been tested successfully with the various guest/client OS
>>> combinations.
>>> ---
>>>    gtk/Makefile.am         |  2 ++
>>>    gtk/spice-gtk-session.c | 56
>>> ++++++++++++++++++++++++++++++++++++++++++++-----
>>>    2 files changed, 53 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/gtk/Makefile.am b/gtk/Makefile.am
>>> index 82aa9a3..5af6642 100644
>>> --- a/gtk/Makefile.am
>>> +++ b/gtk/Makefile.am
>>> @@ -117,6 +117,8 @@ SPICE_GTK_LIBADD_COMMON =           \
>>>
>>>    SPICE_GTK_SOURCES_COMMON =            \
>>>          glib-compat.h                   \
>>> +       spice-util.c                    \
>>> +       spice-util-priv.h               \
>>>          spice-gtk-session.c             \
>>>          spice-gtk-session-priv.h        \
>>>          spice-widget.c                  \
>>> diff --git a/gtk/spice-gtk-session.c b/gtk/spice-gtk-session.c
>>> index 68777eb..476af95 100644
>>> --- a/gtk/spice-gtk-session.c
>>> +++ b/gtk/spice-gtk-session.c
>>> @@ -23,6 +23,7 @@
>>>    #include "spice-gtk-session.h"
>>>    #include "spice-gtk-session-priv.h"
>>>    #include "spice-session-priv.h"
>>> +#include "spice-util-priv.h"
>>>
>>>    #define CLIPBOARD_LAST (VD_AGENT_CLIPBOARD_SELECTION_SECONDARY + 1)
>>>
>>> @@ -548,6 +549,7 @@ static void clipboard_owner_change(GtkClipboard
>>> *clipboard,
>>>
>>>    typedef struct
>>>    {
>>> +    SpiceGtkSession *self;
>>>        GMainLoop *loop;
>>>        GtkSelectionData *selection_data;
>>>        guint info;
>>> @@ -555,21 +557,45 @@ typedef struct
>>>    } RunInfo;
>>>
>>>    static void clipboard_got_from_guest(SpiceMainChannel *main, guint
>>> selection,
>>> -                                     guint type, guchar *data, guint
>>> size,
>>> +                                     guint type, const guchar *data,
>>> guint size,
>>>                                         gpointer user_data)
>>>    {
>>>        RunInfo *ri = user_data;
>>> +    SpiceGtkSessionPrivate *s = ri->self->priv;
>>> +    gchar *conv = NULL;
>>>
>>>        g_return_if_fail(selection == ri->selection);
>>>
>>>        SPICE_DEBUG("clipboard got data");
>>>
>>> -    gtk_selection_data_set(ri->selection_data,
>>> -        gdk_atom_intern_static_string(atom2agent[ri->info].xatom),
>>> -        8, data, size);
>>> +    if (atom2agent[ri->info].vdagent == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
>>> +        /* on windows, gtk+ would already convert to LF endings, but
>>> +           not on unix */
>>> +        if (spice_main_agent_test_capability(s->main,
>>> VD_AGENT_CAP_GUEST_LINEEND_CRLF)) {
>>> +            GError *err = NULL;
>>> +
>>> +            conv = spice_dos2unix((gchar*)data, size, &err);
>>> +            if (err) {
>>> +                g_warning("Failed to convert text line ending: %s",
>>> err->message);
>>> +                g_clear_error(&err);
>>> +                goto end;
>>> +            }
>>> +
>>> +            size = strlen(conv);
>>> +        }
>>> +
>>> +        gtk_selection_data_set_text(ri->selection_data, conv ?:
>>> (gchar*)data, size);
>>
>>
>> Why the "conv ?: (gchar*)data" here ? Should conv not always be non NULL
>> here, unless
>> size = 0, in which case conv == NULL is fine ?
>>
>>
>>
>>
>>> +    } else {
>>> +        gtk_selection_data_set(ri->selection_data,
>>> +            gdk_atom_intern_static_string(atom2agent[ri->info].xatom),
>>> +            8, data, size);
>>> +    }
>>>
>>> +end:
>>>        if (g_main_loop_is_running (ri->loop))
>>>            g_main_loop_quit (ri->loop);
>>> +
>>> +    g_free(conv);
>>>    }
>>>
>>>    static void clipboard_agent_connected(RunInfo *ri)
>>> @@ -604,6 +630,7 @@ static void clipboard_get(GtkClipboard *clipboard,
>>>        ri.info = info;
>>>        ri.loop = g_main_loop_new(NULL, FALSE);
>>>        ri.selection = selection;
>>> +    ri.self = self;
>>>
>>>        clipboard_handler = g_signal_connect(s->main,
>>> "main-clipboard-selection",
>>>
>>> G_CALLBACK(clipboard_got_from_guest),
>>> @@ -740,8 +767,27 @@ static void clipboard_received_cb(GtkClipboard
>>> *clipboard,
>>>            g_free(name);
>>>        }
>>>
>>> +    const guchar *data = gtk_selection_data_get_data(selection_data);
>>> +    gpointer conv = NULL;
>>> +
>>> +    /* gtk+ internal utf8 newline is always LF, even on windows */
>>> +    if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT &&
>>> +        spice_main_agent_test_capability(s->main,
>>> VD_AGENT_CAP_GUEST_LINEEND_CRLF)) {
>>> +        GError *err = NULL;
>>> +
>>> +        conv = spice_unix2dos((gchar*)data, len, &err);
>>> +        if (err) {
>>> +            g_warning("Failed to convert text line ending: %s",
>>> err->message);
>>> +            g_clear_error(&err);
>>> +            return;
>>> +        }
>>> +
>>> +        len = strlen(conv);
>>> +    }
>>> +
>>>        spice_main_clipboard_selection_notify(s->main, selection, type,
>>> -        gtk_selection_data_get_data(selection_data), len);
>>> +                                          conv ?: data, len);
>>> +    g_free(conv);
>>
>>
>> Same question here.
>>
>> Otherwise this looks good.
>>
>> Regards,
>>
>> Hans
>
> conv is non-null only if a conversion was done.

Ok.

Regards,

Hans