Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

visidata: add runtime dependency of clipboard commands #182011

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

AndrewKvalheim
Copy link
Contributor

Description of changes

VisiData’s system clipboard commands (e.g. Edit → Copy → to system clipboard) currently fail with:

FileNotFoundError: [Errno 2] No such file or directory: 'xclip'

This can be corrected in the user’s configuration, and for e.g. Wayland you might prefer to specify different commands anyway—

{
  home.file.".visidatarc".text = ''
    options.clipboard_copy_cmd = '${wl-clipboard}/bin/wl-copy'
    options.clipboard_paste_cmd = '${wl-clipboard}/bin/wl-paste --no-newline'
  '';
}

—but it would be nice if the defaults worked out of the box.

https://github.com/saulpw/visidata/blob/v2.8/visidata/clipboard.py#L18-L19

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nixpkgs-review rev visidata/xclip
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@markus1189 markus1189 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using xclip as the default command assumes that stdenv.isLinux = true implies the environment is also using X11. With is not necessarily true, it might be Wayland. I don't know if xclip works in that case at all, does somebody know it?

@AndrewKvalheim
Copy link
Contributor Author

AndrewKvalheim commented Jul 19, 2022

Yep, it’s naive, but that’s the behavior of the application. For what it’s worth, xclip does appear to work in my Wayland GNOME environment.

The other case worth considering is non-graphical environments, and whether the extra dependency (or visidata.override { xclip = null; }) is unacceptably burdensome there.

If we wanted to get into more elaborate packaging, pass might be a useful example/precedent.

@markus1189
Copy link
Contributor

What do you think about having something like:

lib.optionals withXclip [ xclip ];

instead? And then we could default withXclip to stdenv.isLinux

VisiData’s system clipboard commands—

  - Edit → Copy → to system clipboard → …
  - Edit → Paste → from system clipboard → …

—execute subprocesses defined by configuration options:

  - options.clipboard_copy_cmd
  - options.clipboard_paste_cmd

On Linux, these options default to using `xclip`:

  - https://github.com/saulpw/visidata/blob/v2.8/visidata/clipboard.py#L11-L22

Without it, the system clipboard commands fail with:

    FileNotFoundError: [Errno 2] No such file or directory: 'xclip'
@AndrewKvalheim
Copy link
Contributor Author

That sounds good, thanks. I’m still new to this though so don’t give my input too much weight. :)

@ofborg ofborg bot requested a review from markus1189 July 20, 2022 22:43
@RamKromberg
Copy link
Contributor

Partially off-topic, if anyone runs into copy-pasting type issues with visidata, be sure to update xclip before assuming it's a visidata bug. Upstream stopped doing point releases so the version in nixpkgs is 8 years behind:
https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/misc/xclip/default.nix
https://github.com/astrand/xclip/commits/master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants