[PATCH weston] xwm: Don't clear the selection if it has no text type available

[Bryce Harrington]

On Thu, Feb 04, 2016 at 11:37:36PM +0100, Carlos Garnacho wrote:
> Hi!,
> 
> On Mon, Feb 1, 2016 at 9:36 PM, Derek Foreman <derekf at osg.samsung.com> wrote:
> > weston maintains a copy of the most recently selected "thing" - it picks
> > the first available type when it copies, and saves that one only.
> >
> > When an application quits weston will make the saved selection active.
> >
> > When xwm sees the selection set it will check if any of the offered types
> > are text.  If no text type is offered it will clear the selection.
> >
> > weston then interprets this in the same way as an application exiting and
> > causing the selection to be unset, and we get caught in a live lock with
> > both weston and xwayland consuming as much cpu as they can.
> >
> > The simple fix is to just remove the test for text presence.
> >
> > Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> > ---
> >
> > This is kind of an alternate solution to the problem fixed in
> > http://patchwork.freedesktop.org/patch/70435
> >
> > I don't understand why offers with no text type are cleared here,
> > and couldn't find why with git blame either.  Anyone know why
> > we've been doing this?
> 
> No idea myself, seems a rather arbitrary check. there shouldn't be any
> reason this shouldn't work for other mimetypes offered.
> 
> >
> > With this patch we can get somewhat odd behaviour - if terminology
> > exits with a selection it can only be pasted into another terminology
> > not a weston-terminal, because text wasn't the first offer and weston
> > only saves the first offer.
> >
> > That seems better than a livelock though.  Maybe we can grab all available
> > offers when cloning the selection instead of just the first (but that should
> > be done in addition to this patch, I think, because we could have an app that
> > doesn't offer text at all exit with a selection set and cause the same lock)
> 
> Reviewed-by: Carlos Garnacho <carlosg at gnome.org>
> 
> I agree with the conclusion and the approach in this patch. As you say
> the next step is storing all offers, but I guess it depends on how
> deep we want weston to get into the clipboard manager role. AFAICS
> that'd be most similar to how toolkits/apps used to behave wrt
> SAVE_TARGETS as defined in the clipboard manager spec [1], where
> simply all mimetypes would be stored (see eg. gtk_clipboard_store())
> 
> Cheers,
>   Carlos
> 
> [1] http://www.freedesktop.org/wiki/ClipboardManager/

Thanks, pushed:

To ssh://git.freedesktop.org/git/wayland/weston
   22b1f93..4e18448  master -> master

>From the discussion I'm a little worried that there might be side
effects, or at least that this may need some follow on work, but since
the scope is limited to xwm the risk level here seems acceptable.

Bryce

> >  xwayland/selection.c | 28 ++++------------------------
> >  1 file changed, 4 insertions(+), 24 deletions(-)
> >
> > diff --git a/xwayland/selection.c b/xwayland/selection.c
> > index 25ec848..24aa324 100644
> > --- a/xwayland/selection.c
> > +++ b/xwayland/selection.c
> > @@ -655,8 +655,6 @@ weston_wm_set_selection(struct wl_listener *listener, void *data)
> >         struct weston_wm *wm =
> >                 container_of(listener, struct weston_wm, selection_listener);
> >         struct weston_data_source *source = seat->selection_data_source;
> > -       const char **p, **end;
> > -       int has_text_plain = 0;
> >
> >         if (source == NULL) {
> >                 if (wm->selection_owner == wm->selection_window)
> > @@ -670,28 +668,10 @@ weston_wm_set_selection(struct wl_listener *listener, void *data)
> >         if (source->send == data_source_send)
> >                 return;
> >
> > -       p = source->mime_types.data;
> > -       end = (const char **)
> > -               ((char *) source->mime_types.data + source->mime_types.size);
> > -       while (p < end) {
> > -               weston_log("  %s\n", *p);
> > -               if (strcmp(*p, "text/plain") == 0 ||
> > -                   strcmp(*p, "text/plain;charset=utf-8") == 0)
> > -                       has_text_plain = 1;
> > -               p++;
> > -       }
> > -
> > -       if (has_text_plain) {
> > -               xcb_set_selection_owner(wm->conn,
> > -                                       wm->selection_window,
> > -                                       wm->atom.clipboard,
> > -                                       XCB_TIME_CURRENT_TIME);
> > -       } else {
> > -               xcb_set_selection_owner(wm->conn,
> > -                                       XCB_ATOM_NONE,
> > -                                       wm->atom.clipboard,
> > -                                       XCB_TIME_CURRENT_TIME);
> > -       }
> > +       xcb_set_selection_owner(wm->conn,
> > +                               wm->selection_window,
> > +                               wm->atom.clipboard,
> > +                               XCB_TIME_CURRENT_TIME);
> >  }
> >
> >  void
> > --
> > 2.7.0
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel