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-1586: Don't add \n in clipboard if the line isn't finished. #512

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

Conversation

corentin-soriano
Copy link
Contributor

@corentin-soriano corentin-soriano commented Apr 20, 2024

In the case of selecting multiple lines, do not add \n if the line is not really finished.

Changes:

  • Correct buffer_row length on terminal init.
  • Using the last column of the buffer_row (out of screen) to store a 1 if the word wrap was forced (0 is written when initializing the row).
  • In case of selection on several lines, we add a \n only if the value of the last column is 0.
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'm wondering if this entire section, including the if statement immediately above here, could be simplified to something like this:

guac_terminal_select_normalized_range(terminal, &start_row, &start_col, &end_row, &end_col);
int row = start_row;

do {

  // Copy the current row.
  guac_terminal_clipboard_append_row(terminal, row, start_col, end_col);
  guac_terminal_buffer_row* buffer_row = guac_terminal_buffer_get_row(terminal->buffer, row, 0);
  int last_char = buffer_row->characters[terminal->term_width - 1].value;

  if (last_char == 0)
      guac_common_clipboard_append(terminal->clipboard, "\n", 1);

  // Increment row
  row++;
}
while (row <= end_row);

?

I haven't actually tested that block of code, but it works in my head :-). I'm not sure if there's some reason that the code needs the extra if...else blocks, plus the loop? Maybe there are some corner cases that don't work - like if the last line does or does not end with a line break??

@corentin-soriano
Copy link
Contributor Author

We must keep this existing logic :

  • first row = start_col to $
  • middle rows = ^ to $
  • last row = ^ to end_col

With guac_terminal_clipboard_append_row(terminal, row, start_col, end_col); for each row, we risk having a rectangular selection of this type :
image
We need to keep this selection format here :
image

However, I noticed that if we select a row that has exactly the same number of columns as the display, it will be missing a \n :
Example :
image
Content of clipboard :

[root@vm01 ~]# echo -e "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\ntest\ntest2"
eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeetest
test2

This case is very rare but can exist.

@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch 3 times, most recently from a6a8a3d to e70159a Compare May 30, 2024 09:32
@corentin-soriano corentin-soriano marked this pull request as ready for review May 30, 2024 09:34
@corentin-soriano
Copy link
Contributor Author

I've reworked this PR, it's ready for review!

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.

@corentin-soriano The changes look good to me, although @mike-jumper and @jmuehlner are more familiar with the terminal implementation, so I'm curious if either of them has any feedback on it.

My only other request is to rebase this against the patch branch, as I think this is more of a bug fix than a feature implementation - having a stray newline added to the clipboard when it is not part of the actual text your copying seems like a (minor) bug.

@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch from e70159a to 5da2d4a Compare May 30, 2024 13:33
@corentin-soriano corentin-soriano changed the base branch from main to patch May 30, 2024 13:36
@corentin-soriano
Copy link
Contributor Author

@corentin-soriano The changes look good to me, although @mike-jumper and @jmuehlner are more familiar with the terminal implementation, so I'm curious if either of them has any feedback on it.

My only other request is to rebase this against the patch branch, as I think this is more of a bug fix than a feature implementation - having a stray newline added to the clipboard when it is not part of the actual text your copying seems like a (minor) bug.

Rebased from patch branch.

@hugaleno
Copy link

Just passing here to thank you @corentin-soriano for this contribution and also for the work on Guacamole-192.
And of course want to thank you all guacamole team to the great work being done.
Keep up the good work!

@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch 2 times, most recently from 40b09a6 to d7de57d Compare June 6, 2024 17:08
src/terminal/buffer.c Outdated Show resolved Hide resolved
@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch from d7de57d to 63abf5d Compare June 11, 2024 09:23
@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch 3 times, most recently from a25c0d5 to 108b787 Compare June 17, 2024 14:33
@corentin-soriano corentin-soriano force-pushed the GUACAMOLE-1586_clipboard_dont_break_lines branch from 108b787 to 1521094 Compare June 17, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants