You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/06/21 21:55:34 UTC
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #530: GUACAMOLE-103: Implement common redirect form field.
mike-jumper commented on a change in pull request #530:
URL: https://github.com/apache/guacamole-client/pull/530#discussion_r443259842
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/form/RedirectField.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.guacamole.form;
+
+import java.net.URI;
+
+/**
+ * A Guacamole field that redirects a user to another page.
+ */
+public class RedirectField extends Field {
+
+ /**
+ * The encoded URL of the redirect.
Review comment:
What do you mean by encoded?
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/form/RedirectField.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.guacamole.form;
+
+import java.net.URI;
+
+/**
+ * A Guacamole field that redirects a user to another page.
+ */
+public class RedirectField extends Field {
+
+ /**
+ * The encoded URL of the redirect.
+ */
+ private final URI redirectUrl;
+
+ /**
+ * The message that will be displayed for the user during the redirect
+ * process. This will be processed through Guacamole's translation system.
+ */
+ private final String redirectMsg;
+
+ /**
+ * Creates a new field which facilitates redirection of the user
+ * to another page.
+ *
+ * @param name
+ * The name of this field.
+ *
+ * @param redirectUrl
+ * The URL to which the user should be redirected.
+ *
+ * @param redirectMsg
+ * The message to display during the redirect, which will be processed
+ * through Guacamole's translation system.
+ */
+ public RedirectField(String name, URI redirectUrl, String redirectMsg) {
+
+ // Init base field properties
+ super(name, Field.Type.REDIRECT);
+
+ // Store the URL to which the user will be redirected.
+ this.redirectUrl = redirectUrl;
+ this.redirectMsg = redirectMsg;
+
+ }
+
+ /**
+ * Returns the URL of the redirect.
Review comment:
"The URL of the redirect" reads a little unclear to me.
Earlier, in the constructor, you use the wording "the URL to which the user should be redirected", which is quite clear. Perhaps that would be better here?
##########
File path: guacamole/src/main/webapp/app/form/controllers/redirectFieldController.js
##########
@@ -18,16 +18,26 @@
*/
/**
- * Config block which registers openid-specific field types.
+ * Controller for the redirect field, which redirects the user to the provided
+ * URL.
*/
-angular.module('guacOpenID').config(['formServiceProvider',
- function guacOpenIDConfig(formServiceProvider) {
+angular.module('form').controller('redirectFieldController', ['$scope','$window',
+ function redirectFieldController($scope,$window) {
- // Define field for token from OpenID service
- formServiceProvider.registerFieldType("GUAC_OPENID_TOKEN", {
- templateUrl : 'app/ext/guac-openid/templates/openidTokenField.html',
- controller : 'guacOpenIDController',
- module : 'guacOpenID'
- });
+ /**
+ * Redirect the user to the provided URL.
+ */
+ $window.location.href = $scope.field.redirectUrl;
+
+ /**
+ * Return the text that should be displayed to the user while the redirect
+ * is taking place.
+ *
+ * @return {String}
+ * The text to display for the user during the redirect.
+ */
+ $scope.getRedirectMsg = function getRedirctMsg() {
+ return $scope.field.redirectMsg;
+ };
Review comment:
As `field.redirectMsg` is already on the scope, it's possible to reference that directly rather than via a dedicated function.
##########
File path: guacamole-ext/src/main/java/org/apache/guacamole/form/RedirectField.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.
+ */
+
+package org.apache.guacamole.form;
+
+import java.net.URI;
+
+/**
+ * A Guacamole field that redirects a user to another page.
+ */
+public class RedirectField extends Field {
+
+ /**
+ * The encoded URL of the redirect.
+ */
+ private final URI redirectUrl;
+
+ /**
+ * The message that will be displayed for the user during the redirect
+ * process. This will be processed through Guacamole's translation system.
+ */
+ private final String redirectMsg;
+
+ /**
+ * Creates a new field which facilitates redirection of the user
+ * to another page.
+ *
+ * @param name
+ * The name of this field.
+ *
+ * @param redirectUrl
+ * The URL to which the user should be redirected.
+ *
+ * @param redirectMsg
+ * The message to display during the redirect, which will be processed
+ * through Guacamole's translation system.
+ */
+ public RedirectField(String name, URI redirectUrl, String redirectMsg) {
+
+ // Init base field properties
+ super(name, Field.Type.REDIRECT);
+
+ // Store the URL to which the user will be redirected.
+ this.redirectUrl = redirectUrl;
+ this.redirectMsg = redirectMsg;
+
+ }
+
+ /**
+ * Returns the URL of the redirect.
+ *
+ * @return
+ * The URL of the redirect.
+ */
+ public String getRedirectUrl() {
+ return redirectUrl.toString();
+ }
+
+ /**
+ * Returns the message that will be displayed for the user while the
+ * redirect takes place.
+ *
+ * @return
+ * The message to display for the user.
+ */
+ public String getRedirectMsg() {
+ return redirectMsg;
+ }
Review comment:
I think:
* As this will be a part of the public extension API, it would be better to not abbreviate "message" as "msg".
* It would be better to leverage `TranslatableMessage` for the translatable message rather than just `String`, as the semantics surrounding `TranslatableMessage` are more explicit and variable substitution is inherently supported.
If you agree with both of the above, I suggest simply implementing `Translatable` and its `getTranslatableMessage()` function.
----------------------------------------------------------------
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