mike-jumper commented on a change in pull request #592:
URL: https://github.com/apache/guacamole-client/pull/592#discussion_r601832928
##########
File path: guacamole/src/main/webapp/app/client/controllers/clientController.js
##########
@@ -219,6 +220,26 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
remaining: 15
};
+ /**
+ * Catch window or tab closing (ctrl-w) and prompt the user if there is an
active connection
+ *
+ * @param {Event} e
Review comment:
Please add documentation for parameters accepted by functions.
##########
File path: guacamole/src/main/webapp/app/client/controllers/clientController.js
##########
@@ -219,6 +220,26 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
remaining: 15
};
+ /**
+ * Catch window or tab closing (ctrl-w) and prompt the user if there is an
active connection
+ *
+ * @param {Event} e
+ * @returns {undefined}
Review comment:
If this function returns nothing, there's no need to include an
`@returns`.
##########
File path: guacamole/src/main/webapp/app/client/controllers/clientController.js
##########
@@ -831,6 +853,8 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
// Hide status and sync local clipboard once connected
else if (connectionState ===
ManagedClientState.ConnectionState.CONNECTED) {
+ // Start intercepting ctrl-w / window close
+ $window.addEventListener('beforeunload', windowCloseListener,
false);
Review comment:
I think this will be problematic. Since the event listener is being
added conditionally depending on whether the _current_ client has connected
(and removing it upon error), there will be issues if:
* The user navigates away from the client view (the client will remain
active and connecting in the background, ready for the user to resume)
* The user has multiple connections open within the same tab (they will
fight over handling of `beforeunload`). Depending on whether the event handler
is recognized as the same function by the event stack, this would result either
in the connections removing each other's handlers or the connections adding
multiple copies of the event handler.
I think this instead needs to be at the root level, outside the handling of
a connection. If there were a _single_ `beforeunload` event handler that is
never removed, that handler could:
1. Check whether any clients are present in the overall set of managed
clients (this is exposed by the `guacClientManager` service).
2. If there are clients, prompt. If there aren't, don't.
In fact, `guacClientManager` already handles `unload`:
https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole/src/main/webapp/app/client/services/guacClientManager.js#L164-L165
Perhaps it would make sense for it to handle `beforeunload` as well, and for
that to be the sole instance of this event handler?
--
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:
[email protected]
