mike-jumper commented on code in PR #597:
URL: https://github.com/apache/guacamole-server/pull/597#discussion_r2124840287
##########
src/terminal/terminal.c:
##########
@@ -868,6 +868,14 @@ void guac_terminal_scroll_up(guac_terminal* term,
void guac_terminal_scroll_down(guac_terminal* term,
int start_row, int end_row, int amount) {
+ /*
+ * This is necessary because guac_terminal_copy_rows will set new
GUAC_CHAR_COPY
+ * for cells of shifted rows meanwhile preceding GUAC_CHAR_SET for these
cells
+ * are not actually done yet. The GUAC_CHAR_COPY operation has the highest
priority,
+ * so if we do not 'flush' here, wrong content will be copied.
+ */
+ guac_terminal_display_flush(term->display);
Review Comment:
A couple things:
1. While it might be necessary to get these operations handled/flushed
within the terminal emulator, the internals of `guac_terminal_display_flush()`
mean that this will still result in a full flush of a frame:
https://github.com/apache/guacamole-server/blob/d80112c993a352c4e366317c0ae12919bfa61011/src/terminal/display.c#L863-L873
If the need for this call is specifically that we need to ensure
`GUAC_CHAR_COPY`, etc. are handled before performing yet more copies, then we
should perform a partial flush that does just that (without flushing images via
`guac_common_surface_flush()` and incurring performance penalties).
If there is a way to _only_ flush the operations that need to be flushed
(rather than the entire display), that would be better still.
2. Shouldn't this be at a lower level, like `guac_terminal_copy_rows()` or
`guac_terminal_display_copy_rows()`? `guac_terminal_scroll_down()` is a
specific instance of the issue, but it sounds like the problem is more
fundamental.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
