Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Clipboard support for RemoteDesktop via org.freedesktop.portal.Clipboard #852

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

maxhbooth
Copy link
Contributor

@maxhbooth maxhbooth commented Aug 8, 2022

This PR is to add clipboard controls for RemoteDesktop as described in #843.

xdg-desktop-portal-gnome

Fix #843

@maxhbooth maxhbooth marked this pull request as draft August 8, 2022 13:15
@maxhbooth maxhbooth changed the title [WIP] Add Clipboard support for RemoteDesktopvia org.freedesktop.portal.Clipboard [WIP] Add Clipboard support for RemoteDesktop via org.freedesktop.portal.Clipboard Aug 8, 2022
@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 3 times, most recently from 92d4dc3 to 01ac995 Compare August 10, 2022 16:47
@maxhbooth maxhbooth changed the title [WIP] Add Clipboard support for RemoteDesktop via org.freedesktop.portal.Clipboard Add Clipboard support for RemoteDesktop via org.freedesktop.portal.Clipboard Aug 10, 2022
@maxhbooth maxhbooth marked this pull request as ready for review August 10, 2022 16:48
src/clipboard.c Outdated Show resolved Hide resolved
@gasinvein
Copy link
Member

It it possible to use this without RemoteDesktop? I believe a Clipboard portal would be useful on its own, as a more convenient way to interact with the clipboard on Wayland.

@jadahl
Copy link
Collaborator

jadahl commented Aug 11, 2022

It it possible to use this without RemoteDesktop? I believe a Clipboard portal would be useful on its own, as a more convenient way to interact with the clipboard on Wayland.

Why would you want that? Clipboard in the form that remote desktop applications need should not be exposed to applications for convenience, since it avoids limiting clipboard content to the focused application and user interaction on that application. Doing that has very large privacy implications.

@gasinvein
Copy link
Member

It it possible to use this without RemoteDesktop? I believe a Clipboard portal would be useful on its own, as a more convenient way to interact with the clipboard on Wayland.

Why would you want that?

One example I have in mind is a translation app that has a feature to translate selected text and put translation back to clipboard; this can be triggered without interacting with the app main window, via hotkey or tray icon menu.

Clipboard in the form that remote desktop applications need should not be exposed to applications for convenience, since it avoids limiting clipboard content to the focused application and user interaction on that application. Doing that has very large privacy implications.

I suppose user has to grant the app permission to interact with clipboard first?

@maxhbooth
Copy link
Contributor Author

I suppose user has to grant the app permission to interact with clipboard first?

Yes. We certainly don't want applications to control a user's clipboard without their consent.

One example I have in mind is a translation app that has a feature to translate selected text

In #843 it was mentioned that this interface could be useful for the InputCapture portal in the future, which might solve this use case?

@gasinvein
Copy link
Member

One example I have in mind is a translation app that has a feature to translate selected text

In #843 it was mentioned that this interface could be useful for the InputCapture portal in the future, which might solve this use case?

Sorry, I don't know anything about the proposed InputCapture portal.

@maxhbooth
Copy link
Contributor Author

@jadahl, there hasn't been any progress on this PR for awhile, is there anything I should do to get some feedback on it?

@jadahl
Copy link
Collaborator

jadahl commented Oct 4, 2022

@jadahl, there hasn't been any progress on this PR for awhile, is there anything I should do to get some feedback on it?

I'll try to do a round of review as soon as I can.

Copy link
Collaborator

@jadahl jadahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked close at the actual C code yet, as if we're dropping the Request method dance, it'll likely look different enough to invalidate any review I do today.

data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Show resolved Hide resolved
data/org.freedesktop.portal.RemoteDesktop.xml Show resolved Hide resolved
src/remote-desktop.c Outdated Show resolved Hide resolved
src/remote-desktop.c Outdated Show resolved Hide resolved
src/clipboard.c Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 2 times, most recently from 343c371 to 9130a52 Compare October 31, 2022 18:35
@maxhbooth
Copy link
Contributor Author

maxhbooth commented Nov 17, 2022

@jadahl, thanks for your first review! I think I've addressed all your comments now, if you have time for another round 😄

src/xdg-desktop-portal.c Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.Clipboard.xml Outdated Show resolved Hide resolved
src/clipboard.c Outdated Show resolved Hide resolved
src/clipboard.c Outdated Show resolved Hide resolved
src/clipboard.c Outdated Show resolved Hide resolved
src/remote-desktop.c Outdated Show resolved Hide resolved
tests/templates/clipboard.py Outdated Show resolved Hide resolved
@garnacho
Copy link

I've read through the changes and the proposed API makes sense to me. I feel there should be further documentation on the expected order of events after the SelectionTransfer signal, e.g. how to react to transfer requests of unrelated mime types or if no FD can be obtained.

In that regard, I mildly wonder if passing an FD should be implicitly considered success, writing on an end of a pipe accounts on the other side reading or the pipe will grow full and writes will stall, and having to wait to the end of the transfer for a SelectionWriteDone verifying the success means the other end needs to cache the full result before doing anything useful with it.

@pnowack
Copy link

pnowack commented Nov 18, 2022

Since the portal clipboard API here is based on the private clipboard API between mutter and g-r-d, there is something to note here:

There are thoughts about changing the SelectionWrite and SelectionWriteDone methods in a way by using lists of what they actually currently have for arguments.
The reason is to group calls to kill latency when handling multiple clipboard content requests.
Due to time constraints, this was so far not realized for mutter and g-r-d (other tasks got priority and I only was made aware of this PR today) and so these changes did not went in yet.

For the portal API we probably wanna change the handling in that direction, so that mutter would not have two paths for handling selection writes and their results.
For SelectionWriteDone this would mean, a list would be submitted, where each element contains the serial and its sucess value.
I don't remember my thoughts about SelectionWrite to what it should be changed to be able handle multiple requests at the same time, but using a list of elements of (serial, fd) might be sufficient for this.

@matthiasclasen
Copy link
Contributor

Since this is only about remote desktop sessions, I don't think it should be called org.freedesktop.portal.Clipboard. That name is confusing and implies that this also covers local clipboards, which are handled via display server protocols already.

@jadahl
Copy link
Collaborator

jadahl commented Nov 18, 2022

Since this is only about remote desktop sessions, I don't think it should be called org.freedesktop.portal.Clipboard. That name is confusing and implies that this also covers local clipboards, which are handled via display server protocols already.

This is not a separate clipboard portal, but a way to hook up clipboard to the remote desktop portal. Since we might want clipboard to other portals (e.g. input capture), it makes sense to keep it separate, and not tied to remote desktop, otherwise we need to copy paste the whole API in every place its used, including follow up fixes / changes.

@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 2 times, most recently from 347d3a2 to c8d8cc9 Compare November 28, 2022 22:34
@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 2 times, most recently from 2cdf320 to d889238 Compare February 7, 2023 18:34
@maxhbooth
Copy link
Contributor Author

Thanks @scmv! I've updated the serials in SelectionWrite to use au instead of a(u) now.

@maxhbooth
Copy link
Contributor Author

maxhbooth commented Feb 7, 2023

@jadahl I added some tests and finished your last code review

@maxhbooth maxhbooth requested a review from jadahl February 7, 2023 18:47
@maxhbooth
Copy link
Contributor Author

fyi I think this PR is just waiting for more maintainers to chime in with any thoughts

@jadahl jadahl added this to the 1.18 milestone Apr 14, 2023
@jadahl
Copy link
Collaborator

jadahl commented Apr 14, 2023

This needs a rebase to drop the autotools parts.

@maxhbooth
Copy link
Contributor Author

This needs a rebase to drop the autotools parts.

Done!

Copy link
Collaborator

@jadahl jadahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more or less ready to land. I'd like to go back to a simpler previous version (see comment about SelectionWrite(Done) about what/why), but other than that it seems ready.

data/org.freedesktop.impl.portal.Clipboard.xml Outdated Show resolved Hide resolved
src/remote-desktop.c Show resolved Hide resolved
data/meson.build Show resolved Hide resolved
tests/__init__.py Show resolved Hide resolved
@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 2 times, most recently from f323720 to 5ec1b5a Compare June 20, 2023 21:35
data/org.freedesktop.portal.RemoteDesktop.xml Show resolved Hide resolved
src/remote-desktop.c Outdated Show resolved Hide resolved
@maxhbooth maxhbooth force-pushed the feature/Clipboard branch 3 times, most recently from 6c5b5d1 to b8b92d2 Compare June 21, 2023 19:37
@maxhbooth maxhbooth requested a review from jadahl June 21, 2023 19:38
@maxhbooth
Copy link
Contributor Author

@jadahl Done!

Remove some state that didn't help much keeping track of separately. We'll
want to be able to prepare more in an arbitrary order before starting so
just have a single state for not having started (`INIT`).
Add an interface to allow sessions to interact with the clipboard
(in particular, the remote desktop session.)
Copy link
Collaborator

@jadahl jadahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything lgtm. Planning to land this within some days, e.g. monday next week, if no-one objects before that.

@jadahl jadahl merged commit 252b4a3 into flatpak:main Jul 6, 2023
3 checks passed
@maxhbooth maxhbooth deleted the feature/Clipboard branch July 6, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add Clipboard Control to RemoteDesktop
8 participants