Sync primary to clipboard immediately

[ferki]

This PR is a proposed new feature to sync primary selection to clipboard instantly in order to be able to paste without an explicit copy step when CM_SYNC_SELECTIONS=1 CM_SYNC_PRIMARY_TO_CLIPBOARD=1.

I was wondering about the proper naming of the related environment variable, so I'm up to rename it for a better alternative. Also, there might be a better place for this syncing step.

Please review and merge, or let me know what to change.

Fixes #125.

[cdown]

Hmm, we used to do this but it doesn't work properly with applications like urxvt, which unhighlight text and do other bad things when focus is taken away.

I'm worried people will set CM_SYNC_SELECTIONS=1, since it sounds nice, and then there will be a bunch of undebuggable bug reports as a result (like what happened with CM_OWN_CLIPBOARD...).

Any ideas about that?

[ferki]

Hmm..instead of piping from primary selection, I originally used loading from the file directly:

_xsel -i --clipboard < "$filename"

Since piping worked for my setup (I don't use urxvt anymore), I preferred avoiding the file read operation on each selection (even if I guess the file would be in the file system cache of the OS at that point with a high probability, since it was just written).

Would it help to do it like that? Or perhaps even with <<< "$data" instead of the file? Are there any other alternatives you would consider?

[cdown]

That's also fine, as long as we don't touch the PRIMARY owner.

I'm curious though, this is marked as fixes #125, but seems a tangential concern, maybe?

[ferki]

Originally I was using the quick&dirty patch mentioned on my earlier comment for a good while locally (just checked, the original commit date is 2017-10-19, and even by then, I was already using it for some time).

I opened #125 to have an issue about what I'm solving locally with this, before I open a PR.

Then I decided to add CM_SYNC_SELECTIONS to control the behaviour, and also removed the need for file access, because I hoped those would make the PR more likely to be accepted.

[cambid]

I can confirm that this patch works as expected. Can you please merge this patch and release a new version?

[cdown]

The good news is it looks like urxvt doesn't have this behaviour any more, which makes things easier :-)

My only lingering concern is the name. CM_SYNC_SELECTIONS implies things are bidirectional -- eg. not only this, but also if you change CLIPBOARD, it's reflected in PRIMARY.

Other than that, let me try out this patch for a few days. Feel free to poke me again if I don't get back to you.

[ferki]

Well, naming is one of the hard problems in IT...:) To me CM_SYNC_SELECTIONS means that they will be kept in sync, whereas before they diverged.

I wouldn't mind renaming the variable to anything else as long as the resulting behavior solves my use case :) Let me know what would work better for your taste, and also whether you'd like me to rebase it on top of current development HEAD.

[cdown]

Maybe CM_SYNC_PRIMARY_TO_CLIPBOARD?

[OJFord]

The readme presently says of clipmenu:

3. After selection, the clip is put onto the PRIMARY and CLIPBOARD X selections.

Perhaps revise that to 'After choosing an entry [...]' or something, to avoid confusion?

[ferki]

Maybe CM_SYNC_PRIMARY_TO_CLIPBOARD?

@cdown: I've updated the PR by rebasing it on top of current develop branch, and by changing the variable name to your suggestion.

The readme presently says of clipmenu:

1. After selection, the clip is put onto the PRIMARY and CLIPBOARD X selections.

Perhaps revise that to 'After choosing an entry [...]' or something, to avoid confusion?

@OJFord: hmm, I'm under the impression is that it's still correct.

If one uses clipmenu to select a clip from a list, then pressing enter puts that into both primary and clipboard selections.

And this PR is more about letting clipmenud sync the primary to clipboard without having to go to clipmenu and select a previous clip first to make that happen.

So I believe that part of the README don't need to be changed, but let me know if I mix some things up :upside_down_face:

[OJFord]

Oh no it remains true - I didn't mean to suggest anything had changed in that regard.

It's just that I arrived here essentially out of misunderstanding that sentence, I took the first 'selection' to refer to the same thing as the second 'selections', but it doesn't, annotated it means:

After selection [of an item from the clipmenu list], the clip is put onto the PRIMARY and CLIPBOARD X [X] selections.

So, I thought I was doing something wrong, that it wasn't working, because I made my 'selection' (of text on the screen) and the clip wasn't put onto both.

Since you're adding an option for that, I'm just suggesting this could make the difference a bit clearer.

Once chosen, the clip ...

or

After selecting an entry, the clip ...

or similar.

Doesn't really matter to me, I know now, just thought it could be clearer that it isn't default for the next person to want this behaviour :slightly_smiling_face:.

[ferki]

Thanks for elaborating on that, I see your point now!

[cdown]

After some testing, this doesn't interact well with pass show -c, because it only copies onto the CLIPBOARD selection, but we then immediately nuke what's there with the contents of PRIMARY. The same issue holds for all applications which copy only to CLIPBOARD.

We should only copy from PRIMARY -> CLIPBOARD if PRIMARY changed.

[ferki]

@cdown: thanks for the feedback, I've pushed the logic into the for loop above, and added a check about the current selection being processed (just like @OJFord did in his example code on the related issue :+1: ).

I've also slightly changed the wording from instantly to immediately, I feel that fits better as description.

[cdown]

Hmm, there is a small race here if both PRIMARY and CLIPBOARD change at the same time, where the contents of CLIPBOARD will now likely never be recorded. In practice it might not be a problem, but still seems worth being conscious about.

How about adding a new associative array, updated_sel, which stores a 0 or 1 for the last round for that selection, then doing it outside the loop? That would make sure that even if we do end up overwriting it in this situation, we still store it in the clip database. What do you think? :)

[ferki]

@cdown: I hope it's acceptable now :)

[cdown]

Thanks! I'll run with this for a few days and report back.

[cdown]

Okay, here's another case this doesn't interact very well with. In Chrome (and presumably Chromium), when you select some text on the page, the intermediate selections as you drag all go to PRIMARY. That means if you do the following:

1. Copy some text
2. Drag over some other text
3. Try to paste the text in step 1

...you end up just pasting the same thing you selected in step 2. This behaviour is really annoying and probably broken, but inevitably it will result in a large number of user complaints running with CM_SYNC_PRIMARY_TO_CLIPBOARD.

Since this is an opt-in feature, I don't think this is a blocker, but we should make it clear that this will not work well with Chrome. Usually I wouldn't call out specific applications' misfeatures in --help or elsewhere, but I know the load is going to be high if we don't do it for this one.

How about changing the text to something like this:

- $CM_SYNC_PRIMARY_TO_CLIPBOARD: sync selections from primary to clipboard immediately. (default: 0)
  WARNING: Does not work well with Chrome-like browsers due to a misfeature in Chrome.

[ferki]

I'm using this patch for more than 3 years now with both Chrome and surf, but I'm afraid I don't understand the problem you described.

That being said, feel free to add any warning message you deem necessary to avoid a possible flood of support requests.

[cdown]

Here's a video that clearly shows the problem: https://www.youtube.com/watch?v=_CmJ0ZORpjo

First, the string "Hi there" is placed. Then, the string "Foo" is placed. "Foo" is copied. Then one goes to select "Hi there", which results in the following copies onto PRIMARY, one after the other:

1. e
2. re
3. ere
4. [...]
5. Hi there

This then overwrites "Foo" on CLIPBOARD due to this patchset, and thus, when you paste, "Hi there" is pasted, instead of "Foo".

[ferki]

Hmm, I haven't noticed a problem due to the "incremental" selection, but the pasting of "Hi there" is the feature that is being implemented here, so that part works as expected. In other words, the purpose of this whole story is to be able to paste whatever is selected without expressly pressing CTRL-C or anything (as described in the the opening post of the related #125 issue).

I'm not sure if there's anything we can do about the incremental updates. I usually double or triple click to select text in the browser and the terminal, so I guess that's why I didn't see it on my end. If I drag to select text, clipmenu shows the last selected text, and I see no incremental bits in the history when I open it with dmenu.

[cdown]

Sure thing, then. We had people complain about this before, which was why the duplicate detection was added in the first place, so I know somebody's going to enable it and then complain :-)

With the warning, I think this is probably good to go, pending a couple more days of testing.

[ferki]

I believe people who look for this behavior are the ones who are looking for an equivalent feature of some other software, e.g the one that is called "sync selection contents" in Parcellite. I think that's why I originally named the variable CM_SYNC_SELECTIONS in the first place. It's all about "making sure you can paste a selection right away without pressing anything to copy first", what other solutions seem to call "syncing selections".

In that sense, there might be even better names or description for this, and/or the warning message might not be necessary (which seems to highlight Chrome only, while it's probably true for anything where text is selected incrementally by dragging the cursor, but doesn't seem to affect the end-result after all).

My hypothesis is that clipnotify sends a "selection has changed" notification event for each new character selected/deselected while dragging, causing clipmenud to process the change in the selection content on each update, but still the last update wins. Does that sound correct? Do we need a separate issue about this?

[cdown]

It's all about "making sure you can paste a selection right away without pressing anything to copy first", what other solutions seem to call "syncing selections".

The incremental part is a "feature" of the way Chrome manages the PRIMARY buffer, it's not really universal. Last time I looked other browsers and text boxes in gtk/etc didn't have the same behaviour of putting it into PRIMARY before selection was finished, but maybe I'm wrong or things have changed.

But yes, the use of PRIMARY itself without explicit copy is more widespread.

My hypothesis is that clipnotify sends a "selection has changed" notification event for each new character selected/deselected while dragging, causing clipmenud to process the change in the selection content on each update, but still the last update wins. Does that sound correct? Do we need a separate issue about this?

It's nothing to do with clipnotify itself, clipnotify just does what it's told :-) This problem even existed when we used to do the sleep 0.5 polling method -- the problem is that the browser updates the PRIMARY buffer before selection ends. That's certainly not universal behaviour.

The warning still needs to be there, since I know for sure there will be tickets otherwise -- that's exactly what happened when we used to manipulate PRIMARY, but sure, it may not need to be Chrome-specific.

[ferki]

Thanks @cdown for going into the details, it is valuable knowledge to be aware of.

[cdown]

Thanks for working on this! I appreciate you sticking with it.

Thinking more, as long as the tickets are obviously related to this, that shouldn't be too much of a problem. So I think between adding an ugly warning and the potential for tickets, let's err on the side of no warning for now and we can always add it later if needed, especially with it off by default.

So I think all that's needed before merge is a few days more testing to see if there are any more edge cases :-)

[OJFord]

Thanks for working on this! I appreciate you sticking with it

Just wanted to echo that (and.. I used it to copy that quote :grin:) - thanks!

Will let you know if I hit anything unexpected over next few days, but it's saving me frustration (of guessing whether or not / forgetting choosing from clipmenu necessary for my copy-paste) already. Just a little grudgingly-learned muscle memory to undo perhaps :slightly_smiling_face:.

And thanks also @cdown for being receptive to it, despite it presumably not being how you want (your own tool) to work for your own purposes! Much appreciated.

[ferki]

Thanks folks, I also appreciate knowing that my efforts are considered useful :heart:

[cdown]

Thanks!

[ferki]

Thanks for the feedback and merging, @cdown! :heart: