GUACAMOLE-1292: prompt when closing the window/tab to avoid accidents with ctrl-w #592
Conversation
There was a problem hiding this comment.
Please also read through our contribution guidelines when you can:
In particular, each change needs a corresponding JIRA issue, and that JIRA issue needs to be tagged in the commit message(s) following established convention. You can see examples of this in practice in our git history.
guacamole/src/main/webapp/app/client/controllers/clientController.js
Outdated
Show resolved
Hide resolved
phreakocious
changed the title
phreakocious
changed the title
|
Was there anything else you'd like altered? |
| /** | ||
| * Catch window or tab closing (ctrl-w) and prompt the user if there is an active connection | ||
| * | ||
| * @param {Event} e |
There was a problem hiding this comment.
Please add documentation for parameters accepted by functions.
| * Catch window or tab closing (ctrl-w) and prompt the user if there is an active connection | ||
| * | ||
| * @param {Event} e | ||
| * @returns {undefined} |
There was a problem hiding this comment.
If this function returns nothing, there's no need to include an @returns.
| // Start intercepting ctrl-w / window close | ||
| $window.addEventListener('beforeunload', windowCloseListener, false); |
There was a problem hiding this 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:
- Check whether any clients are present in the overall set of managed clients (this is exposed by the
guacClientManagerservice). - If there are clients, prompt. If there aren't, don't.
In fact, guacClientManager already handles unload:
Perhaps it would make sense for it to handle beforeunload as well, and for that to be the sole instance of this event handler?
|
@phreakocious - FYI: looks like you inadvertently pushed a bunch of commits from |
Thanks for the heads up. |
|
@phreakocious: One of your commits still lacks the |
It will likely be awhile before I'll have time to address the concerns raised in the last review. It did work to achieve the intended goal when I last tested it, but there could certainly be edge cases. |
|
@phreakocious Thanks for letting us know. If it gets addressed prior to 1.4.0 release, great, otherwise we'll push it to the next release. |
|
Hi, I'm here to see if that issue can be reactivated. |
|
Just chiming in and voicing my support for this PR. |
|
Thanks @Cyberes and @webtroter - at this point we're just waiting for the contributor to finish fixing up the requested items, or someone else to come along and for the changes and finish up the work. |
Unintentionally closing a guac tab with ctrl-w is a frequent complaint. This change prompts the user when closing the tab if there is an active connection.