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 2020/06/23 07:29:22 UTC
[GitHub] [guacamole-server] mike-jumper opened a new pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
mike-jumper opened a new pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285
This change restructures the RDP keymap translation system such that:
* Overall modifier and lock state are tracked, not just the state of individual keysyms.
* Multiple possible key definitions may be associated with each keysym. For example, the `en-us-qwerty` keymap describes both `Caps+Shift+A` and `A` as possible ways to type a lowercase "a".
* When a key is pressed, the key definition which requires the least complex additional events will be chosen based on the current keyboard lock/modifier state.
* The PDU sent from the RDP server when the lock state changes remotely is handled, keeping local tracking of keyboard state in sync.
* Rather than release automatically-pressed keys immediately after pressing the required scancode, maintain consistent state, instead releasing _all_ automatically-pressed keys only after the user has released all manually-pressed keys client-side.
With this in place, modifiers and locks are set only as required, and the behavior resulting in [GUACAMOLE-518](https://issues.apache.org/jira/browse/GUACAMOLE-518) no longer occurs.
This unfortunately required painstakingly researching and editing all currently-supported keymaps ... with the exception of the Japanese keymap which has additional issues and will need to be corrected at a later date via [GUACAMOLE_520](https://issues.apache.org/jira/browse/GUACAMOLE-520). The results of that research can be found here:
https://issues.apache.org/jira/browse/GUACAMOLE-518?focusedCommentId=17142479&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17142479
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444268231
##########
File path: src/protocols/rdp/Makefile.am
##########
@@ -241,7 +241,7 @@ rdp_keymaps = \
$(srcdir)/keymaps/da_dk_qwerty.keymap \
$(srcdir)/keymaps/tr_tr_qwerty.keymap
-_generated_keymaps.c: $(rdp_keymaps)
+_generated_keymaps.c: $(rdp_keymaps) $(srcdir)/keymaps/generate.pl
$(AM_V_GEN) $(srcdir)/keymaps/generate.pl $(rdp_keymaps)
Review comment:
Should `generate.pl` really be specified twice, here? My `Makefile` knowlege is thin, but seems like it is duplicated?
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -217,20 +315,120 @@ static guac_rdp_key* guac_rdp_keyboard_map_key(guac_rdp_keyboard* keyboard,
* The keysym of the key to lookup within the given keyboard.
*
* @return
- * A pointer to the guac_rdp_key structure representing the definition and
- * state of the key having the given keysym, or NULL if no such key is
+ * A pointer to the guac_rdp_key structure representing the definition(s)
+ * and state of the key having the given keysym, or NULL if no such key is
* defined within the keyboard layout of the RDP server.
*/
static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
int keysym) {
/* Verify that the key is actually defined */
- guac_rdp_key* key = guac_rdp_keyboard_map_key(keyboard, keysym);
- if (key == NULL || key->definition == NULL)
+ guac_rdp_key** key_by_keysym = guac_rdp_keyboard_map_key(keyboard, keysym);
+ if (key_by_keysym == NULL)
return NULL;
- /* Key is defined within keyboard */
- return key;
+ return *key_by_keysym;
+
+}
+
+/**
+ * Given a key which may have multiple possible definitions, returns the
+ * definition that currently has the lowest cost, taking into account the
+ * current keyboard lock and modifier states.
+ *
+ * @param keyboard
+ * The guac_rdp_keyboard associated with the current RDP session.
+ *
+ * @param key
+ * The key being whose lowest-cost possible definition should be retrieved.
Review comment:
The word here makes me scratch my head a bit...
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -248,37 +446,21 @@ static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
* The keymap to use to populate the given client's keysym/scancode
* mapping.
*/
-static void __guac_rdp_keyboard_load_keymap(guac_rdp_keyboard* keyboard,
+static void guac_rdp_keyboard_load_keymap(guac_rdp_keyboard* keyboard,
Review comment:
If this is still `static`, and thus presumably only used within the context of this file, why the change in name?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444410040
##########
File path: src/protocols/rdp/Makefile.am
##########
@@ -241,7 +241,7 @@ rdp_keymaps = \
$(srcdir)/keymaps/da_dk_qwerty.keymap \
$(srcdir)/keymaps/tr_tr_qwerty.keymap
-_generated_keymaps.c: $(rdp_keymaps)
+_generated_keymaps.c: $(rdp_keymaps) $(srcdir)/keymaps/generate.pl
$(AM_V_GEN) $(srcdir)/keymaps/generate.pl $(rdp_keymaps)
Review comment:
Yep. The overall syntax of `Makefile` rules is:
```
output: list of files required for producing output
command to produce output
```
In this case, `generate.pl` should be listed as it is part of the source tree and changes to the script require a rebuild of the output. Until added here, changes that I made to `generate.pl` during testing required manually deleting `_generated_keymaps.c` to force a rebuild.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman merged pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
necouchman merged pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444413193
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -217,20 +315,120 @@ static guac_rdp_key* guac_rdp_keyboard_map_key(guac_rdp_keyboard* keyboard,
* The keysym of the key to lookup within the given keyboard.
*
* @return
- * A pointer to the guac_rdp_key structure representing the definition and
- * state of the key having the given keysym, or NULL if no such key is
+ * A pointer to the guac_rdp_key structure representing the definition(s)
+ * and state of the key having the given keysym, or NULL if no such key is
* defined within the keyboard layout of the RDP server.
*/
static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
int keysym) {
/* Verify that the key is actually defined */
- guac_rdp_key* key = guac_rdp_keyboard_map_key(keyboard, keysym);
- if (key == NULL || key->definition == NULL)
+ guac_rdp_key** key_by_keysym = guac_rdp_keyboard_map_key(keyboard, keysym);
+ if (key_by_keysym == NULL)
return NULL;
- /* Key is defined within keyboard */
- return key;
+ return *key_by_keysym;
+
+}
+
+/**
+ * Given a key which may have multiple possible definitions, returns the
+ * definition that currently has the lowest cost, taking into account the
+ * current keyboard lock and modifier states.
+ *
+ * @param keyboard
+ * The guac_rdp_keyboard associated with the current RDP session.
+ *
+ * @param key
+ * The key being whose lowest-cost possible definition should be retrieved.
Review comment:
Yikes, me too. I'll reword.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444412603
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -248,37 +446,21 @@ static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
* The keymap to use to populate the given client's keysym/scancode
* mapping.
*/
-static void __guac_rdp_keyboard_load_keymap(guac_rdp_keyboard* keyboard,
+static void guac_rdp_keyboard_load_keymap(guac_rdp_keyboard* keyboard,
Review comment:
Apparently, the double underscore prefix is actually a reserved namespace in C. It's supposed to be dedicated to the use of the compiler. Having learned this, I've been switching use of `__guac_*` to `guac_*` wherever I encounter 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444415214
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -217,20 +315,120 @@ static guac_rdp_key* guac_rdp_keyboard_map_key(guac_rdp_keyboard* keyboard,
* The keysym of the key to lookup within the given keyboard.
*
* @return
- * A pointer to the guac_rdp_key structure representing the definition and
- * state of the key having the given keysym, or NULL if no such key is
+ * A pointer to the guac_rdp_key structure representing the definition(s)
+ * and state of the key having the given keysym, or NULL if no such key is
* defined within the keyboard layout of the RDP server.
*/
static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
int keysym) {
/* Verify that the key is actually defined */
- guac_rdp_key* key = guac_rdp_keyboard_map_key(keyboard, keysym);
- if (key == NULL || key->definition == NULL)
+ guac_rdp_key** key_by_keysym = guac_rdp_keyboard_map_key(keyboard, keysym);
+ if (key_by_keysym == NULL)
return NULL;
- /* Key is defined within keyboard */
- return key;
+ return *key_by_keysym;
+
+}
+
+/**
+ * Given a key which may have multiple possible definitions, returns the
+ * definition that currently has the lowest cost, taking into account the
+ * current keyboard lock and modifier states.
+ *
+ * @param keyboard
+ * The guac_rdp_keyboard associated with the current RDP session.
+ *
+ * @param key
+ * The key being whose lowest-cost possible definition should be retrieved.
Review comment:
OK - I've removed that "being" that snuck in there somehow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-server] necouchman commented on a change in pull request #285: GUACAMOLE-518: Correct Caps Lock and Shift behavior within RDP keymap translation system.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #285:
URL: https://github.com/apache/guacamole-server/pull/285#discussion_r444285658
##########
File path: src/protocols/rdp/keyboard.c
##########
@@ -217,20 +315,120 @@ static guac_rdp_key* guac_rdp_keyboard_map_key(guac_rdp_keyboard* keyboard,
* The keysym of the key to lookup within the given keyboard.
*
* @return
- * A pointer to the guac_rdp_key structure representing the definition and
- * state of the key having the given keysym, or NULL if no such key is
+ * A pointer to the guac_rdp_key structure representing the definition(s)
+ * and state of the key having the given keysym, or NULL if no such key is
* defined within the keyboard layout of the RDP server.
*/
static guac_rdp_key* guac_rdp_keyboard_get_key(guac_rdp_keyboard* keyboard,
int keysym) {
/* Verify that the key is actually defined */
- guac_rdp_key* key = guac_rdp_keyboard_map_key(keyboard, keysym);
- if (key == NULL || key->definition == NULL)
+ guac_rdp_key** key_by_keysym = guac_rdp_keyboard_map_key(keyboard, keysym);
+ if (key_by_keysym == NULL)
return NULL;
- /* Key is defined within keyboard */
- return key;
+ return *key_by_keysym;
+
+}
+
+/**
+ * Given a key which may have multiple possible definitions, returns the
+ * definition that currently has the lowest cost, taking into account the
+ * current keyboard lock and modifier states.
+ *
+ * @param keyboard
+ * The guac_rdp_keyboard associated with the current RDP session.
+ *
+ * @param key
+ * The key being whose lowest-cost possible definition should be retrieved.
Review comment:
The wording here makes me scratch my head a bit...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org