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 to x0vncserver #1831

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

gujjwal00
Copy link
Contributor

Most of the groundwork related to selections is already present in TxWindow. And XDesktop already makes use of XFixes, which can provide a notification whenever selection changes. I have added XSelection to handle the rest (took some notes from vncSelection.c in Xvnc).

  • Only CLIPBOARD selection is handled for now (because I don't personally use PRIMARY). Is this sufficient?
  • When notified about selection change, current implementation will immediately ask for clip-data from selection owner, and will then notify VNC clients. It simplifies things a bit and in most configurations net outcome is the same , but vncSelection.c seems to only request data from selection owner when explicitly asked by clients in handleClipboardRequest().

I didn't want to make it too complicated before getting some feedback.

Issue: #529

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

This is awesome! Nice work!

@@ -181,10 +181,13 @@ XDesktop::XDesktop(Display* dpy_, Geometry *geometry_)
if (XFixesQueryExtension(dpy, &xfixesEventBase, &xfixesErrorBase)) {
XFixesSelectCursorInput(dpy, DefaultRootWindow(dpy),
XFixesDisplayCursorNotifyMask);

XFixesSelectSelectionInput(dpy, DefaultRootWindow(dpy), xaCLIPBOARD,
XFixesSetSelectionOwnerNotifyMask);
Copy link
Member

Choose a reason for hiding this comment

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

How difficult would it be to also support the selection clipboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be very difficult, but it does brings in this problem. So a similar approach will be needed here.


// When a client says it has clipboard data, request it
void XDesktop::handleClipboardAnnounce(bool available) {
if (available) server->requestClipboard();
Copy link
Member

Choose a reason for hiding this comment

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

Could we wait until another application requests it? It would avoid transferring things needlessly.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, didn't see you had already mentioned this.

It's not a requirement to fix this, but it would be nice if it's not too much work.

Copy link
Contributor Author

@gujjwal00 gujjwal00 Sep 10, 2024

Choose a reason for hiding this comment

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

Sorry for not being clear, but in PR summary I was talking about how current code issues XConvertSelection() commands immediately on a clipboard change on server side, without first calling announceClipboard(true) and waiting for handleClipboardRequest() call from VNC client. But I already have a solution to do it the 2nd way.

As for the server->requestClipboard(); here, my main motivation was to start with a simple working solution. Retrieving the clipboard from VNC client after another app requests it means that we need to queue that request (and potentially others) and fulfill it later when data is available. As you said, it would avoid needless transfers, and I am hoping to implement it.

Although, requesting it immediately means that when user selects 'Paste', the data is available immediately without potential lag. It also avoids the problem mentioned above about owning multiple selections, as the data will always be cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to implement this lazy loading, but it really complicates things. In addition to the issues mentioned above:

  • Eventually transferred data can be larger then MaxCutText, so selection ownership needs to be released in that case
  • Can't rely on TXWindow to generate SelectionNotify events, because selectionRequest() will only add the request to queue.

So, I plan to keep this simple for now. Because in many cases, data will be immediately transferred anyway (needing to cache data if one selection ownership is lost, clients doing unsolicited transfer, clients not supporting extended clipboard).

Copy link
Member

Choose a reason for hiding this comment

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

Okay. We can have that as a possible future improvement.

unix/x0vncserver/XSelection.cxx Show resolved Hide resolved
@CendioOssman CendioOssman added the enhancement New feature or request label Sep 10, 2024
@francoisp
Copy link

👍 👍

// FIXME: We don't support clipboard yet
Configuration::removeParam("AcceptCutText");
Configuration::removeParam("SendCutText");
Configuration::removeParam("MaxCutText");
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the man page as well now that these are available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gujjwal00
Copy link
Contributor Author

Updates:

  • Added support for PRIMARY selection
  • Data from selection owners is retrieved lazily (when client asks via XDesktop::handleClipboardRequest())
  • Improved error handling

@gujjwal00 gujjwal00 marked this pull request as ready for review September 25, 2024 16:21
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank for your hard work!

unix/x0vncserver/XDesktop.cxx Outdated Show resolved Hide resolved
@CendioOssman CendioOssman merged commit dce6061 into TigerVNC:master Sep 27, 2024
11 checks passed
@bitcoinmeetups
Copy link

Ok, I want to test it. How do I install this version where clipboard works for x0vncserver on Debian 12?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants