Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman merged PR #695: URL: https://github.com/apache/guacamole-client/pull/695 -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on code in PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#discussion_r1386746809
##
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##
@@ -44,7 +44,7 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
const requestService = $injector.get('requestService');
const tunnelService = $injector.get('tunnelService');
const userPageService= $injector.get('userPageService');
-
+const guacFullscreen = $injector.get('guacFullscreen');
Review Comment:
done
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
mike-jumper commented on code in PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#discussion_r1385704931
##
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##
@@ -44,7 +44,7 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
const requestService = $injector.get('requestService');
const tunnelService = $injector.get('tunnelService');
const userPageService= $injector.get('userPageService');
-
+const guacFullscreen = $injector.get('guacFullscreen');
Review Comment:
This is fairly important, IMHO. Maintaining alphabetical order for things
like `#include`, `import`, blocks of retrievals from the AngularJS `$injector`,
etc. helps avoid merge conflicts.
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on code in PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#discussion_r1385647195
##
guacamole/src/main/frontend/src/app/client/controllers/clientController.js:
##
@@ -44,7 +44,7 @@ angular.module('client').controller('clientController',
['$scope', '$routeParams
const requestService = $injector.get('requestService');
const tunnelService = $injector.get('tunnelService');
const userPageService= $injector.get('userPageService');
-
+const guacFullscreen = $injector.get('guacFullscreen');
Review Comment:
Nitpick, but could you put this service in alphabetical order with the ones
above it, and then keep the space below this before the next code comment?
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1800259861
> I ended up in adding it to $scope.$on('$destroy', function
clientViewDestroyed() function.
=> solved in my eyes
Seems reasonable to me.
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1793414438
> I fell over an problem concerning browsers not implementing
`keyboard.lock` api (e.g. firefox): If you set fullscreen mode and use the
browsers back functionality (e.g. mouse xbutton1) the screen will stay in
fullscreen mode. This might be confusing to some users.
>
> 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. Is there an event like this
@necouchman ?
I ended up in adding it to `$scope.$on('$destroy', function
clientViewDestroyed()` function.
=> solved in my eyes
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1793376217 I fell over an problem concerning browsers not implementing `keyboard.lock` api (e.g. firefox): If you set fullscreen mode and use the browsers back functionality (e.g. mouse xbutton1) the screen will stay in fullscreen mode. This might be confusing to some users. 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. Is there an event like this @necouchman ? -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1763376124 > Is the failing docker build a problem @necouchman? The problem within the build seems not related to this PR. 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. :shrug: -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1763369307 Is the failing docker build a problem @necouchman? The problem within the build seems not related to this PR. -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1763356834 > To be honest maybe you should add a linter or [codestyle checker](https://maven.apache.org/plugins/maven-checkstyle-plugin/usage.html) to your project and setup your rules. Ah, nice, I will have a look. -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1763316582 > 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. I understand that and it's ok. I hope it's fne now @necouchman To be honest maybe you should add a linter or [codestyle checker](https://maven.apache.org/plugins/maven-checkstyle-plugin/usage.html) to your project and setup your rules. So you haven't to explain it to contributers (which is annoying I know) -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on code in PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#discussion_r1359808885
##
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * A service for providing true fullscreen and keyboard lock support.
+ * Keyboard lock is currently only supported by Chromium based browsers
+ * (Edge >= V79, Chrome >= V68 and Opera >= V55)
+ */
+angular.module('client').factory('guacFullscreen', ['$injector',
+
+
+function guacFullscreen($injector) {
Review Comment:
It's not always done the same coding style. I tried to align it to your
example
--
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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on code in PR #695:
URL: https://github.com/apache/guacamole-client/pull/695#discussion_r1359374329
##
guacamole/src/main/frontend/src/translations/en.json:
##
@@ -62,6 +62,7 @@
"ACTION_CANCEL": "@:APP.ACTION_CANCEL",
"ACTION_CLEAR_CLIENT_MESSAGES" : "Clear",
"ACTION_CLEAR_COMPLETED_TRANSFERS" : "Clear",
+"ACTION_FULLSCREEN": "Fullscreen",
Review Comment:
I think this should be down below `ACTION_DISCONNECT` by Alphabetical order?
##
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * A service for providing true fullscreen and keyboard lock support.
+ * Keyboard lock is currently only supported by Chromium based browsers
+ * (Edge >= V79, Chrome >= V68 and Opera >= V55)
+ */
+angular.module('client').factory('guacFullscreen', ['$injector',
+
+
+function guacFullscreen($injector) {
+
+var service = {};
+
+// toggles current fullscreen mode (off if on, on if off)
+service.toggleFullscreenMode = function toggleFullscreenMode(){
+if(!service.isInFullscreenMode()){
+service.setFullscreenMode(true);
+}else{
+service.setFullscreenMode(false);
+}
+}
+
+// check is browser in true fullscreen mode
+service.isInFullscreenMode=function isInFullscreenMode(){
+return document.fullscreenElement;
+}
+
+// set fullscreen mode
+service.setFullscreenMode = function setFullscreenMode(state) {
+if(document.fullscreenEnabled){
Review Comment:
Spaces between `if` and `(` and between `)` and `{`
##
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * A service for providing true fullscreen and keyboard lock support.
+ * Keyboard lock is currently only supported by Chromium based browsers
+ * (Edge >= V79, Chrome >= V68 and Opera >= V55)
+ */
+angular.module('client').factory('guacFullscreen', ['$injector',
+
+
+function guacFullscreen($injector) {
Review Comment:
Looking at other services, from a style perspective I think you can remove
the space between the `angular...` line and the `function...` line, and then
indent the function line one more. For example, see:
https://github.com/apache/guacamole-client/blob/d1faaa9605c5eef668da8bf84279d7a88cad5af7/guacamole/src/main/frontend/src/app/client/services/guacClientManager.js#L20-L25
##
guacamole/src/main/frontend/src/app/client/services/guacFullscreen.js:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
sirux88 commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1762777057 @necouchman: If there's a chance to get it merged by time I'm willing to work on this for sure. Just let me know. Since it is only on the jira issue but affecs functionality: 1) `True fullscreen mode` should work on each maintained browser as stated here => https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API 2) `Keyboard lock` will only work on chromium based browsers => https://developer.mozilla.org/en-US/docs/Web/API/Keyboard/lock -- 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]
Re: [PR] GUACAMOLE-1525: Support for true fullscreen mode and keyboard lock [guacamole-client]
necouchman commented on PR #695: URL: https://github.com/apache/guacamole-client/pull/695#issuecomment-1762514767 @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. -- 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]
