[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.

[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