[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119410766
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -49,11 +49,29 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
/**
* The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
+ * user interface provided by this directive. We populate the clipboard
+ * editor via this DOM element rather than updating a model so that we
+ * are prepared for future support of rich text contents.
*
* @type Element
*/
-var element = $element[0];
+var element = $element[0].querySelectorAll('.clipboard.active')[0];
+
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard Editor
+ * will not be displayed with Clipboard data.
+ *
+ * @type Boolean
+ */
+$scope.isActive = false;
+
+/**
+ * Updates clipboard editor to be active.
+ */
+$scope.setActive = function setActive() {
+$scope.isActive = true
Review Comment:
Semicolon missing here
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119289963
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -49,11 +49,29 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
/**
* The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
+ * user interface provided by this directive. We populate the clipboard
+ * editor via this DOM element rather than updating a model so that we
+ * are prepared for future support of rich text contents.
*
* @type Element
*/
-var element = $element[0];
+var element = $element[0].querySelector('.clipboard');
Review Comment:
Hmm, now that I look at this, it makes me a little uncomfortable. This is
just selecting the first element with the `clipboard` class, but now that
there's 2 of those, it's only the ordering in the template that makes this work
- if somebody came in later and changed the order around, clipboard sync would
just stop working properly and it might not be obvious why.
How about updating this to specifically only match the active clipboard?
Either by modifying this selector to match only elements that have the
`clipboard` class, but _not_ the `inactive` class, or (probably better) by
adding an `active` class to the active clipboard and querying based on that?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119279778
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -49,19 +49,37 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
/**
* The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
+ * user interface provided by this directive. We populate the clipboard
+ * editor via this DOM element rather than updating a model so that we
+ * are prepared for future support of rich text contents.
*
* @type Element
*/
-var element = $element[0];
+var element = $element[0].querySelector('.clipboard');
+
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard Editor
+ * will not be displayed with Clipboard data.
+ *
+ * @type Boolean
+ */
+$scope.isActive = false;
+
+/**
+ * Updates clipboard editor to be active.
+ */
+$scope.setActive = function setActive() {
+$scope.isActive = true
+};
/**
* Rereads the contents of the clipboard field, updating the
* ClipboardData object on the scope as necessary. The type of data
* stored within the ClipboardData object will be heuristically
* determined from the HTML contents of the clipboard field.
*/
-var updateClipboardData = function updateClipboardData() {
+const updateClipboardData = function updateClipboardData() {
Review Comment:
I'm a fan of using `const` over `var` for this sort of thing for sure - but
since no actual changes were made to this function, we should just leave it
unchanged as much as possible - to ensure clean git history. Same with the
removal of line 105 below. Let's just leave things the way they were.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119273814
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -49,19 +49,37 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
/**
* The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
+ * user interface provided by this directive. We populate the clipboard
+ * editor via this DOM element rather than updating a model so that we
+ * have support for rich text contents.
Review Comment:
I think we're getting a bit ahead of ourselves claiming that we "have
support for rich text contents". Might want to clarify that this is to enable
future support for this functionality, since we certainly don't have it now.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119272481
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -49,19 +49,37 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
/**
* The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
+ * user interface provided by this directive. We populate the clipboard
+ * editor via this DOM element rather than updating a model so that we
+ * have support for rich text contents.
*
* @type Element
*/
-var element = $element[0];
+var element = $element[0].querySelector('.clipboard');
+
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard Editor
+ * will not be displayed with Clipboard data.
+ *
+ * @type Boolean
+ */
+$scope.isActive = false;
+
+/**
+ * Updates clipboard editor be active.
Review Comment:
I think you're missing a preposition here.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119200881
##
guacamole/src/main/frontend/src/app/clipboard/templates/guacClipboard.html:
##
@@ -1 +1,4 @@
-
+
+
+{{'CLIENT.TEXT_CLIPBOARD_AWAITING_FOCUS' | translate}}
Review Comment:
This mostly looks good, though I'm not a big fan of directly setting scope
variables like this inside the the template like this:
`ng-focus="isActive=true"`.
This looks like it's moving controller logic a bit too far into the
template, sort of the opposite problem as before. If you created a
`setActive()` function or similar on the controller scope, that would probably
restore the balance, so to speak.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119189226
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -82,17 +83,12 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// If the clipboard data is a string, render it as text
if (typeof data.data === 'string')
-element.value = data.data;
+$scope.clipboardData = data.data;
// Ignore other data types for now
};
-// Update the internally-stored clipboard data when events are fired
-// that indicate the clipboard field may have been changed
-element.addEventListener('input', updateClipboardData);
Review Comment:
Hey so, looking back at this - the reason these were initially done this
way, with the direct DOM access in the controller - was part of an attempt to
allow rich text content in the clipboard (formatting, images, etc).
Obviously it doesn't currently support that, but the current changes in this
PR would have to be undone if we want to try to go down that road again. For
that reason, I think we should just stick with the existing logic for updating
the clipboard contents.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119161187
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -48,25 +48,26 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
function guacClipboardController($scope, $injector, $element) {
/**
- * The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
- *
- * @type Element
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard Editor
+ * will not be displayed with Clipboard data.
+ *
+ * @type Boolean
*/
-var element = $element[0];
-
+$scope.isActive = false;
+
Review Comment:
Oops! Trailing whitespace.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119152660
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -47,26 +47,21 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
-/**
- * The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
- *
- * @type Element
- */
-var element = $element[0];
-
+// Default to inactive Clipboard Editor so any contents would be
hidden.
+$scope.isActive = false;
Review Comment:
To match the styling of controllers here, this should have a block comment
with a `@type` annotation. The one you had for the `scope` config in the
controller definition would work here.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119107481
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -40,33 +40,38 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
var config = {
restrict: 'E',
replace : true,
+scope: {
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard
Editor
+ * will not be updated with Clipboard data.
+ *
+ * @type Boolean
+ */
+isActive : '=?',
+},
templateUrl : 'app/clipboard/templates/guacClipboard.html'
};
// guacClipboard directive controller
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
-/**
- * The DOM element which will contain the clipboard contents within the
- * user interface provided by this directive.
- *
- * @type Element
- */
-var element = $element[0];
-
+// Check if isActive was defined. If not default to false
+$scope.isActive = angular.isDefined($scope.isActive) ? $scope.isActive
: false;
Review Comment:
What is this logic for? When would `$scope.isActive` be defined at
controller initialization time?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1119106733
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -40,33 +40,38 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
var config = {
restrict: 'E',
replace : true,
+scope: {
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard
Editor
+ * will not be updated with Clipboard data.
+ *
+ * @type Boolean
+ */
+isActive : '=?',
Review Comment:
I don't think you need this? This `scope` in the directive config is used
for passing arguments to the directive - which you're not doing that I can see.
You can just set stuff directly on `$scope` in the controller.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117755536
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,17 +141,23 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+element.addEventListener('blur', hideAndUpdateClipboardEditor);
Review Comment:
I don't think there's any one single correct answer for this one - I could
see some users (like me) getting frustrated when the clipboard contents keep
getting hidden while working in a connection, making it more difficult to keep
track of the clipboard content.
On the other hand, I could see why some users might want to automatically
rehide every time, in case they copy a password locally and have it get
populated into their clipboard in the background without realizing it.
I almost wonder if this should be a configuration setting...
@mike-jumper thoughts?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117742357
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -40,13 +40,31 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
var config = {
restrict: 'E',
replace : true,
+scope: {
+/**
+ * The message that will be displayed within the Clipboard Editor
+ * when it does not have focus.
+ *
+ * @type String
+ */
+inactiveContent : '=',
Review Comment:
What I meant was to have the translation done in the template, as opposed to
the controller. It _is_ technically done in a template now, but it should be
the clipboard template, not the client template, and doesn't need to be passed
through any controllers or javascript code at all.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117492635
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -71,6 +89,32 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
};
+/**
+ * Allows Clipboard Editor to begin displaying clipboard data, reads
the
+ * clipboard data and updates the content of the clipboard editor to
the
+ * given data.
+ */
+var displayAndUpdateClipboardEditor = function
displayAndUpdateClipboardEditor() {
+isActive = true;
+element.value = ''
+element.classList.remove("inactive");
Review Comment:
Or alternatively, you could consider rendering completely different elements
based on `isActive` using e.g. `ng-view="isActive"` or `ng-view="!isActive"`.
That might end up being easier and probably wouldn't require dynamically
fiddling CSS classes.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117489112
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,17 +141,23 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+element.addEventListener('blur', hideAndUpdateClipboardEditor);
Review Comment:
Do we really want to hide the contents of the clipboard every time it's
blurred? That would get pretty annoying. I'd think that just initially hiding
the clipboard every time the sidebar is opened and re-hiding when the sidebar
is closed should be good enough.
I'd welcome other opinions on this, however.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117485991
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -71,6 +89,32 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
};
+/**
+ * Allows Clipboard Editor to begin displaying clipboard data, reads
the
+ * clipboard data and updates the content of the clipboard editor to
the
+ * given data.
+ */
+var displayAndUpdateClipboardEditor = function
displayAndUpdateClipboardEditor() {
+isActive = true;
+element.value = ''
+element.classList.remove("inactive");
+
+clipboardService.getClipboard().then((data) => {
+updateClipboardEditor(data);
+}, angular.noop);
+};
+
+/**
+ * Prevent the Clipboard Editor from displaying clipboard data.
+ */
+var hideAndUpdateClipboardEditor = function
hideAndUpdateClipboardEditor() {
Review Comment:
This entire function probably doesn't need to exist - all this can be done
directly in the template with conditional rendering depending on the value of
`isActive`.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117480336
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,17 +141,23 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+element.addEventListener('blur', hideAndUpdateClipboardEditor);
// Update remote clipboard if local clipboard changes
$scope.$on('guacClipboard', function clipboardChanged(event, data) {
-updateClipboardEditor(data);
-});
+// Don't update with clipboard data unless focused.
+if (!isActive) {
+return
+}
-// Init clipboard editor with current clipboard contents
-clipboardService.getClipboard().then((data) => {
updateClipboardEditor(data);
-}, angular.noop);
Review Comment:
Why d
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117479626
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -40,13 +40,31 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
var config = {
restrict: 'E',
replace : true,
+scope: {
+/**
+ * The message that will be displayed within the Clipboard Editor
+ * when it does not have focus.
+ *
+ * @type String
+ */
+inactiveContent : '=',
+},
templateUrl : 'app/clipboard/templates/guacClipboard.html'
};
// guacClipboard directive controller
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
+/**
+ * When isActive is set to true then the Clipboard data will be
+ * displayed in the Clipboard Editor. When false, the Clipboard Editor
+ * will not be updated with Clipboard data.
+ *
+ * @type Boolean
+ */
+var isActive = false;
Review Comment:
If you expose this to the template (on `$scope`), you could have the
template itself render differently based on the state. That would move all the
display logic (css styles, translations, etc) out of the controller and into
the view where it belongs.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117477784
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -40,13 +40,31 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
var config = {
restrict: 'E',
replace : true,
+scope: {
+/**
+ * The message that will be displayed within the Clipboard Editor
+ * when it does not have focus.
+ *
+ * @type String
+ */
+inactiveContent : '=',
Review Comment:
It's a little weird for the clipboard element to have to have the parent
tell it what its own inactive text should be. This functionality should really
be internal to the clipboard directive.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1117477306
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -71,6 +89,32 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
};
+/**
+ * Allows Clipboard Editor to begin displaying clipboard data, reads
the
+ * clipboard data and updates the content of the clipboard editor to
the
+ * given data.
+ */
+var displayAndUpdateClipboardEditor = function
displayAndUpdateClipboardEditor() {
+isActive = true;
+element.value = ''
+element.classList.remove("inactive");
Review Comment:
You can do this same thing with `ng-class` in the template - no need to put
this kind of functionality directly in the controller.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1115007468
##
guacamole/src/main/frontend/src/translations/en.json:
##
@@ -162,6 +162,7 @@
"TEXT_USER_LEFT" : "{USERNAME} has left the
connection.",
"TEXT_RECONNECT_COUNTDOWN": "Reconnecting in {REMAINING}
{REMAINING, plural, one{second} other{seconds}}...",
"TEXT_FILE_TRANSFER_PROGRESS" : "{PROGRESS} {UNIT, select, b{B}
kb{KB} mb{MB} gb{GB} other{}}",
+"TEXT_CLIPBOARD_AWAITING_FOCUS" : "Focus here to view clipboard
data...",
Review Comment:
"Focus" seems like an excessively technical term here: one that people not
fluent in web technologies might not understand. If the required action is to
click on the clipboard, it should probably just say that plainly.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1115010730
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,12 +119,20 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+
// Update remote clipboard if local clipboard changes
$scope.$on('guacClipboard', function clipboardChanged(event, data) {
updateClipboardEditor(data);
});
+// Update the prefocus clipboard editor message
+guacTranslate('CLIENT.TEXT_CLIPBOARD_AWAITING_FOCUS', '').then(
Review Comment:
Also, what if a user literally has the text "Focus here to view clipboard
data..." in their clipboard?
Having some quick visual method to clearly identify that the clipboard is
inactive would be nice. Rather than just sticking a message into the textarea -
how about visually disabling (greying out, etc) the textarea and rendering a
message in a different spot where it couldn't be confused with actual clipboard
contents?
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1115007468
##
guacamole/src/main/frontend/src/translations/en.json:
##
@@ -162,6 +162,7 @@
"TEXT_USER_LEFT" : "{USERNAME} has left the
connection.",
"TEXT_RECONNECT_COUNTDOWN": "Reconnecting in {REMAINING}
{REMAINING, plural, one{second} other{seconds}}...",
"TEXT_FILE_TRANSFER_PROGRESS" : "{PROGRESS} {UNIT, select, b{B}
kb{KB} mb{MB} gb{GB} other{}}",
+"TEXT_CLIPBOARD_AWAITING_FOCUS" : "Focus here to view clipboard
data...",
Review Comment:
"Focus" seems like an excessively technical term here: one that people not
fluent in web technologies might not understand. If the required action is to
hover over the clipboard, it should probably just say that plainly.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1115005495
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,12 +119,20 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+
// Update remote clipboard if local clipboard changes
$scope.$on('guacClipboard', function clipboardChanged(event, data) {
updateClipboardEditor(data);
});
+// Update the prefocus clipboard editor message
+guacTranslate('CLIENT.TEXT_CLIPBOARD_AWAITING_FOCUS', '').then(
Review Comment:
It might be a bit cleaner/easier to just do the translation in the template.
That would also improve separation of concerns - the controller shouldn't have
to care about what specific messages are being displayed to the user. All it
should need to know is what the state of the clipboard is, exposing that state
to the template which can then render different elements (with translated text
in them) according to that state.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1114845032
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -47,6 +47,12 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
+/**
+ * Set to true if the clipboard data should be displayed in the
Review Comment:
You might consider rewording this comment to be about what this variable
_does_, not what the code should set it to.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1114844082
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -47,6 +47,12 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
+/**
+ * Set to true if the clipboard data should be displayed in the
+ * Clipboard Editor. Otherwise, false.
+ */
+var shouldDisplay = false;
Review Comment:
To match the existing code style of other variables declared here, this
should have a `@type` annotation.
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -92,6 +110,12 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// that indicate the clipboard field may have been changed
element.addEventListener('input', updateClipboardData);
element.addEventListener('change', updateClipboardData);
+element.addEventListener('focus', displayAndUpdateClipboardEditor);
+
+// Update remote clipboard if local clipboard changes
Review Comment:
Is this copy pasta? Looks like it's a verbatim copy of the block directly
below.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1114845032
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -47,6 +47,12 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
config.controller = ['$scope', '$injector', '$element',
function guacClipboardController($scope, $injector, $element) {
+/**
+ * Set to true if the clipboard data should be displayed in the
Review Comment:
You might consider rewording this comment to be about what this variable
_does_ not what the code should set it to.
--
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]
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #796: GUACAMOLE-1740: Don't display clipboard contents in the clipboard editor until it is focused on.
jmuehlner commented on code in PR #796:
URL: https://github.com/apache/guacamole-client/pull/796#discussion_r1114842252
##
guacamole/src/main/frontend/src/app/clipboard/directives/guacClipboard.js:
##
@@ -82,7 +100,7 @@ angular.module('clipboard').directive('guacClipboard',
['$injector',
// If the clipboard data is a string, render it as text
if (typeof data.data === 'string')
-element.value = data.data;
+element.value = (shouldDisplay) ? data.data : '';
Review Comment:
It looks like this just shows an empty clipboard until the user clicks on
the clipboard element, and then the data pops in, yeah? It'd probably be lot
more obvious to users if there was some sort of styling to indicate an inactive
clipboard, and some indication to the user about what to do if they want to see
the clipboard.
--
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]
