Automagical copy paste #1174

bulldozier · 2018-12-04T22:30:47Z

No description provided.

…hen it acts

samhed · 2018-12-06T22:12:36Z

Hi @bulldozier, and thanks for the pull request.

I believe that we could certainly improve on our clipboard feature and make it more convenient. I did have a quick look at your code but didn't have time to test it yet. For starters I have one request and one question:

Please provide a description for your feature here in the PR.
app/ui.js does already have a popup-message function called "showStatus()". While I didn't look into details about how your new overlay message works, I don't want to have another function doing the same thing. Could showStatus() be used instead?

CendioOssman

The principle with an automatic clipboard is nice. But there are some details that need fixing. Also:

Fix up the lint errors (see the travis build)
Could you add some tests for this?
Please try to improve the commit messages. Anyone browsing through the log should be able to quickly tell what this is about.

CendioOssman · 2018-12-07T08:19:37Z

app/ui.js

@@ -936,12 +938,43 @@ const UI = {
        }
    },

+    readClipboard(callback) {
+      var readfuture = navigator.clipboard.readText();


This API isn't available everywhere, so we need some handling here when it isn't.

CendioOssman · 2018-12-07T08:20:43Z

app/ui.js

+    },
+
+    enterVncClick(e) {
+        UI.readClipboard(text => {


Why not merge readClipboard() directly in to this function? It doesn't seem to be called elsewhere.

CendioOssman · 2018-12-07T08:21:39Z

app/ui.js

+    enterVncClick(e) {
+        UI.readClipboard(text => {
+            document.getElementById('noVNC_clipboard_text').value = text;
+            UI.rfb.clipboardPasteFrom(text);


Is this step needed? Don't we get a notification when we change the value?

CendioOssman · 2018-12-07T08:24:49Z

app/ui.js

+    copyFromLocalClipboard() {
+	UI.readClipboard(text => {
+	    var clipVal = document.getElementById('noVNC_clipboard_text').value;
+	    if ( clipVal != text ) {


I think this is essential, otherwise we have no mechanism that avoids just blindly overwriting the server clipboard whenever focus changes.

However this is still a hack, so I think we should reference the clipboardchange event and that we're waiting for browsers to implement it.

CendioOssman · 2018-12-07T08:25:35Z

app/ui.js

-        document.getElementById('noVNC_clipboard_text').value = e.detail.text;
-        Log.Debug("<< UI.clipboardReceive");
+	var curvalue = document.getElementById('noVNC_clipboard_text').value;
+	if (curvalue != e.detail.text) {


👎

We should always update the local clipboard on a proper event.

CendioOssman · 2018-12-07T08:27:22Z

app/ui.js

+	document.addEventListener('focus', UI.focusVNC);
+	document.addEventListener('focusout', UI.focusoutVNC);
+	document.addEventListener('mousemove', UI.mouseMoveVNC);
+	document.addEventListener('mousedown', UI.mouseDownVNC);


Why do we need all of these events? Isn't it sufficient with just focus?

I discovered while implementing the clipboard feature that the Chrome API clipboard.readText() is still buggy. It fails sometimes (not-deterministic why) with undefined error. When using all these events here to try to read it, it mostly works. I reported the bug to the Chrome community and suggest not implement this feature until it doesn't work properly.

That's disappointing. Do you have a link?

Yes of course, follow this: https://bugs.chromium.org/p/chromium/issues/detail?id=949926&q=clipboard&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified

There I described how to reproduce the bug.

CendioOssman · 2018-12-07T08:27:57Z

app/ui.js

-        }
+	if ( UI.needToCheckClipboardChange ) {
+	    UI.copyFromLocalClipboard();
+	}


Please don't hijack existing (and unrelated) event handlers.

CendioOssman · 2018-12-07T08:32:23Z

A good guide for git commit messages can be found here:

https://chris.beams.io/posts/git-commit/

bulldozier · 2018-12-10T19:18:55Z

Thanks for all the feedback. I am working on fixing all the issues. Also, I didn't actually mean to submit the pull request yet.

samhed · 2018-12-10T21:31:45Z

Hi @bulldozier no problem, I put the "Work in Progress" tag on this for now then.

It's really hard to say why that code was ever added. It just makes everything work wrong when using macos.

Fix keyboard for macos users

CendioOssman · 2019-01-11T13:49:06Z

@bulldozier, any progress?

And those commits that were added seem unrelated.

samhed · 2019-02-07T11:18:38Z

@bulldozier are you still interested in getting this merged?

bulldozier · 2019-02-07T15:37:00Z

I will eventually circle back around to this and fix a few things. The code is not ready for merge.

…

On Thu, Feb 7, 2019 at 3:18 AM Samuel Mannehed ***@***.***> wrote: @bulldozier <https://github.com/bulldozier> are you still interested in getting this merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1174 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHC8dNMrahfeZifavVmrDqg-J7lXVnLKks5vLAuVgaJpZM4ZBohk> .

akamos · 2019-04-03T14:38:53Z

Hi!
We would require this feature in our project, but as @bulldozier mentioned the code is still not ready to merge. I would like to use this code base of him and make it ready to get merged into master.
How can I proceed? Should I use the forked version of @bulldozier or should I create a new clean fork from master and copy/paste the code base from @bulldozier and create a new pull request with clean messages as required?

CendioOssman · 2019-04-04T07:36:07Z

We want a clean history, so in this case I'd say to "copy" his code and finish things. Make sure to give proper attribution though, e.g. in the commit message or using multiple --author.

samhed · 2019-12-20T16:03:42Z

Closing due to lack of activity.

kent-at-google added 2 commits November 13, 2018 16:04

Automagical copy paste!

709193d

Enhanced copy/paste to work with more events and to popup a message w…

f77cf27

…hen it acts

samhed added feature ui/full labels Dec 6, 2018

CendioOssman requested changes Dec 7, 2018

View reviewed changes

samhed added the WIP label Dec 10, 2018

bukzor and others added 3 commits December 19, 2018 16:29

Fix keyboard for macos users

aff1207

It's really hard to say why that code was ever added. It just makes everything work wrong when using macos.

Merge pull request #1 from bukzor/patch-1

16858a5

Fix keyboard for macos users

Merge remote-tracking branch 'origin/master' into automagical-copy-paste

d1c6c70

akamos mentioned this pull request Sep 25, 2019

Auto copy-paste with navigator.clipboard API #1301

Closed

samhed added the noresponse label Nov 28, 2019

juanjoDiaz mentioned this pull request Dec 18, 2019

Add automatic clipboard support #1347

Open

samhed closed this Dec 20, 2019

snickell mentioned this pull request Jul 13, 2021

Add clipboard support juanjoDiaz/noVNC#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automagical copy paste #1174

Automagical copy paste #1174

bulldozier commented Dec 4, 2018

samhed commented Dec 6, 2018

CendioOssman left a comment

CendioOssman Dec 7, 2018

CendioOssman Dec 7, 2018

CendioOssman Dec 7, 2018

CendioOssman Dec 7, 2018

CendioOssman Dec 7, 2018

CendioOssman Dec 7, 2018

akamos Apr 5, 2019

CendioOssman Apr 8, 2019

akamos Apr 8, 2019 •

edited

CendioOssman Dec 7, 2018

CendioOssman commented Dec 7, 2018

bulldozier commented Dec 10, 2018

samhed commented Dec 10, 2018

CendioOssman commented Jan 11, 2019

samhed commented Feb 7, 2019

bulldozier commented Feb 7, 2019 via email

akamos commented Apr 3, 2019 •

edited

CendioOssman commented Apr 4, 2019

samhed commented Dec 20, 2019

Automagical copy paste #1174

Automagical copy paste #1174

Conversation

bulldozier commented Dec 4, 2018

samhed commented Dec 6, 2018

CendioOssman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akamos Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CendioOssman commented Dec 7, 2018

bulldozier commented Dec 10, 2018

samhed commented Dec 10, 2018

CendioOssman commented Jan 11, 2019

samhed commented Feb 7, 2019

bulldozier commented Feb 7, 2019 via email

akamos commented Apr 3, 2019 • edited

CendioOssman commented Apr 4, 2019

samhed commented Dec 20, 2019

akamos Apr 8, 2019 •

edited

akamos commented Apr 3, 2019 •

edited