On Unix, copy to clipboard using wl-clipboard if xclip is not available

[tezkerek]

Type of change

• ✅ Bug fix (non-breaking change which fixes an issue)

Description and Context

This fix allows copying passwords on Wayland, where xclip does not work. I've also slightly changed the code so that other clipboard tools could be easily added in the future. #3812

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

[droidmonkey]

No need for a struct, use a QPair

[tezkerek]

Isn't it clearer to have a struct with suggestively named fields?

I don't really know Qt and C++, and this doesn't seem to work:

QList<QPair<QString, QStringList>> clipPrograms;
#ifdef Q_OS_UNIX
    clipPrograms << qMakePair("xclip", (QStringList() << "-i" << "selection" << "clipboard"))
                 << qMakePair("wl-copy", QStringList());
#endif

/usr/include/qt/QtCore/qpair.h:61:31: error: array used as initializer
   61 |         : first(t1), second(t2) {}

[droidmonkey]

Its just more code to deal with. Try doing:

clipPrograms << {QStringLiteral("xclip"), QStringList() << "arg1" << "arg2"}

You can also forgo the QStringList and just use a QString. When you pass it to QProcess just split the arguments: process.second().split(" ")

[tezkerek]

I figured out how to use QPair, it works now. I don't know why the Ubuntu build failed though. It says something about GUI tests, but I only touched the CLI part.

[louib]

@tezkerek I think for historical reason the CLI tests are bundled with the GUI tests. In this case I think there really might be a problem with the patch:

[14:39:05][Step 5/5] FAIL!  : TestCli::testClip() Compared values are not the same
[14:39:05][Step 5/5]    Actual   (clipboard->text())  : ""
[14:39:05][Step 5/5]    Expected (QString("Password")): "Password"
[14:39:05][Step 5/5]    Loc: [/opt/buildagent/work/c401303cba1b4098/tests/TestCli.cpp(481)]

[tezkerek]

I'd somehow managed to mess up xclip's arguments. I fixed that.

I have another issue now: it seems that xclip doesn't fail to run on Wayland, it simply doesn't copy it to Wayland's clipboard. So if xclip is installed and the user is running Wayland, clipping still won't work, because xclip will exit successfully.

Maybe check for WAYLAND_DISPLAY to determine what the user is running? Or simply let the user specify what clipboard program they wish to use?

[louib]

@tezkerek I'm not sure what's best to be honest. Maybe change the order? Start with wl-clipboard and fallback to xclip, would that work?

[tezkerek]

I really don't get how QClipboard works. The cli test fails when I use wl-copy (even if it works when I test it manually), and passes when I use xclip (even if it doesn't work when I test it manually, because I'm running wayland).

Is it because of xwayland? The test fails even with QT_QPA_PLATFORM set.

[louib]

@tezkerek did you try using WAYLAND_DISPLAY as you proposed? If this env var is always defined on wayland, that might be the way to go.

[tezkerek]

Yes, that's how I did it, and it works when I use it. But the test fails, maybe because it uses QClipboard to retrieve the clipboard contents. And if I use xclip on Wayland, the test passes, even if trying it manually doesn't work. I don't know if it's a bug in qt, or if it has something to do with xwayland, or something else.

…

On Fri, Feb 28, 2020, 23:07 louib ***@***.***> wrote: @tezkerek <https://github.com/Tezkerek> did you try using WAYLAND_DISPLAY as you proposed? If this env var is always defined on wayland, that might be the way to go. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4323?email_source=notifications&email_token=ADBAIG6ACOPHT2LFPELJF6TRFF4IXA5CNFSM4KUQ2MO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENKFDGQ#issuecomment-592728474>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADBAIG6VLTYFTLKTIZ74X3LRFF4IXANCNFSM4KUQ2MOQ> .

[droidmonkey]

Is this ready to go or do you want to fix the test wayland situation?

[tezkerek]

I could modify it to just skip the clip test if it's running on Wayland, for now.

[louib]

@tezkerek another option could be to fetch the text from the clipboard after pasting to make sure the operation was successful, and fallback on the other programs if it didn't work.

[tezkerek]

@louib How would I fetch the text? QClipboard isn't available in the cli tool, or am I wrong?

[louib]

@tezkerek you're right, it's not available. We could fetch the text using the same command used for clipping, but that would be additional work and I'm not sure it works for all the clipboard apps we use.

[droidmonkey]

Ping @tezkerek @louib whatcha wanna do about this PR?

[tezkerek]

Well, the clip test still fails on Wayland, and I don't know how to debug that. If you're okay with just skipping the test when running on Wayland, it works.

[droidmonkey]

I'm ok with skipping it on Wayland.