GUACAMOLE-1586: Don't add \n in clipboard if the line isn't finished.

[corentin-soriano]

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.

[necouchman]

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]

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 :

We need to keep this selection format here :

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 :

Content of clipboard :

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

This case is very rare but can exist.

[corentin-soriano]

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

[necouchman]

@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 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]

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!

[necouchman]

I have just a couple of minor concerns about a couple of the comments.

[hugaleno]

@necouchman Is this merge going to be included on 1.6.0 release? I think that would be great

[corentin-soriano]

@necouchman, @jmuehlner, have you any other comments on this PR?

[jmuehlner]

@necouchman, @jmuehlner, have you any other comments on this PR?

It's looking pretty good to me - though I do wonder if it should be targeted at 1.6.0 - it is a bug fix, afterall.

[necouchman]

@necouchman, @jmuehlner, have you any other comments on this PR?

It's looking pretty good to me - though I do wonder if it should be targeted at 1.6.0 - it is a bug fix, afterall.

Agreed - @corentin-soriano, if you could rebase against staging/1.6.0, I think this belongs in the next release.

[corentin-soriano]

Thank you for your feedback!
I changed the base branch to staging/1.6.0.