Re: [PATCH weston] xwm: Don't clear the selection if it has no text type available
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 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
> > ---
> >
> > 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
>
> 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 l
Re: [PATCH weston] xwm: Don't clear the selection if it has no text type available
Hi!,
On Mon, Feb 1, 2016 at 9:36 PM, Derek Foreman 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
> ---
>
> 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
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/
>
>
> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston] xwm: Don't clear the selection if it has no text type available
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
---
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?
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)
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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
