[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #385: GUACAMOLE-1622: Restructured code to resolve scrollbar resizing bug.
mike-jumper commented on code in PR #385: URL: https://github.com/apache/guacamole-server/pull/385#discussion_r920512889 ## src/terminal/terminal.c: ## @@ -500,26 +528,33 @@ guac_terminal* guac_terminal_create(guac_client* client, term->clipboard = guac_common_clipboard_alloc(); term->disable_copy = options->disable_copy; -/* Calculate available text display area by character size */ -int rows, columns; -calculate_rows_and_columns(term, height, width, &rows, &columns); +/* Set size of available screen area */ +term->outer_width = width; +term->outer_height = height; -/* Calculate available text display area in pixels */ +/* Calculate available display area of text in pixels */ int available_height, available_width; -calculate_height_and_width(term, rows, columns, -&available_height, &available_width); +calculate_available_height_and_width(term, +height, width, &available_height, &available_width); -/* Set size of available screen area */ -term->outer_height = height; -term->outer_width = width; +/* Calculate available display area of text by character size */ +int rows, columns; +calculate_rows_and_columns(term, available_height, available_width, +&rows, &columns); -/* Set rows and columns size */ -term->term_height = rows; -term->term_width = columns; +/* Calculate height & width within predefined maximum of rows and columns */ +int adjusted_height = height; +int adjusted_width = width; +calculate_adjusted_height_and_width(term, rows, columns, +&adjusted_height, &adjusted_width); /* Set pixel size */ -term->height = available_height; -term->width = available_width; +term->width = adjusted_width; +term->height = adjusted_height; Review Comment: Welp, these changes as they stand now are definitely nice, readable, and minimal. If you can squash that "Propose a fix ..." commit up into the main commit (or rephrase to capture the nature of the change rather than the nature of the PR workflow), I'm happy to merge. -- 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]
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #385: GUACAMOLE-1622: Restructured code to resolve scrollbar resizing bug.
mike-jumper commented on code in PR #385: URL: https://github.com/apache/guacamole-server/pull/385#discussion_r920287615 ## src/terminal/terminal.c: ## @@ -500,26 +528,33 @@ guac_terminal* guac_terminal_create(guac_client* client, term->clipboard = guac_common_clipboard_alloc(); term->disable_copy = options->disable_copy; -/* Calculate available text display area by character size */ -int rows, columns; -calculate_rows_and_columns(term, height, width, &rows, &columns); +/* Set size of available screen area */ +term->outer_width = width; +term->outer_height = height; -/* Calculate available text display area in pixels */ +/* Calculate available display area of text in pixels */ int available_height, available_width; -calculate_height_and_width(term, rows, columns, -&available_height, &available_width); +calculate_available_height_and_width(term, +height, width, &available_height, &available_width); -/* Set size of available screen area */ -term->outer_height = height; -term->outer_width = width; +/* Calculate available display area of text by character size */ +int rows, columns; +calculate_rows_and_columns(term, available_height, available_width, +&rows, &columns); -/* Set rows and columns size */ -term->term_height = rows; -term->term_width = columns; +/* Calculate height & width within predefined maximum of rows and columns */ +int adjusted_height = height; +int adjusted_width = width; +calculate_adjusted_height_and_width(term, rows, columns, +&adjusted_height, &adjusted_width); /* Set pixel size */ -term->height = available_height; -term->width = available_width; +term->width = adjusted_width; +term->height = adjusted_height; Review Comment: Would you be happy with the changes from f6fcd357616b517eaf19f41c77538704f5a44e24, but with the set of common calls to those functions instead called by some sort of more abstract setter for `outer_height` and `outer_width`? -- 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]
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #385: GUACAMOLE-1622: Restructured code to resolve scrollbar resizing bug.
mike-jumper commented on code in PR #385: URL: https://github.com/apache/guacamole-server/pull/385#discussion_r919519381 ## src/terminal/terminal.c: ## @@ -500,26 +528,33 @@ guac_terminal* guac_terminal_create(guac_client* client, term->clipboard = guac_common_clipboard_alloc(); term->disable_copy = options->disable_copy; -/* Calculate available text display area by character size */ -int rows, columns; -calculate_rows_and_columns(term, height, width, &rows, &columns); +/* Set size of available screen area */ +term->outer_width = width; +term->outer_height = height; -/* Calculate available text display area in pixels */ +/* Calculate available display area of text in pixels */ int available_height, available_width; -calculate_height_and_width(term, rows, columns, -&available_height, &available_width); +calculate_available_height_and_width(term, +height, width, &available_height, &available_width); -/* Set size of available screen area */ -term->outer_height = height; -term->outer_width = width; +/* Calculate available display area of text by character size */ +int rows, columns; +calculate_rows_and_columns(term, available_height, available_width, +&rows, &columns); -/* Set rows and columns size */ -term->term_height = rows; -term->term_width = columns; +/* Calculate height & width within predefined maximum of rows and columns */ +int adjusted_height = height; +int adjusted_width = width; +calculate_adjusted_height_and_width(term, rows, columns, +&adjusted_height, &adjusted_width); /* Set pixel size */ -term->height = available_height; -term->width = available_width; +term->width = adjusted_width; +term->height = adjusted_height; Review Comment: This body of code is now identical to its counterpart in `guac_terminal_resize()`: https://github.com/apache/guacamole-server/blob/f6fcd357616b517eaf19f41c77538704f5a44e24/src/terminal/terminal.c#L1497-L1519 Can these not be rolled into a single, abstract function? -- 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]
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #385: GUACAMOLE-1622: Restructured code to resolve scrollbar resizing bug.
mike-jumper commented on code in PR #385:
URL: https://github.com/apache/guacamole-server/pull/385#discussion_r917172055
##
src/terminal/terminal.c:
##
@@ -1458,22 +1428,38 @@ int guac_terminal_resize(guac_terminal* terminal, int
width, int height) {
/* Acquire exclusive access to terminal */
guac_terminal_lock(terminal);
-/* Calculate available text display area by character size */
-int rows, columns;
-calculate_rows_and_columns(terminal, height, width, &rows, &columns);
+ /* Set size of available screen area */
Review Comment:
Looks like a space was accidentally removed here (indentation is now
misaligned).
##
src/terminal/terminal.c:
##
@@ -1458,22 +1428,38 @@ int guac_terminal_resize(guac_terminal* terminal, int
width, int height) {
/* Acquire exclusive access to terminal */
guac_terminal_lock(terminal);
-/* Calculate available text display area by character size */
-int rows, columns;
-calculate_rows_and_columns(terminal, height, width, &rows, &columns);
+ /* Set size of available screen area */
+terminal->outer_width = width;
+terminal->outer_height = height;
-/* Calculate available text display area in pixels */
+/* Calculate available display area */
int available_height, available_width;
-calculate_height_and_width(terminal, rows, columns,
-&available_height, &available_width);
+calculate_available_height_and_width(terminal,
+height, width, &available_height, &available_width);
-/* Set size of available screen area */
-terminal->outer_height = height;
-terminal->outer_width = width;
+/* Calculate dimensions */
+int rows= available_height / display->char_height;
+int columns = available_width / display->char_width;
-/* Set pixel size */
-terminal->height = available_height;
-terminal->width = available_width;
+int margin = terminal->display->margin;
+
+/* Keep height within predefined maximum */
+if (rows > GUAC_TERMINAL_MAX_ROWS) {
+rows = GUAC_TERMINAL_MAX_ROWS;
+available_height = rows * display->char_height;
+height = available_height + 2 * margin;
+}
+
+/* Keep width within predefined maximum */
+if (columns > GUAC_TERMINAL_MAX_COLUMNS) {
+columns = GUAC_TERMINAL_MAX_COLUMNS;
+available_width = columns * display->char_width;
+width = available_width + GUAC_TERMINAL_SCROLLBAR_WIDTH + 2 * margin;
+}
Review Comment:
This used to be within `calculate_rows_and_columns()`, avoiding duplication
and ensuring bounds are always enforced. Why did this need to be removed from
`calculate_rows_and_columns()` and re-duplicated?
--
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]
