mike-jumper commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r898402988
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,65 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in pixels within the terminal
+ * and set those values on the terminal struct.
+ * Then use the pixel height and width to calculate the available height and
+ * width in characters and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ * This function requires outer_width, outer_height,
+ * display->margin, display->char_height, and display->char_width
+ * to be set on terminal in order to calculate available dimensions.
Review Comment:
> The terminal provides character width and height for calculations. This
function requires ...
That may be the case, but I think this should be written from the
perspective of the caller, not from the perspective of the implementation.
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,65 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in pixels within the terminal
+ * and set those values on the terminal struct.
+ * Then use the pixel height and width to calculate the available height and
+ * width in characters and store the results in the pointer arguments.
Review Comment:
Please reflow to match the style of the other comments/docs.
```
/**
* Clearly not a deal-breaker, but it's a bit odd to add a newline between
* sentences, even though not necessary to wrap.
* Then finish it off with a sentence fragment that maybe should either be
part
* of that first sentence or omitted entirely as an internal implementation
* detail.
*/
```
;)
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,65 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in pixels within the terminal
+ * and set those values on the terminal struct.
+ * Then use the pixel height and width to calculate the available height and
+ * width in characters and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ * This function requires outer_width, outer_height,
+ * display->margin, display->char_height, and display->char_width
+ * to be set on terminal in order to calculate available dimensions.
+ *
+ * @param columns
+ * Set with the available width for text of the terminal, by column count.
+ *
+ * @param rows
+ * Set with the available height for text of the terminal, by row count.
+ */
+static void calculate_and_save_available_dimensions(guac_terminal* term,
+ int* columns, int* rows) {
Review Comment:
The way this function is used still strikes me as odd. For example:
```
/* Set available screen area on the terminal */
int rows, columns;
calculate_and_save_available_dimensions(term, &columns, &rows);
/* Set rows and columns size */
term->term_width = columns;
term->term_height = rows;
```
Here, it seems like `calculate_and_save_available_dimensions()` could/should
definitely just be a setter that abstracts away the logic of calculating the
available space.
Similarly for something like:
```
/* Set available screen area on the terminal */
int rows, columns;
calculate_and_save_available_dimensions(terminal, &columns, &rows);
/* Resize default layer to given pixel dimensions */
guac_terminal_repaint_default_layer(terminal, client->socket);
/* Resize terminal if row/column dimensions have changed */
if (columns != terminal->term_width || rows != terminal->term_height) {
...
```
if the function were to return whether the dimensions actually changed, then
this could be abstracted and cleaned up.
Am I missing something?
I really think this would be far better implemented as something like:
```
static int guac_terminal_set_outer_dimensions(guac_terminal* term, int
width, int height)
```
where width/height are the dimensions in pixels (which this function would
assign to `outer_width` and `outer_height`), and a non-zero value would be
returned if the number of rows/colums actually changed.
I think this would be far better conceptualized as an abstraction that
allows the caller to set the outer width/height and not have to worry about
manually calculating the other properties, rather than a non-abstract piece of
shared utility code. ie:
```
int dimensions_changed = guac_terminal_set_outer_dimensions(term, width,
height); /* Tada */
```
rather than:
```
term->outer_width = width;
term->outer_height = height;
int rows, columns;
make_it_so(term, &rows, &columns);
if (manual comparison against rows and columns) { ... }
```
I'm not a big fan of an approach that involves pumping values into a struct
and then calling a `do_it()` function to perform the calculations necessary to
make the rest of the struct correct, at least not if it can be avoided.
--
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]
