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

Remove client-server shortcut paste code #2810

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

matt335672
Copy link
Member

Fixes #1839

@jsorg71 - I'd appreciate your feedback on this PR. Are I missing something obvious?

When significant amounts of data is coming from the client in a fragmented CLIPRDR_DATA_RESPONSE PDU, this code provides a way to start copying it to a requesting client before it is all read.

The only advantage of this code is to provide a slight speedup before a paste is visible on the server.

There are significant problems with this code. Notably, it is very difficult to parse Unicode text coming through this route. Each UTF-16 character can occupy up to 4 bytes, and a fragmentation boundary could occur at any point within a UTF-16 character.

When significant amounts of data is coming from the client in a
fragmented CLIPRDR_DATA_RESPONSE PDU, this code provides a way to
start copying it to a requesting client before it is all read.

The only advantage of this code is to provide a slight speedup
before a paste is visible on the server.

There are significant problems with this code. Notably, it is
very difficult to parse Unicode text coming through this route. Each
UTF-16 character can occupy up to 4 bytes, and a fragmentation
boundary could occur at any point within a UTF-16 character.
@matt335672
Copy link
Member Author

In the absence of any feedback I'm going to merge this change as it fixes a customer problem. Happy to revert if it proves necessary.

@matt335672 matt335672 merged commit eb1c3cd into neutrinolabs:devel Oct 11, 2023
13 checks passed
@matt335672 matt335672 deleted the remove_ss_clipboard_code branch October 11, 2023 09:38
@jsorg71
Copy link
Contributor

jsorg71 commented Oct 11, 2023

Hi Matt, Sorry for delay,
I think you can see what this ss code was doing. The clipboard channel chunks out clipboard data to transmit, the X11 clipboard chunks out data to transfer between X11 clients. Hook them up so the app starts getting data right away. Seems great except you need to know the size in the beginning and converting UTF 16 to UTF 8 you can not know the size. I did notice you can guess the size too small but guessing too large hangs X. We could get around that by dumping zeros at the end maybe.
I think it's ok to just remove it. If we need something like this back, we can improve on it.

@matt335672
Copy link
Member Author

I got the gist of the code. The main problem I had was more related to #2603 rather than the size. Addressing #2603 means coping with UTF-16 characters being either 2 or 4 bytes (e.g. 😥 maps to 0x3d 0xd8 0x25 0xde in the byte stream). Converting that to UTF-8 on the fly in the ss code means caching up to 3 bytes potentially when we hit a fragmentation point. It's doable, but messy.

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.

Large clipboard pastes fail
2 participants