mike-jumper commented on code in PR #942:
URL: https://github.com/apache/guacamole-client/pull/942#discussion_r1450906395
##########
guacamole/src/main/frontend/src/app/client/templates/client.html:
##########
@@ -47,7 +47,7 @@
<!-- Menu -->
<div class="menu" ng-class="{open: menu.shown}" id="guac-menu">
- <div class="menu-content" ng-if="menu.shown"
guac-touch-drag="menuDrag">
+ <div class="menu-content" guac-touch-drag="menuDrag">
Review Comment:
The `ng-if` was originally added here by
https://github.com/apache/guacamole-client/commit/9065b497c2333b6034a490ccfe41d36e91d21e5b,
which states:
> GUACAMOLE-352: Remove Guacamole menu entirely when closed. Input fields
within the menu must not continue to receive input.
Is that no longer an issue?
[GUACAMOLE-352](https://issues.apache.org/jira/browse/GUACAMOLE-352) was the
addition of support for dead keys, which involved adding a hidden input field
to receive composition events via `Guacamole.InputSink`.
##########
guacamole/src/main/frontend/src/app/form/directives/form.js:
##########
@@ -240,6 +248,28 @@ angular.module('form').directive('guacForm', [function
form() {
};
+
+ /**
+ * Returns whether the given field should be disabled (read-only)
+ * when presented to the current user.
+ *
+ * @param {Field} field
+ * The field to check.
+ *
+ * @returns {Boolean}
+ * true if the given field should be disabled, false otherwise.
+ */
+ $scope.isDisabled = function isDisabled(field) {
+
+ /*
+ * The field is disabled if either the form as a whole is
disabled,
+ * or if a client is provided to the directive, and the field
is
+ * marked as pending.
+ */
+ return $scope.disabled ||
+ _.get($scope.client, ['arguments', field.name,
'pending']);
+ }
Review Comment:
Missing semicolon.
##########
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##########
@@ -139,6 +139,21 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
};
+ /**
+ * True if and only if a "guacFieldFocused" event was received, and a
+ * corresponding "guacFieldBlurred" event has not yet been received.
+ * This is intended to allow fields to receive keyboard input even when
+ * the menu is not being shown.
+ *
+ * @type {boolean}
+ */
+ $scope.fieldIsFocused = false;
+
+ // Enable and disable the custom field focused state as the relevant
+ // events are received
+ $scope.$on('guacFieldFocused', () => $scope.fieldIsFocused = true);
+ $scope.$on('guacFieldBlurred', () => $scope.fieldIsFocused = false);
Review Comment:
If multiple extensions emit these events, depending on the order things are
processed, it looks possible that extension X might emit `guacFieldBlurred`
after extension & emits `guacFieldFocused`, causing `$scope.fieldIsFocused` to
erroneously report `false`.
I think the way focus is tracked here should be modified such that this is
not possible.
Another thought: would it make sense for any such extension to instead
directly handle `guacBeforeKeydown` and `guacBeforeKeyup` on its own? I don't
think there's anything preventing multiple listeners for those events from
invoking `preventDefault()` according to their own, independent criteria. If
that's the case, then there would be no need for these additional events and
corresponding flag.
##########
guacamole/src/main/frontend/src/app/client/styles/menu.css:
##########
@@ -137,10 +137,10 @@
.menu,
.menu.closed {
left: -480px;
- opacity: 0;
+ visibility: hidden;
}
.menu.open {
left: 0px;
- opacity: 1;
+ visibility: visible;
Review Comment:
This will defeat the intent of the following transition:
https://github.com/apache/guacamole-client/blob/ddf9e723e324583b82c1617555052792341c0206/guacamole/src/main/frontend/src/app/client/styles/menu.css#L34
##########
guacamole/src/main/frontend/src/app/form/directives/form.js:
##########
@@ -79,7 +80,14 @@ angular.module('form').directive('guacForm', [function
form() {
*
* @type String
*/
- focused : '='
+ focused : '=',
+
+ /**
+ * The client associated with this form, if any.
Review Comment:
To avoid a Too Much Magic condition here, I think we should also document
the automatic field enable/disable behavior that's driven by this. It's not
otherwise clear to a user of the `<guac-form ...>` directive that setting the
`client` attribute will cause their field(s) to become disabled if they happen
to match the names of arguments that were submitted to that client.
##########
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##########
@@ -468,6 +483,31 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
else if (menuShownPreviousState)
$scope.applyParameterChanges($scope.focusedClient);
+ /* Broadcast changes to the menu display state */
Review Comment:
I see you're in C mode. ;)
Please use `//` for non-block comments in JS.
##########
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##########
@@ -468,6 +483,31 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
else if (menuShownPreviousState)
$scope.applyParameterChanges($scope.focusedClient);
+ /* Broadcast changes to the menu display state */
+ $scope.$broadcast('guacMenuShown', menuShown);
+
+ });
+
+ // Toggle the menu when the guacClientToggleMenu event is received
+ $scope.$on('guacToggleMenu',
+ () => $scope.menu.shown = !$scope.menu.shown);
+
+ // Show the menu when the guacClientShowMenu event is received
+ $scope.$on('guacShowMenu', () => $scope.menu.shown = true);
+
+ // Hide the menu when the guacClientHideMenu event is received
+ $scope.$on('guacHideMenu', () => $scope.menu.shown = false);
+
+ // Broadcast any mouse events caught from clients back down to the rest
+ // of the client page
+ $scope.$on('guacClientMouseEvent', (angularEvent, mouseEvent) => {
+ $scope.$broadcast('guacMouseEvent', mouseEvent);
+ });
+
+ // Broadcast any touch events caught from clients back down to the rest
+ // of the client page
+ $scope.$on('guacClientTouchEvent', (angularEvent, touchEvent) => {
+ $scope.$broadcast('guacTouchEvent', touchEvent);
Review Comment:
What's the utility of these new `guacClientMouseEvent`, `guacMouseEvent`,
`guacClientTouchEvent`, and `guacTouchEvent` events?
##########
guacamole/src/main/frontend/src/app/client/directives/guacClient.js:
##########
@@ -263,6 +266,9 @@ angular.module('client').directive('guacClient', [function
guacClient() {
if (!client || !display)
return;
+ // Send any touch events up to be handled by a parent
+ $scope.$emit('guacClientTouchEvent', event);
Review Comment:
This looks to be indented one level too far.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
