You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/06/08 21:14:10 UTC
[GitHub] [guacamole-server] aleitner opened a new pull request, #383: GUACAMOLE-1622: Added margins to ssh sessions.
aleitner opened a new pull request, #383:
URL: https://github.com/apache/guacamole-server/pull/383
* Offset default layer by new `GUAC_TERMINAL_OFFSET` constant.
* Take new constant `GUAC_TERMINAL_MARGINS` into account when calculating available height and width of the terminal
* Fixed "Reraw" typo to "Redraw"
* Made `get_available_dimensions` function for duplicated logic
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r896017364
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,68 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * 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 aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term,
Review Comment:
At this point, having the function called `get_...` is pretty misleading - usually you'd expect a getter to return a value, which this doesn't do.
Maybe `calculate_and_save_...` or similar might be better?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r893893199
##########
src/terminal/terminal/display.h:
##########
@@ -123,6 +128,12 @@ typedef struct guac_terminal_display {
*/
int height;
+ /**
+ * The size of margins between the console text and the border in pixels.
+ *
Review Comment:
Looks like there's an empty line here that could be removed.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894779582
##########
src/terminal/terminal/display.h:
##########
@@ -44,6 +44,17 @@
*/
#define GUAC_TERMINAL_MAX_CHAR_WIDTH 2
+/**
+ * The size of margins between the console text and the border in mm.
+ */
+#define GUAC_TERMINAL_MARGINS 2
Review Comment:
I figured if we wanted to make it adjustable we'd remove the `GUAC_TERMINAL_MARGINS` and add it to the options or something, and it's much easier to think of adjusting in terms of mm rather than 0.0393701 inches
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner merged pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner merged PR #383:
URL: https://github.com/apache/guacamole-server/pull/383
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r892875496
##########
src/terminal/terminal.c:
##########
@@ -511,6 +494,38 @@ guac_terminal* guac_terminal_create(guac_client* client,
}
+void get_available_dimensions(guac_terminal* term, int width, int height, int* aw, int* ah, int* columns, int* rows) {
Review Comment:
It's a minor thing - but to match styling, there should be a new line between the function definition and the enclosed block.
Additionally, if possible, please wrap code at 80 characters. We generally use 8 spaces as an indent for line continuations.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894779582
##########
src/terminal/terminal/display.h:
##########
@@ -44,6 +44,17 @@
*/
#define GUAC_TERMINAL_MAX_CHAR_WIDTH 2
+/**
+ * The size of margins between the console text and the border in mm.
+ */
+#define GUAC_TERMINAL_MARGINS 2
Review Comment:
I figured if we wanted to make it adjustable we'd remove the `GUAC_TERMINAL_MARGINS` and add it to the options are something, and it's much easier to think of adjusting in terms of mm rather than 0.0393701 inches
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
necouchman commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894450130
##########
src/terminal/display.c:
##########
@@ -1020,7 +1035,11 @@ int guac_terminal_display_set_font(guac_terminal_display* display,
if (new_width != display->width || new_height != display->height)
guac_terminal_display_resize(display, new_width, new_height);
+
return 0;
}
+int get_margin_by_dpi(int dpi) {
+ return dpi * GUAC_TERMINAL_MARGINS / 25.4;
Review Comment:
Maybe `25.4` should be defined as a constant somewhere, which would also help me understand why the value is chosen :-).
##########
src/terminal/terminal/terminal.h:
##########
@@ -279,6 +279,34 @@ guac_terminal_options* guac_terminal_options_create(
*/
void guac_terminal_free(guac_terminal* term);
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for caluclations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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.
+ */
+void get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows);
+
Review Comment:
Looks like this is only used in `terminal.c` - is there a reason to make it available in the header, or should it be a `static` function limited to `terminal.c`?
##########
src/terminal/terminal/display.h:
##########
@@ -369,5 +380,16 @@ void guac_terminal_display_clear_select(guac_terminal_display* display);
int guac_terminal_display_set_font(guac_terminal_display* display,
const char* font_name, int font_size, int dpi);
+/**
+ * Calculate the size of margins around the terminal based on DPI.
+ *
+ * @param dpi
+ * The resolution of the display in DPI.
+ *
+ * @return
+ * Calculated size of margin in pixels
+ */
+int get_margin_by_dpi(int dpi);
+
Review Comment:
I don't see this function used anywhere outside of `display.c` - is there some reason it should be made available outside of the file, or perhaps it should be a `static inc` within `display.c`?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894849128
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
1) It looks like both locations where `get_available_dimensions` is called we end up setting `outer_width` and `outer_height` to the same `width` and `height` that get passed in to `get_available_dimensions`. I can update the function to just use the `terminal` for those values then.
2) I initially had the function as `set_available_dimensions` but in the `guac_terminal_resize` function we needed access to both the original dimensions and the new dimensions from `set_available_dimensions` when the `__guac_terminal_resize` function is called so to prevent rewriting a significant amount of code inside `__guac_terminal_resize` I made the `set_available_dimensions` function a get rather than a set.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r892892734
##########
src/terminal/terminal/terminal.h:
##########
@@ -97,6 +97,16 @@
*/
#define GUAC_TERMINAL_WHEEL_SCROLL_AMOUNT 3
+/**
+ * The size of margins between the console text and the border in pixels.
+ */
+#define GUAC_TERMINAL_MARGINS 16
Review Comment:
This and `GUAC_TERMINAL_OFFSET` will need to be in some pixel-independent units to allow the margins to scale for different display resolutions. Not all screens will have the same DPI, and the terminal font rendering takes this into account.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r899251626
##########
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:
Okay instead of cramming everything into the `calculate_and_save_available_dimensions` function I separated the logic into single responsibility functions. The purpose of each function should be much more straightforward
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894792295
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
Is there any case where this would not be used to calculate available dimensions from `outer_width` and `outer_height`, and is there any case where this is not ultimately used to assign the resulting values back to the `guac_terminal`?
If not, perhaps this would be better as `set_available_dimensions(term, width, height)`?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
necouchman commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r902799098
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,88 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in characters for text display in
+ * the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param height
+ * The outer height of the terminal, in pixels.
+ *
+ * @param width
+ * The outer width of the terminal, in pixels.
+ *
+ * @param rows
+ * Calculated height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * Calculated width of the terminal for text display, in characters.
+ *
+ */
+static void calculate_rows_and_columns(guac_terminal* term,
+ int height, int width, int *rows, int *columns) {
+
+ int margin = term->display->margin;
+ int char_width = term->display->char_width;
+ int char_height = term->display->char_height;
+
+ /* Calculate available display area */
+ int available_width = width - GUAC_TERMINAL_SCROLLBAR_WIDTH - 2 * margin;
+ if (available_width < 0)
+ available_width = 0;
+
+ int available_height = height - 2 * margin;
+ if (available_height < 0)
+ available_height = 0;
+
+ /* Calculate dimensions */
+ *rows = available_height / char_height;
+ *columns = available_width / char_width;
+
+ /* Keep height within predefined maximum */
+ if (*rows > GUAC_TERMINAL_MAX_ROWS) {
+ *rows = GUAC_TERMINAL_MAX_ROWS;
+ }
+
+ /* Keep width within predefined maximum */
+ if (*columns > GUAC_TERMINAL_MAX_COLUMNS) {
+ *columns = GUAC_TERMINAL_MAX_COLUMNS;
+ }
+}
+
+/**
+ * Calculate the available height and width in pixels of the terminal for text
+ * display in the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param rows
+ * The available height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * The available width of the terminal for text display, in characters.
+ *
+ * @param height
+ * Calculated available height of the terminal for text display, in pixels.
+ *
+ * @param width
+ * Calculated available width of the terminal for text display, in pixels.
Review Comment:
Should these be noted as "Pointer to..." as well?
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,88 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in characters for text display in
+ * the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param height
+ * The outer height of the terminal, in pixels.
+ *
+ * @param width
+ * The outer width of the terminal, in pixels.
+ *
+ * @param rows
+ * Calculated height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * Calculated width of the terminal for text display, in characters.
Review Comment:
Should these be noted as "pointer to..."?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r898189464
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
good call
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r892873830
##########
src/terminal/display.c:
##########
@@ -229,6 +229,9 @@ guac_terminal_display* guac_terminal_display_alloc(guac_client* client,
guac_protocol_send_move(client->socket, display->select_layer,
display->display_layer, 0, 0, 0);
+ guac_protocol_send_move(client->socket, display->display_layer,
Review Comment:
A comment here about what this is for would be pretty chill.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894849128
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
1) It looks like both locations where `get_available_dimensions` is called we end up setting `outer_width` and `outer_height` to the same `width` and `height` that get passed in to `get_available_dimensions`. I can update the function to just use the `terminal` for those values then.
2) I initially had the function as `set_available_dimensions` but in the `guac_terminal_resize` function we needed access to both the original dimensions and the new dimensions from `set_available_dimensions`. The reason is that `__guac_terminal_resize` makes some checks assuming the `terminal->width` is the width before resize, and sets the resized width on the terminal at the end of the function. So to prevent rewriting a significant amount of code inside `__guac_terminal_resize` I made the `set_available_dimensions` function a get rather than a set.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r893893308
##########
src/terminal/terminal/display.h:
##########
@@ -369,5 +380,16 @@ void guac_terminal_display_clear_select(guac_terminal_display* display);
int guac_terminal_display_set_font(guac_terminal_display* display,
const char* font_name, int font_size, int dpi);
+/**
+ * Calculate the size of margins around the terminal based on DPI.
+ *
+ * @param dpi
+ * The resolution of the display in DPI.
+ *
+ * @return
+ * Calculated size of margin in pixels
Review Comment:
Period please.
##########
src/terminal/terminal/terminal.h:
##########
@@ -279,6 +279,34 @@ guac_terminal_options* guac_terminal_options_create(
*/
void guac_terminal_free(guac_terminal* term);
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for caluclations.
Review Comment:
Typo: `caluclations`
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r898203955
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,68 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * 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 aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term,
Review Comment:
Thanks! That's a lot simpler - though now that the function updates the terminal struct directly, the block comment for the function should actually say that - reading the comment as it exists now, you'd think it only updates the passed-in pointers.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r896020901
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
I see that there is special handling for `columns` and `rows` for resizes, but in both cases where `get_available_dimensions` is called, the next thing that's done is setting the terminal height and width using `aw` and `ah`.
Why not skip setting the pointers and just set those directly on the terminal?
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894783322
##########
src/terminal/terminal/display.h:
##########
@@ -44,6 +44,17 @@
*/
#define GUAC_TERMINAL_MAX_CHAR_WIDTH 2
+/**
+ * The size of margins between the console text and the border in mm.
+ */
+#define GUAC_TERMINAL_MARGINS 2
Review Comment:
Makes sense to me.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894849128
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,67 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available space within the terminal
+ * and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param width
+ * The width of the terminal, in pixels.
+ *
+ * @param height
+ * The height of the terminal, in pixels.
+ *
+ * @param aw
+ * Set with the available width for text of the terminal, in pixels.
+ *
+ * @param ah
+ * Set with the available height for text of the terminal, in pixels.
+ *
+ * @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 get_available_dimensions(guac_terminal* term, int width, int height,
+ int* aw, int* ah, int* columns, int* rows) {
Review Comment:
1) It looks like both locations where `get_available_dimensions` is called we end up setting `outer_width` and `outer_height` to the same `width` and `height` that get passed in to `get_available_dimensions`. I can update the function to just use the `terminal` for those values then.
2) I initially had the function as `set_available_dimensions` but in the `guac_terminal_resize` function we needed access to both the original dimensions and the new dimensions from `set_available_dimensions`. The reason is that `__guac_terminal_resize` makes some checks assuming the `terminal->width` is the width before resize, and commits the resized width at the end of the function. So to prevent rewriting a significant amount of code inside `__guac_terminal_resize` I made the `set_available_dimensions` function a get rather than a set.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r892879469
##########
src/terminal/terminal.c:
##########
@@ -425,27 +416,19 @@ guac_terminal* guac_terminal_create(guac_client* client,
term->clipboard = guac_common_clipboard_alloc();
term->disable_copy = options->disable_copy;
- /* Calculate character size */
- int rows = height / term->display->char_height;
- int columns = available_width / term->display->char_width;
-
- /* Keep height within predefined maximum */
- if (rows > GUAC_TERMINAL_MAX_ROWS) {
- rows = GUAC_TERMINAL_MAX_ROWS;
- height = rows * term->display->char_height;
- }
+ /* Set available screen area on the terminal */
+ int available_width, available_height, rows, columns;
+ get_available_dimensions(term, width, height, &available_width, &available_height, &columns, &rows);
- /* Keep width within predefined maximum */
- if (columns > GUAC_TERMINAL_MAX_COLUMNS) {
- columns = GUAC_TERMINAL_MAX_COLUMNS;
- available_width = columns * term->display->char_width;
- width = available_width + GUAC_TERMINAL_SCROLLBAR_WIDTH;
- }
+ /* Set size of available screen area */
+ term->outer_width = width;
+ term->outer_height = height;
/* Set pixel size */
- term->width = width;
- term->height = height;
+ term->width = available_width;
+ term->height = available_height;
+ /* set rows and columns size */
Review Comment:
Uppercase please
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] aleitner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
aleitner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r902833372
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,88 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in characters for text display in
+ * the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param height
+ * The outer height of the terminal, in pixels.
+ *
+ * @param width
+ * The outer width of the terminal, in pixels.
+ *
+ * @param rows
+ * Calculated height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * Calculated width of the terminal for text display, in characters.
+ *
+ */
+static void calculate_rows_and_columns(guac_terminal* term,
+ int height, int width, int *rows, int *columns) {
+
+ int margin = term->display->margin;
+ int char_width = term->display->char_width;
+ int char_height = term->display->char_height;
+
+ /* Calculate available display area */
+ int available_width = width - GUAC_TERMINAL_SCROLLBAR_WIDTH - 2 * margin;
+ if (available_width < 0)
+ available_width = 0;
+
+ int available_height = height - 2 * margin;
+ if (available_height < 0)
+ available_height = 0;
+
+ /* Calculate dimensions */
+ *rows = available_height / char_height;
+ *columns = available_width / char_width;
+
+ /* Keep height within predefined maximum */
+ if (*rows > GUAC_TERMINAL_MAX_ROWS) {
+ *rows = GUAC_TERMINAL_MAX_ROWS;
+ }
+
+ /* Keep width within predefined maximum */
+ if (*columns > GUAC_TERMINAL_MAX_COLUMNS) {
+ *columns = GUAC_TERMINAL_MAX_COLUMNS;
+ }
+}
+
+/**
+ * Calculate the available height and width in pixels of the terminal for text
+ * display in the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param rows
+ * The available height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * The available width of the terminal for text display, in characters.
+ *
+ * @param height
+ * Calculated available height of the terminal for text display, in pixels.
+ *
+ * @param width
+ * Calculated available width of the terminal for text display, in pixels.
Review Comment:
done
##########
src/terminal/terminal.c:
##########
@@ -335,6 +335,88 @@ guac_terminal_options* guac_terminal_options_create(
return options;
}
+/**
+ * Calculate the available height and width in characters for text display in
+ * the terminal and store the results in the pointer arguments.
+ *
+ * @param terminal
+ * The terminal provides character width and height for calculations.
+ *
+ * @param height
+ * The outer height of the terminal, in pixels.
+ *
+ * @param width
+ * The outer width of the terminal, in pixels.
+ *
+ * @param rows
+ * Calculated height of the terminal for text display, in characters.
+ *
+ * @param columns
+ * Calculated width of the terminal for text display, in characters.
Review Comment:
done
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r892877371
##########
src/terminal/terminal/terminal.h:
##########
@@ -279,6 +289,33 @@ guac_terminal_options* guac_terminal_options_create(
*/
void guac_terminal_free(guac_terminal* term);
+/**
+ * Calculate the available space within the terminal
Review Comment:
Sorry to be such a stickler about style, but block comments should end in periods.
Also, a little bit more detail about how the function works is useful to understand the function at a glance. In this case, it would be good to mention something about the calculation results being stuffed into the provided int* pointers.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894751380
##########
src/terminal/terminal/display.h:
##########
@@ -44,6 +44,17 @@
*/
#define GUAC_TERMINAL_MAX_CHAR_WIDTH 2
+/**
+ * The size of margins between the console text and the border in mm.
+ */
+#define GUAC_TERMINAL_MARGINS 2
Review Comment:
Why not just have a single constant for the margin in inches? I love the metric system as much as anyone else, but it seems a bit overly complicated to use 2 constants to convert mm to inches every time, rather than just working with inches directly.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on a diff in pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#discussion_r894758383
##########
src/terminal/terminal/display.h:
##########
@@ -44,6 +44,17 @@
*/
#define GUAC_TERMINAL_MAX_CHAR_WIDTH 2
+/**
+ * The size of margins between the console text and the border in mm.
+ */
+#define GUAC_TERMINAL_MARGINS 2
Review Comment:
Note: this isn't a demand for a change; I'm just curious. If you really prefer it this way, I'm ok with keeping it.
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] jmuehlner commented on pull request #383: GUACAMOLE-1622: Added margins to ssh sessions.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on PR #383:
URL: https://github.com/apache/guacamole-server/pull/383#issuecomment-1163345603
LGTM!
--
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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org