[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #592: prompt when closing the window/tab to avoid accidents with ctrl-w

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #592:
URL: https://github.com/apache/guacamole-client/pull/592#discussion_r578791816



##########
File path: guacamole/src/main/webapp/app/client/controllers/clientController.js
##########
@@ -219,6 +220,12 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
         remaining: 15
     };
 
+    // Catch window or tab closing (ctrl-w) and prompt the user
+    var ctrlwlistener = function onBeforeUnload(e) {
+            e.preventDefault();
+            e.returnValue = '';
+     };

Review comment:
       1. Please correct the indentation here. The body of the function appears to be indented far further than elsewhere (we use 4-space indents for each level), and the closing brace of the function definition is misaligned.
   2. Please use JSDoc to document functions, their return values, and their parameters.
   3. I think calling this function `ctrlwlistener()` is a bit misleading. Not all browsers will use Ctrl+W for closing a tab or window, nor is this function directly related to that shortcut.

##########
File path: guacamole/src/main/webapp/app/client/controllers/clientController.js
##########
@@ -776,6 +783,8 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
 
         // Client error
         else if (connectionState === ManagedClientState.ConnectionState.CLIENT_ERROR) {
+            // Stop intercepting window close
+            $window.removeEventListener('beforeunload', ctrlwlistener, false);

Review comment:
       Why repeatedly add, remove, add, remove, etc. the listener vs. checking the relevant state within the listener itself?
   
   What about connections that are running in the background, as will occur if the user navigates back to the home screen or to a settings screen without disconnecting?




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