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 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){
+
+ if(state && !service.isInFullscreenMode())
document.documentElement.requestFullscreen().then(navigator.keyboard.lock());
+ else if(!state && service.isInFullscreenMode())
document.exitFullscreen().then(navigator.keyboard.unlock());
Review Comment:
These lines are long enough that it probably makes sense to not try to do
one-liners, here, and, instead, put the `document..` part on its own line.
Also, please put spaces between the `if` and the opening parenthesis.
##########
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(){
Review Comment:
Spaces around the equals sign, 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) {
+
+ 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);
+ }
+ }
Review Comment:
* Please put spaces between parenthesis and brackets.
* Please put spaces between the `if` and its opening parenthesis, and
between the `else` and its bracket.
* Please don't cuddle the `else` clause with the closing bracket above it.
--
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]
