You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by jamhall <gi...@git.apache.org> on 2018/03/04 12:13:21 UTC
[GitHub] guacamole-client pull request #261: GUACAMOLE-516: On screen keyboard - adde...
GitHub user jamhall opened a pull request:
https://github.com/apache/guacamole-client/pull/261
GUACAMOLE-516: On screen keyboard - added method to reset all pressed keys
On screen keyboard - added method to reset all pressed keys
https://issues.apache.org/jira/browse/GUACAMOLE-516
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jamhall/guacamole-client GUACAMOLE-516
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/261.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 #261
----
commit 4807186b121907f148f33674e1a74d7a23c5190c
Author: Jamie Hall <ha...@...>
Date: 2018-03-04T12:11:36Z
GUACAMOLE-516: On screen keyboard - added method to reset all pressed keys
----
---
[GitHub] guacamole-client pull request #261: GUACAMOLE-516: On screen keyboard - adde...
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-client/pull/261#discussion_r172060624
--- Diff: guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js ---
@@ -455,6 +455,17 @@ Guacamole.OnScreenKeyboard = function(layout) {
};
+ /**
+ * Resets the state of this keyboard, releasing all keys, and firing keyup
+ * events for each released key.
+ */
+ this.reset = function() {
--- End diff --
Though historical code has not followed this, please define a name for the function:
this.reset = function reset() {
...
};
This is being done for all new code as doing otherwise results in unreadable stack traces.
---
[GitHub] guacamole-client pull request #261: GUACAMOLE-516: On screen keyboard - adde...
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-client/pull/261#discussion_r172060570
--- Diff: guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js ---
@@ -455,6 +455,17 @@ Guacamole.OnScreenKeyboard = function(layout) {
};
+ /**
+ * Resets the state of this keyboard, releasing all keys, and firing keyup
+ * events for each released key.
+ */
+ this.reset = function() {
+ for (var keysym in pressed) {
--- End diff --
The `pressed` structure within `Guacamole.OnScreenKeyboard` is indexed by key name, not keysym:
https://github.com/apache/guacamole-client/blob/5db2e3cae75a658b33515112a39eb2f5f8eef4a8/guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js#L51-L59
---
[GitHub] guacamole-client pull request #261: GUACAMOLE-516: On screen keyboard - adde...
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-client/pull/261#discussion_r172060648
--- Diff: guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js ---
@@ -455,6 +455,17 @@ Guacamole.OnScreenKeyboard = function(layout) {
};
+ /**
+ * Resets the state of this keyboard, releasing all keys, and firing keyup
+ * events for each released key.
+ */
+ this.reset = function() {
+ for (var keysym in pressed) {
+ var key = getActiveKey(keysym);
+ osk.onkeyup(key.keysym);
+ }
+ }
--- End diff --
Missing semicolon.
---
[GitHub] guacamole-client pull request #261: GUACAMOLE-516: On screen keyboard - adde...
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-client/pull/261#discussion_r172060530
--- Diff: guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js ---
@@ -455,6 +455,17 @@ Guacamole.OnScreenKeyboard = function(layout) {
};
+ /**
+ * Resets the state of this keyboard, releasing all keys, and firing keyup
+ * events for each released key.
+ */
+ this.reset = function() {
+ for (var keysym in pressed) {
+ var key = getActiveKey(keysym);
--- End diff --
Rather than manually pull the active key, and manually invoke `onkeyup`, it would be better to invoke `release()`:
https://github.com/apache/guacamole-client/blob/5db2e3cae75a658b33515112a39eb2f5f8eef4a8/guacamole-common-js/src/main/webapp/modules/OnScreenKeyboard.js#L346-L377
The `release()` function takes care of invoking `onkeyup` (if it's defined) as well as proper updating of `pressed`.
Directly calling `onkeyup` like this is problematic because:
1. The `pressed` structure tracks all currently-pressed keys, but this function does not update that structure. After manually invoking `onkeyup`, the `pressed` structure will be out of sync with the actual key state.
2. Usages of `Guacamole.OnScreenKeyboard` are not required to provide a function `onkeyup`. If that function is omitted, `reset()` as written here will fail.
---