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/02/10 06:23:12 UTC
[GitHub] [guacamole-server] mather opened a new pull request #256:
GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
mather opened a new pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256
KeyboardType, KeyboardSubtype, KeyboardFunctionKey is required for Japanese keyboard setting actually,
but the patch has been applied to overwrite values only for Japanese keyboards.
see: https://github.com/FreeRDP/FreeRDP/pull/707
I think it should be specified from Guacamole as FreeRDP user side.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mather commented on a change in pull request
#256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mather commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r378048629
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
Review comment:
It's good to migrate naming.
Should I replace these name in this PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r377296057
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
+ * See https://docs.microsoft.com/ja-jp/windows/win32/api/winuser/nf-winuser-getkeyboardtype
Review comment:
Please point to the English documentation link rather than the Japanese version.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r381071918
##########
File path: src/protocols/rdp/settings.c
##########
@@ -1189,9 +1189,15 @@ void guac_rdp_push_settings(guac_client* client,
rdp_settings->DesktopHeight = guac_settings->height;
rdp_settings->AlternateShell = guac_rdp_strdup(guac_settings->initial_program);
rdp_settings->KeyboardLayout = guac_settings->server_layout->freerdp_keyboard_layout;
- rdp_settings->KeyboardType = guac_settings->server_layout->freerdp_keyboard_type;
- rdp_settings->KeyboardSubType = guac_settings->server_layout->freerdp_keyboard_subtype;
- rdp_settings->KeyboardFunctionKey = guac_settings->server_layout->freerdp_keyboard_function_key;
+ if (guac_settings->server_layout->freerdp_keyboard_type > 0) {
Review comment:
Seeing as the value being compared is a `UINT32`, testing that the value is non-zero would be more appropriate. The value can't be negative.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mather commented on a change in pull request
#256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mather commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r378048761
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
+ * See https://docs.microsoft.com/ja-jp/windows/win32/api/winuser/nf-winuser-getkeyboardtype
Review comment:
I will replace to English page URL.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r381070916
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
Review comment:
Yes, I would say so. With three new layout-related members being added here, if `freerdp_` is not an appropriate prefix, then it shouldn't be used for the new members, and the older, existing member should be corrected to follow the new convention. The member in question is not part of a public API, and avoiding the rename would amount to technical debt.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r377299193
##########
File path: src/protocols/rdp/settings.c
##########
@@ -1189,6 +1189,9 @@ void guac_rdp_push_settings(guac_client* client,
rdp_settings->DesktopHeight = guac_settings->height;
rdp_settings->AlternateShell = guac_rdp_strdup(guac_settings->initial_program);
rdp_settings->KeyboardLayout = guac_settings->server_layout->freerdp_keyboard_layout;
+ rdp_settings->KeyboardType = guac_settings->server_layout->freerdp_keyboard_type;
+ rdp_settings->KeyboardSubType = guac_settings->server_layout->freerdp_keyboard_subtype;
+ rdp_settings->KeyboardFunctionKey = guac_settings->server_layout->freerdp_keyboard_function_key;
Review comment:
Will this cause problems for the keymap files that don't have these values defined?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mather commented on a change in pull request
#256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mather commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r378048629
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
Review comment:
It's good to migrate name.
Should I replace these name in this PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r377296649
##########
File path: src/protocols/rdp/keymaps/ja_jp_qwerty.keymap
##########
@@ -20,6 +20,9 @@
parent "base"
name "ja-jp-qwerty"
freerdp "KBD_JAPANESE"
+freerdp_keyboard_type "KBD_TYPE_JAPANESE"
Review comment:
Is this something that should be set for the other layouts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r377298585
##########
File path: src/protocols/rdp/keymap.h
##########
@@ -103,6 +103,24 @@ struct guac_rdp_keymap {
*/
const UINT32 freerdp_keyboard_layout;
+ /**
+ * FreeRDP keyboard type definition.
Review comment:
If the keyboard layout, type, subtype, etc. are Windows/RDP standard values and not specific to FreeRDP, perhaps we should take this opportunity to move away from `freerdp_keyboard_layout`, `freerdp_keyboard_type`, etc. here (and `freerdp` within the keymaps) and migrate to naming that isn't tied to the underlying RDP implementation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mather commented on a change in pull request
#256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mather commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r378049450
##########
File path: src/protocols/rdp/settings.c
##########
@@ -1189,6 +1189,9 @@ void guac_rdp_push_settings(guac_client* client,
rdp_settings->DesktopHeight = guac_settings->height;
rdp_settings->AlternateShell = guac_rdp_strdup(guac_settings->initial_program);
rdp_settings->KeyboardLayout = guac_settings->server_layout->freerdp_keyboard_layout;
+ rdp_settings->KeyboardType = guac_settings->server_layout->freerdp_keyboard_type;
+ rdp_settings->KeyboardSubType = guac_settings->server_layout->freerdp_keyboard_subtype;
+ rdp_settings->KeyboardFunctionKey = guac_settings->server_layout->freerdp_keyboard_function_key;
Review comment:
Sorry, it should not be set if no value.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-server] mather commented on a change in pull request
#256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
Posted by GitBox <gi...@apache.org>.
mather commented on a change in pull request #256: GUACAMOLE-951: Add ability to set KeyboardType for FreeRDP
URL: https://github.com/apache/guacamole-server/pull/256#discussion_r378047164
##########
File path: src/protocols/rdp/keymaps/ja_jp_qwerty.keymap
##########
@@ -20,6 +20,9 @@
parent "base"
name "ja-jp-qwerty"
freerdp "KBD_JAPANESE"
+freerdp_keyboard_type "KBD_TYPE_JAPANESE"
Review comment:
FreeRDP has default values.
https://github.com/FreeRDP/FreeRDP/blob/master/libfreerdp/core/settings.c#L345-L347
So, no need to specify if there is no problem for other layouts.
----------------------------------------------------------------
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
With regards,
Apache Git Services