search

isamert / scli

a simple terminal user interface for signal messenger (using signal-cli)
GNU General Public License v3.0
432 stars 40 forks source link

Implement Wayland clipboard support #198

Closed maximbaz closed 1 year ago

maximbaz commented 1 year ago

I finally got annoyed enough with absence of proper Wayland clipboard integration to get my hands on implementing this 😁

This is almost a copy-paste of xclip support, with small refactoring to not duplicate actual logic.

No config required, and no behavior change should be done for existing users of xclip integration.

Fixes #158

exquo commented 1 year ago

Thanks for implementing the Wayland clipboard interaction! And sorry for a late reply!

No config required, and no behavior change should be done for existing users of xclip integration.

👍 That's great!

Some comments on the code:

To reduce the code duplication, I think it's better to re-structure it a bit: make a base Clipboard class for most of the methods, and override a few attributes and methods in derived Xclip and WLclip classes.

It's also preferable to test which clipboard manager is installed (with use_xclip()) only once, rather than every time the clipboard is used.

I've made a rough draft and pushed it in a commit here. Let me know what you think!

maximbaz commented 1 year ago

It's awesome, many thanks for the refactoring!

I will get this tested, but from the first glance I have only one comment: with the current order of executables in get_installed_clipb_manager, wayland clipboard would be prioritized over X11 for people who happened to have both installed. If we do it this way, there is a potential that someone has both installed, and scli would suddenly behave differently for them. Don't know if it's purely theoretical risk, I just wanted to point this out 🙂

maximbaz commented 1 year ago

I fixed some small typos, and now it works as expected 👍

maximbaz commented 1 year ago

@exquo do you think we can finalize this PR? 😊

exquo commented 1 year ago

Yes, I will take another look at it soon! Sorry it had got delayed, appreciate your effort here!

exquo commented 1 year ago

Thanks for the reminder! Yes, it's time to pull this in.

More of the clipboard related code needs to be rewritten. I've started on it in 56dfd25f95fc3752bb3ecb987f396d66a9fc2ff0, but have not completed it yet. However it's already an improvement on the current version, and can be used.

I've added another commit with some more refactoring. If it works for Wayland, I can merge this PR into the main branch.

maximbaz commented 1 year ago

It still works as expected, thanks! During rebase, one of us must have pulled a few extra commits into this PR, which probably don't belong to it - could you confirm? Feel free to drop them if they are not necessary, or let me know and I can do it :+1: Otherwise, lets get this merged :raised_hands:

exquo commented 1 year ago

Thanks for testing! The commits are from the develop branch. I have changed the merge-to branch to develop, and only the commits relevant to this PR show up.

maximbaz commented 1 year ago

Ahh, makes sense! Feel free to merge when you think it's ready, I have no more comments 😉