-
Notifications
You must be signed in to change notification settings - Fork 712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock #695
GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock #695
Conversation
28b0335
to
c60bc6e
Compare
c60bc6e
to
028b45a
Compare
@sirux88 I know you've been frustrated by difficulty of getting PRs merged. Are you still willing to work on this one? I have a couple of minor style issues and then I think this one could be ready to go and close out several long-standing issues. |
@necouchman: Since it is only on the jira issue but affecs functionality:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sirux88 - Mostly some style change requests. It can seem like some of these are trivial or pedantic, but when you scale it up to the amount of code, here, it makes a difference after a while for the overall readability of the code, so we tend to be very particular about the style being consisten.
Other than that, unless @mike-jumper or @jmuehlner have any concerns, I think it's good.
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js
Outdated
Show resolved
Hide resolved
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js
Outdated
Show resolved
Hide resolved
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js
Outdated
Show resolved
Hide resolved
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js
Outdated
Show resolved
Hide resolved
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js
Outdated
Show resolved
Hide resolved
028b45a
to
9bef98c
Compare
I understand that and it's ok. I hope it's fine now @necouchman To be honest maybe you should add a linter or codestyle checker to your project and setup your rules. So you haven't to explain it to contributers (which is annoying I know) |
Ah, nice, I will have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me, now. I will give @mike-jumper and @jmuehlner a while longer to respond with any issues they have, otherwise I'll go ahead and merge it.
Is the failing docker build a problem @necouchman? |
Yeah, seems to be something with the Github infrastructure, not directly related to the PR. I was seeing similar issues the other night when I was rebasing some PRs, and they cleared themselves by the next day. 🤷 |
I fell over an problem concerning browsers not implementing My approach to solve it would be to unload fullscreen mode if the client is "minimized". Meaning guacamole application is going to its homescreen. But I haven't found an event to register to. |
79ba6bb
to
1f53bff
Compare
I ended up in adding it to |
1f53bff
to
c1e6532
Compare
Seems reasonable to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more style change request.
guacamole/src/main/frontend/src/app/client/controllers/clientController.js
Outdated
Show resolved
Hide resolved
and keyboard lock
c1e6532
to
57e9fd1
Compare
associated JIRA issue: https://issues.apache.org/jira/browse/GUACAMOLE-1525
see also: GUACAMOLE-989 and GUACAMOLE-124
To start fullscreen mode:
Open gucamole's special menu via Ctrl+Alt+Shift and click fullsceen:
("Vollbild" is german for Fullscreen)