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

GUACAMOLE-1973: Add support for XTerm bracketed-paste mode #533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scottp-dpaw
Copy link

No description provided.

@necouchman
Copy link
Contributor

@scottp-dpaw Since this is a new feature, and not a bug fix, this can be based against the main branch of the Apache repo rather than the patch branch. :-)

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

I have two overall concerns with the code, here, as illustrated by the one comment I've left inline, below:

  1. You've done a reasonable job of documenting things within the code, but there are a few places that could use slightly more documentation.
  2. There are several places where constants may be useful.

Other than that, I'll let @mike-jumper and @jmuehlner take a closer look, since they're much more familiar with the terminal code than I am.

@@ -1500,6 +1501,87 @@ int guac_terminal_send_string(guac_terminal* term, const char* data) {

}

int guac_terminal_send_clipboard(guac_terminal *term) {
char *filtered = guac_mem_alloc(term->clipboard->length + 12);
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be completely obvious to you why you need to allocate 12 bytes in addition to the clipboard length, here, but it isn't to me. This is one example of a place where 1) more comments throughout the code might be helpful, and 2) this may be a good place for a constant to replace the literal 12.

@scottp-dpaw scottp-dpaw changed the base branch from patch to main July 31, 2024 03:23
@scottp-dpaw
Copy link
Author

No worries. In lieu of making constants for every bitmask, I've added clearer comments describing the sequence of bits that we're trying to find for each codepoint. Hopefully that should provide enough context to the reader about what we're trying to do, if not I'm open to suggestions.
Gotta say writing out more comments was a worthwhile exercise; I found two errors with my original logic. Turns out Unicode is hard!

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.

2 participants