You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by darchons <gi...@git.apache.org> on 2018/01/10 04:36:07 UTC
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
GitHub user darchons opened a pull request:
https://github.com/apache/guacamole-server/pull/132
GUACAMOLE-470: Make terminal colors configurable through "color-scheme" parameter
Change the "color-scheme" parameter into a series of semicolon-separated name-value pairs, so that individual terminal colors can be configured. For example, this configuration changes the terminal to use a "solarized light" color scheme:
<param name="color-scheme">
background: rgb:fd/f6/e3;
foreground: rgb:65/7b/83;
color0: rgb:07/36/42;
color8: rgb:00/2b/36;
color1: rgb:dc/32/2f;
color9: rgb:cb/4b/16;
color2: rgb:85/99/00;
color10: rgb:58/6e/75;
color3: rgb:b5/89/00;
color11: rgb:65/7b/83;
color4: rgb:26/8b/d2;
color12: rgb:83/94/96;
color5: rgb:d3/36/82;
color13: rgb:6c/71/c4;
color6: rgb:2a/a1/98;
color14: rgb:93/a1/a1;
color7: rgb:ee/e8/d5;
color15: rgb:fd/f6/e3;
</param>
Backward compatibility is preserved by translating previously supported values into the corresponding new format (e.g. `gray-black` is translated to `foreground: color7; background: color0`).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/darchons/guacamole-server guac-470-configurable-terminal-colors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/132.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #132
----
commit 4154fac228d0ae855117ce44cf6ef990b9abc3bf
Author: Jim Chen <nc...@...>
Date: 2018-01-10T03:08:13Z
GUACAMOLE-470: Set palette index for parsed RGB colors.
Parsed RGB colors do not correspond to any palette entry, so set the
palette index to -1.
commit cdaceb0d94a412de177dd3b19c6ceb6b4b6fd7fb
Author: Jim Chen <nc...@...>
Date: 2018-01-10T03:14:56Z
GUACAMOLE-470: Add support for configurable default palette.
Add support for configuring a default palette for a terminal display.
When the default palette is specified during display creation, that
palette is used instead of GUAC_TERMINAL_INITIAL_PALETTE when resetting
the display palette.
commit 002dac47004086a4ede18270c77e8124c45fbf01
Author: Jim Chen <nc...@...>
Date: 2018-01-10T03:21:02Z
GUACAMOLE-470: Support configurable colors in color-scheme parameter.
Add support for configuring individual colors in the color-scheme
parameter, by parsing the parameter content into name-value pairs.
Backward compatibility is preserved by translating previously supported
values into corresponding new values.
----
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161150916
--- Diff: src/terminal/terminal.c ---
@@ -255,52 +257,203 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme) {
+/**
+ * Compare a non-null-terminated string spanning token_start to token_end, with
+ * a null-terminating string at literal.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* token_start,
+ const char* token_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, token_start, token_end - token_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | token_end - token_start |, so if the two are equal, literal should
+ * have its null-terminator at | token_end - token_start |. */
+ return (int) (unsigned char) literal[token_end - token_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+/**
+ * Parse a color-scheme configuration string, and return specified
+ * foreground/background colors and color palette.
+ *
+ * @param client
+ *
+ * @param color_scheme
+ * A semicolon-separated list of name-value pairs, i.e.
+ * "<name>: <value> [; <name>: <value> [; ...]]".
+ * For example, "color2: rgb:cc/33/22; background: color5".
--- End diff --
Is the idea behind this format to model the resource names used by xterm?
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161141682
--- Diff: src/terminal/terminal.c ---
@@ -255,52 +257,203 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme) {
+/**
+ * Compare a non-null-terminated string spanning token_start to token_end, with
+ * a null-terminating string at literal.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* token_start,
+ const char* token_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, token_start, token_end - token_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | token_end - token_start |, so if the two are equal, literal should
+ * have its null-terminator at | token_end - token_start |. */
+ return (int) (unsigned char) literal[token_end - token_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+/**
+ * Parse a color-scheme configuration string, and return specified
+ * foreground/background colors and color palette.
+ *
+ * @param client
+ *
+ * @param color_scheme
+ * A semicolon-separated list of name-value pairs, i.e.
+ * "<name>: <value> [; <name>: <value> [; ...]]".
+ * For example, "color2: rgb:cc/33/22; background: color5".
+ *
+ * @param foreground
+ * (out) Parsed foreground color.
--- End diff --
There's a specific Doxygen syntax for output parameters: https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdparam
I've only ever noted that a parameter is used for output through admittedly verbose prose, but if you want to annotate the output quality via something like `(out)`, it looks like things would be correctly annotated as:
```
@param[out] foreground
...
```
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161149994
--- Diff: src/terminal/terminal.c ---
@@ -255,52 +257,203 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme) {
+/**
+ * Compare a non-null-terminated string spanning token_start to token_end, with
+ * a null-terminating string at literal.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* token_start,
+ const char* token_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, token_start, token_end - token_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | token_end - token_start |, so if the two are equal, literal should
+ * have its null-terminator at | token_end - token_start |. */
+ return (int) (unsigned char) literal[token_end - token_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+/**
+ * Parse a color-scheme configuration string, and return specified
+ * foreground/background colors and color palette.
+ *
+ * @param client
+ *
+ * @param color_scheme
+ * A semicolon-separated list of name-value pairs, i.e.
+ * "<name>: <value> [; <name>: <value> [; ...]]".
+ * For example, "color2: rgb:cc/33/22; background: color5".
+ *
+ * @param foreground
+ * (out) Parsed foreground color.
+ *
+ * @param background
+ * (out) Parsed background color.
+ *
+ * @param palette
+ * (out) Parsed color palette. Null is returned if color_scheme does not
+ * require a custom palette. Otherwise, malloc() is used to allocate a new
+ * palette, which must be freed manually by the calling code.
+ */
+static void guac_terminal_parse_color_scheme(guac_client* client,
+ const char* color_scheme, guac_terminal_color* foreground,
+ guac_terminal_color* background,
+ const guac_terminal_color (**palette)[256]) {
+
+ /* Set default gray-black color scheme and initial palette. */
+ *foreground = GUAC_TERMINAL_INITIAL_PALETTE[GUAC_TERMINAL_COLOR_GRAY];
+ *background = GUAC_TERMINAL_INITIAL_PALETTE[GUAC_TERMINAL_COLOR_BLACK];
+ *palette = NULL;
+
+ const guac_terminal_color (*ref_palette)[256] = &GUAC_TERMINAL_INITIAL_PALETTE;
+ guac_terminal_color (*new_palette)[256] = NULL;
+
+ /* Pointer to the next token to parse. */
+ const char* next_token = color_scheme;
+
+ /* True if expecting the current token to be a value; false if expecting a name. */
+ bool expect_value = false;
+
+ /* True if current token should be skipped. */
+ bool skip_token = false;
+
+ /* Target to place the parsed color value into. */
+ guac_terminal_color* value_target = NULL;
+
+ /* Terminator for the current token. */
+ char token_terminator = ';';
+
+ while (next_token) {
+ /* Use token_start and token_end to denote the current token. */
+ const char* token_start = next_token;
+ /* Strip leading spaces in current token. */
+ while (isspace(*token_start))
+ token_start++;
+
+ /* Find where this token ends */
+ const char* token_end = strpbrk(token_start, ":;");
+ if (token_end) {
+ next_token = token_end + 1;
+ token_terminator = *token_end;
+ }
+ else {
+ /* We've reached the end of the string. */
+ next_token = NULL;
+ token_end = token_start + strlen(token_start);
+ token_terminator = ';';
+ }
+ /* Strip trailing spaces in current token. */
+ while (token_end > token_start && isspace(*(token_end - 1)))
+ token_end--;
+
+ if (token_start == token_end) {
+ if (expect_value) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Empty color value at position %d.",
+ token_start - color_scheme);
+ return;
+ }
+ }
- /* If invalid, default to "gray-black" */
- else {
- guac_client_log(client, GUAC_LOG_WARNING,
- "Invalid color scheme: \"%s\". Defaulting to \"gray-black\".",
- color_scheme);
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+ else if (skip_token) {
+ expect_value = false;
+ skip_token = false;
+ }
+
+ else if (expect_value) {
+ expect_value = false;
+
+ /* Parse color<n> value. */
+ int index = -1;
+ if (sscanf(token_start, GUAC_TERMINAL_SCHEME_NUMBERED "%d", &index) &&
+ index >= 0 && index <= 255) {
+ *value_target = (*ref_palette)[index];
+ }
+
+ /* Parse X11 value. */
+ else if (!guac_terminal_xparsecolor(token_start, value_target)) {
+ if (!guac_terminal_color_scheme_compare_token(token_start,
+ token_end, "rgb")) {
+ /* This value was a "rgb:R/G/B" value, so the next token
+ * will be the "R/G/B" part. Skip processing that part. */
+ expect_value = true;
+ skip_token = true;
+ }
+ }
+
+ else {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Invalid color value: \"%.*s\".",
+ token_end - token_start, token_start);
+ return;
+ }
+ }
+
+ /* Expecting a name. */
+ else if (!guac_terminal_color_scheme_compare_token(
+ token_start, token_end, GUAC_TERMINAL_SCHEME_FOREGROUND)) {
+ value_target = foreground;
+ expect_value = true;
+ }
+
+ else if (!guac_terminal_color_scheme_compare_token(
+ token_start, token_end, GUAC_TERMINAL_SCHEME_BACKGROUND)) {
+ value_target = background;
+ expect_value = true;
+ }
+
+ else {
+ /* Parse color<n> value. */
+ int index = -1;
+ if (sscanf(token_start, GUAC_TERMINAL_SCHEME_NUMBERED "%d", &index) &&
+ index >= 0 && index <= 255) {
+
+ /* Allocate a new palette now that we are mutating it. */
+ if (!new_palette) {
+ const size_t size = sizeof(guac_terminal_color[256]);
+ new_palette = (guac_terminal_color(*)[256]) malloc(size);
--- End diff --
Why conditionally allocate space, copy, etc.? It seems like the caller should provide initialized space (presumably a copy of `GUAC_TERMINAL_INITIAL_PALETTE`), with the scheme parser mutating that provided space as specified, no?
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by darchons <gi...@git.apache.org>.
Github user darchons commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r188500441
--- Diff: src/terminal/display.c ---
@@ -246,7 +246,8 @@ int __guac_terminal_set(guac_terminal_display* display, int row, int col, int co
guac_terminal_display* guac_terminal_display_alloc(guac_client* client,
const char* font_name, int font_size, int dpi,
- guac_terminal_color* foreground, guac_terminal_color* background) {
+ guac_terminal_color* foreground, guac_terminal_color* background,
+ const guac_terminal_color (*palette)[256]) {
--- End diff --
I left this the way it is, because foreground/background are currently part of the character attributes, so I wasn't sure if I should pull them out into a new struct with the palette.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r189474561
--- Diff: src/terminal/terminal.c ---
@@ -255,53 +260,252 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme,
- const int backspace) {
+/**
+ * Compare a non-null-terminated string to a null-terminated literal, in the
+ * same manner as strcmp().
+ *
+ * @param str_start
+ * Start of the non-null-terminated string.
+ *
+ * @param str_end
+ * End of the non-null-terminated string, after the last character.
+ *
+ * @param literal
+ * The null-terminated literal to compare against.
+ *
+ * @return
+ * Zero if the two strings are equal and non-zero otherwise.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* str_start,
+ const char* str_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, str_start, str_end - str_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | str_end - str_start |, so if the two are equal, literal should
+ * have its null-terminator at | str_end - str_start |. */
+ return (int) (unsigned char) literal[str_end - str_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+/**
+ * Strip the leading and trailing spaces of a bounded string.
+ *
+ * @param[in,out] str_start
+ * Address of a pointer to the start of the string. On return, the pointer
+ * is advanced to after any leading spaces.
+ *
+ * @param[in,out] str_end
+ * Address of a pointer to the end of the string, after the last character.
+ * On return, the pointer is moved back to before any trailing spaces.
+ */
+static void guac_terminal_color_scheme_strip_spaces(const char** str_start,
+ const char** str_end) {
+
+ /* Strip leading spaces. */
+ while (*str_start < *str_end && isspace(**str_start))
+ (*str_start)++;
+
+ /* Strip trailing spaces. */
+ while (*str_end > *str_start && isspace(*(*str_end - 1)))
+ (*str_end)--;
+}
+
+/**
+ * Parse the name part of the name-value pair within the color-scheme
+ * configuration.
+ *
+ * @param client
+ *
+ * @param name_start
+ * Start of the name string.
+ *
+ * @param name_end
+ * End of the name string, after the last character.
+ *
+ * @param foreground
+ * Pointer to the foreground color.
+ *
+ * @param background
+ * Pointer to the background color.
+ *
+ * @param palette
+ * Pointer to the palette array.
+ *
+ * @param[out] target
+ * On return, pointer to the color struct that corresponds to the name.
+ *
+ * @return
+ * Zero if successful or non-zero otherwise.
+ */
+static int guac_terminal_parse_color_scheme_name(guac_client* client,
+ const char* name_start, const char* name_end,
+ guac_terminal_color* foreground, guac_terminal_color* background,
+ guac_terminal_color (*palette)[256],
+ guac_terminal_color** target) {
+
+ guac_terminal_color_scheme_strip_spaces(&name_start, &name_end);
+
+ if (!guac_terminal_color_scheme_compare_token(
+ name_start, name_end, GUAC_TERMINAL_SCHEME_FOREGROUND)) {
+ *target = foreground;
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
+
+ if (!guac_terminal_color_scheme_compare_token(
+ name_start, name_end, GUAC_TERMINAL_SCHEME_BACKGROUND)) {
+ *target = background;
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+
+ /* Parse color<n> value. */
+ int index = -1;
+ if (sscanf(name_start, GUAC_TERMINAL_SCHEME_NUMBERED "%d", &index) &&
+ index >= 0 && index <= 255) {
+ *target = &(*palette)[index];
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Unknown color name: \"%.*s\".",
+ name_end - name_start, name_start);
+ return 1;
+}
+
+/**
+ * Parse the value part of the name-value pair within the color-scheme
+ * configuration.
+ *
+ * @param client
--- End diff --
Same here - please document this parameter.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161150191
--- Diff: src/terminal/display.c ---
@@ -246,7 +246,8 @@ int __guac_terminal_set(guac_terminal_display* display, int row, int col, int co
guac_terminal_display* guac_terminal_display_alloc(guac_client* client,
const char* font_name, int font_size, int dpi,
- guac_terminal_color* foreground, guac_terminal_color* background) {
+ guac_terminal_color* foreground, guac_terminal_color* background,
+ const guac_terminal_color (*palette)[256]) {
--- End diff --
It's beginning to feel like perhaps the foreground, background, and 256-color palette should be moved into their own struct.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-server/pull/132
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161148445
--- Diff: src/terminal/display.c ---
@@ -317,6 +319,9 @@ guac_terminal_display* guac_terminal_display_alloc(guac_client* client,
void guac_terminal_display_free(guac_terminal_display* display) {
+ /* Free default palette. */
+ free((void*) display->default_palette);
--- End diff --
Not necessarily a request for a change, but I am curious: is this necessary? This is the first I've seen an explicit typecast to `(void*)` for a call to `free()`.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r189474404
--- Diff: src/terminal/terminal.c ---
@@ -255,53 +260,252 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme,
- const int backspace) {
+/**
+ * Compare a non-null-terminated string to a null-terminated literal, in the
+ * same manner as strcmp().
+ *
+ * @param str_start
+ * Start of the non-null-terminated string.
+ *
+ * @param str_end
+ * End of the non-null-terminated string, after the last character.
+ *
+ * @param literal
+ * The null-terminated literal to compare against.
+ *
+ * @return
+ * Zero if the two strings are equal and non-zero otherwise.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* str_start,
+ const char* str_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, str_start, str_end - str_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | str_end - str_start |, so if the two are equal, literal should
+ * have its null-terminator at | str_end - str_start |. */
+ return (int) (unsigned char) literal[str_end - str_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+/**
+ * Strip the leading and trailing spaces of a bounded string.
+ *
+ * @param[in,out] str_start
+ * Address of a pointer to the start of the string. On return, the pointer
+ * is advanced to after any leading spaces.
+ *
+ * @param[in,out] str_end
+ * Address of a pointer to the end of the string, after the last character.
+ * On return, the pointer is moved back to before any trailing spaces.
+ */
+static void guac_terminal_color_scheme_strip_spaces(const char** str_start,
+ const char** str_end) {
+
+ /* Strip leading spaces. */
+ while (*str_start < *str_end && isspace(**str_start))
+ (*str_start)++;
+
+ /* Strip trailing spaces. */
+ while (*str_end > *str_start && isspace(*(*str_end - 1)))
+ (*str_end)--;
+}
+
+/**
+ * Parse the name part of the name-value pair within the color-scheme
+ * configuration.
+ *
+ * @param client
--- End diff --
Please document this parameter as well.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by darchons <gi...@git.apache.org>.
Github user darchons commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r188500611
--- Diff: src/terminal/terminal.c ---
@@ -255,52 +257,203 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme) {
+/**
+ * Compare a non-null-terminated string spanning token_start to token_end, with
+ * a null-terminating string at literal.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* token_start,
+ const char* token_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, token_start, token_end - token_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | token_end - token_start |, so if the two are equal, literal should
+ * have its null-terminator at | token_end - token_start |. */
+ return (int) (unsigned char) literal[token_end - token_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+/**
+ * Parse a color-scheme configuration string, and return specified
+ * foreground/background colors and color palette.
+ *
+ * @param client
+ *
+ * @param color_scheme
+ * A semicolon-separated list of name-value pairs, i.e.
+ * "<name>: <value> [; <name>: <value> [; ...]]".
+ * For example, "color2: rgb:cc/33/22; background: color5".
--- End diff --
Yep, makes it easier to transfer xterm colors to guacamole (also I was looking for a "standard" format to denote terminal colors, and xterm seems like a good de-facto standard versus the formats used by other terminal emulators)
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by darchons <gi...@git.apache.org>.
Github user darchons commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r188500310
--- Diff: src/terminal/display.c ---
@@ -317,6 +319,9 @@ guac_terminal_display* guac_terminal_display_alloc(guac_client* client,
void guac_terminal_display_free(guac_terminal_display* display) {
+ /* Free default palette. */
+ free((void*) display->default_palette);
--- End diff --
I made `default_palette` to have `const` elements to ensure correct usage elsewhere, but that meant we need to jump through this hoop when calling `free()`
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r189474807
--- Diff: src/terminal/terminal.c ---
@@ -255,53 +260,252 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme,
- const int backspace) {
+/**
+ * Compare a non-null-terminated string to a null-terminated literal, in the
+ * same manner as strcmp().
+ *
+ * @param str_start
+ * Start of the non-null-terminated string.
+ *
+ * @param str_end
+ * End of the non-null-terminated string, after the last character.
+ *
+ * @param literal
+ * The null-terminated literal to compare against.
+ *
+ * @return
+ * Zero if the two strings are equal and non-zero otherwise.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* str_start,
+ const char* str_end, const char* literal) {
- int default_foreground;
- int default_background;
+ const int result = strncmp(literal, str_start, str_end - str_start);
+ if (result != 0)
+ return result;
- /* Default to "gray-black" color scheme if no scheme provided */
- if (color_scheme == NULL || color_scheme[0] == '\0') {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
- }
+ /* At this point, literal is same length or longer than
+ * | str_end - str_start |, so if the two are equal, literal should
+ * have its null-terminator at | str_end - str_start |. */
+ return (int) (unsigned char) literal[str_end - str_start];
+}
- /* Otherwise, parse color scheme */
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GRAY_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+/**
+ * Strip the leading and trailing spaces of a bounded string.
+ *
+ * @param[in,out] str_start
+ * Address of a pointer to the start of the string. On return, the pointer
+ * is advanced to after any leading spaces.
+ *
+ * @param[in,out] str_end
+ * Address of a pointer to the end of the string, after the last character.
+ * On return, the pointer is moved back to before any trailing spaces.
+ */
+static void guac_terminal_color_scheme_strip_spaces(const char** str_start,
+ const char** str_end) {
+
+ /* Strip leading spaces. */
+ while (*str_start < *str_end && isspace(**str_start))
+ (*str_start)++;
+
+ /* Strip trailing spaces. */
+ while (*str_end > *str_start && isspace(*(*str_end - 1)))
+ (*str_end)--;
+}
+
+/**
+ * Parse the name part of the name-value pair within the color-scheme
+ * configuration.
+ *
+ * @param client
+ *
+ * @param name_start
+ * Start of the name string.
+ *
+ * @param name_end
+ * End of the name string, after the last character.
+ *
+ * @param foreground
+ * Pointer to the foreground color.
+ *
+ * @param background
+ * Pointer to the background color.
+ *
+ * @param palette
+ * Pointer to the palette array.
+ *
+ * @param[out] target
+ * On return, pointer to the color struct that corresponds to the name.
+ *
+ * @return
+ * Zero if successful or non-zero otherwise.
+ */
+static int guac_terminal_parse_color_scheme_name(guac_client* client,
+ const char* name_start, const char* name_end,
+ guac_terminal_color* foreground, guac_terminal_color* background,
+ guac_terminal_color (*palette)[256],
+ guac_terminal_color** target) {
+
+ guac_terminal_color_scheme_strip_spaces(&name_start, &name_end);
+
+ if (!guac_terminal_color_scheme_compare_token(
+ name_start, name_end, GUAC_TERMINAL_SCHEME_FOREGROUND)) {
+ *target = foreground;
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_BLACK_WHITE) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_BLACK;
- default_background = GUAC_TERMINAL_COLOR_WHITE;
+
+ if (!guac_terminal_color_scheme_compare_token(
+ name_start, name_end, GUAC_TERMINAL_SCHEME_BACKGROUND)) {
+ *target = background;
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_GREEN_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_DARK_GREEN;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+
+ /* Parse color<n> value. */
+ int index = -1;
+ if (sscanf(name_start, GUAC_TERMINAL_SCHEME_NUMBERED "%d", &index) &&
+ index >= 0 && index <= 255) {
+ *target = &(*palette)[index];
+ return 0;
}
- else if (strcmp(color_scheme, GUAC_TERMINAL_SCHEME_WHITE_BLACK) == 0) {
- default_foreground = GUAC_TERMINAL_COLOR_WHITE;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Unknown color name: \"%.*s\".",
+ name_end - name_start, name_start);
+ return 1;
+}
+
+/**
+ * Parse the value part of the name-value pair within the color-scheme
+ * configuration.
+ *
+ * @param client
+ *
+ * @param value_start
+ * Start of the value string.
+ *
+ * @param value_end
+ * End of the value string, after the last character.
+ *
+ * @param palette
+ * The current color palette.
+ *
+ * @param[out] target
+ * On return, the parsed color.
+ *
+ * @return
+ * Zero if successful or non-zero otherwise.
+ */
+static int guac_terminal_parse_color_scheme_value(guac_client* client,
+ const char* value_start, const char* value_end,
+ const guac_terminal_color (*palette)[256],
+ guac_terminal_color* target) {
+
+ guac_terminal_color_scheme_strip_spaces(&value_start, &value_end);
+
+ /* Parse color<n> value. */
+ int index = -1;
+ if (sscanf(value_start, GUAC_TERMINAL_SCHEME_NUMBERED "%d", &index) &&
+ index >= 0 && index <= 255) {
+ *target = (*palette)[index];
+ return 0;
}
- /* If invalid, default to "gray-black" */
- else {
- guac_client_log(client, GUAC_LOG_WARNING,
- "Invalid color scheme: \"%s\". Defaulting to \"gray-black\".",
- color_scheme);
- default_foreground = GUAC_TERMINAL_COLOR_GRAY;
- default_background = GUAC_TERMINAL_COLOR_BLACK;
+ /* Parse X11 value. */
+ if (!guac_terminal_xparsecolor(value_start, target))
+ return 0;
+
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Invalid color value: \"%.*s\".",
+ value_end - value_start, value_start);
+ return 1;
+}
+
+/**
+ * Parse a color-scheme configuration string, and return specified
+ * foreground/background colors and color palette.
+ *
+ * @param client
--- End diff --
Please document this parameter as well.
---
[GitHub] guacamole-server pull request #132: GUACAMOLE-470: Make terminal colors conf...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/132#discussion_r161148689
--- Diff: src/terminal/terminal.c ---
@@ -255,52 +257,203 @@ void* guac_terminal_thread(void* data) {
}
-guac_terminal* guac_terminal_create(guac_client* client,
- const char* font_name, int font_size, int dpi,
- int width, int height, const char* color_scheme) {
+/**
+ * Compare a non-null-terminated string spanning token_start to token_end, with
+ * a null-terminating string at literal.
+ */
+static int guac_terminal_color_scheme_compare_token(const char* token_start,
--- End diff --
The parameters and return values of all functions need to be documented.
---