swaywm / wlr-protocols

Wayland protocols designed for use in wlroots (and other compositors)
122 stars 29 forks source link

Race using wlr_data_control to create a clipboard manager #92

Open davidedmundson opened 4 years ago

davidedmundson commented 4 years ago

Our clipboard manager (klipper) works as follows:

We had this on X, and I've retrofitted wlr_data_control into it.

I've implemented wlr_data_control and all of wl-copy, wl-paste work perfectly, to some extent klipper works, but there's a race I can't fix neatly, which I think would happen for all other clipboard managers trying to do the same thing.

My race is:

first copy:

second copy:

The compositor gets these in any order, and we end up replacing our new clipboard content with out-of-date previous clipboard contents.


Ultimately to prevent a race I think we need an additional fence like:

    <event name="selection">
        <arg name="id" type="object" interface="zwlr_data_control_offer_v1" allow_null=true>
         An integer value that increases, like the xdg_shell configure events
        <arg name="serial" type="int">
    </event>

    <request name="set_selection">
          <arg name="source" type="object" interface="zwlr_data_control_source_v1"
        allow-null="true"/>
        Optional serial which matches the clipboard event we're responding to. If this is out-of-date this selection will be immediately cancelled and ignored. If negative, we always apply the new selection.
        <arg name="serial" type="int"> 
    </request>
davidedmundson commented 4 years ago

As an alternative, less technically correct, but good enough solution to my problem.

    <request name="set_selection">
          <arg name="source" type="object" interface="zwlr_data_control_source_v1"
        allow-null="true"/>
        <arg name="replaces_current_clipboard" type="boolean"> Some sort of flag to say we only apply the contents if no client has a selection.  Otherwise the selection is immediately cancelled.
    </request>
emersion commented 4 years ago

I agree that race is worth fixing. Serials are uint in the Wayland protocol and any value is a valid serial, so I guess we'll need something else (like two separate requests).

But if an application really wants to unset the selection, should a clipboard manager always reset it to the previous value? For instance if an application copies a password to the clipboard and wants to empty the clipboard after 30 seconds, we don't want the clipboard manager to reset the clipboard with the password text afterwards.

davidedmundson commented 4 years ago

But if an application really wants to unset the selection, should a clipboard manager always reset it to the previous value?

Yes, but that's also more complex. We don't want to just not restore it, we also want to not show it in our UI or save it to disk. That needs to happen at the time the selection is made, not the time it's cleared.

On X11 we "solve" this with a magic mimetype. We have a mimetype of x-kde-passwordManagerHint and then our clipboard manager just skips it.

emersion commented 4 years ago

Well, in a perfect world, all clients support the text-input protocol, and the passwords never go through the clipboard, they are fed directly to the receiving client via text-input.

I wonder if there are other use-cases for emptying the clipboard?

davidedmundson commented 4 years ago

I had a quick look through our bug reports for klipper, which go back 14 years, and password managers seem to be the only case where a behaviour change was requested.

davidedmundson commented 4 years ago

I agree that race is worth fixing. Serials are uint in the Wayland protocol and any value is a valid serial, so I guess we'll need something else (like two separate requests).

Should I submit a MR to do that in a v2. If I do it as 2 requests, and I send the serial of the selection event separately then it could be backwards compatible.

emersion commented 4 years ago

Yeah, I think that'd be a good idea.

claell commented 3 years ago

@davidedmundson Do you have an update on the state of this? If I am not mistaken this issue starts to hit now after the release of Plasma 5.20, so this will likely produce user frustration soon.

davidedmundson commented 3 years ago

I merged a hacky workaround for 5.20 Effectively like above but via a mimetype offer with a special key meaning it's from klipper and only replaces an empty clipboard.

Obviously I want to merge something cleaner and universal but the time pressure is off.

claell commented 3 years ago

Has that been recently and still needs time to be integrated into 5.20.1, maybe? Asking, because I am experiencing this bug on 5.20 right now. For reference: https://bugs.kde.org/show_bug.cgi?id=424754.

emersion commented 2 years ago

wlr-protocols has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/issues/92