Skip to content

add a cross platform clip command to the standard library#8695

Merged
fdncred merged 13 commits intonushell:mainfrom
amtoine:feature/stdlib/cross-platform-clip-command
Apr 5, 2023
Merged

add a cross platform clip command to the standard library#8695
fdncred merged 13 commits intonushell:mainfrom
amtoine:feature/stdlib/cross-platform-clip-command

Conversation

@amtoine
Copy link
Copy Markdown
Member

@amtoine amtoine commented Mar 31, 2023

Should close the associated poin in #8311

Description

this PR adds a clip command to std which

  • has error support in case the clipboard system command is not installed
  • removes any ANSI escape sequences from the input to have a clean copied text
  • can throw desktop notifications on linux in case of a long pipeline which needs to be copied (can be disabled with --no-notify)
  • echoes the copied data to the output of the terminal (can be disabled with --silent)
  • has examples and dependencies listed
  • has "charpage" 65001 (a.k.a. utf-8) support for windows

new additions from 9cd3c95 to ad3e8de

  • better OS context error support
  • use of the match statement for scalability
  • match the value of the $nu.os-info.name
  • add macOS with pbcopy to the list of supported systems
  • throw an error when the OS is not supported
  • add simple tests to make sure clip works on all main OSes with the CI had to be removed in 6028b98 (see the failing test)

User-Facing Changes

users can now access a clip to copy the output of any pipeline to the system clipboard, on any system

Tests + Formatting

$nothing

After Submitting

$nothing

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Mar 31, 2023

Cool. A few more cases that might be nice to handle (not saying they need to happen in this PR):

  1. If we're using SSH, try setting the clipboard using OSC 52
  2. If we're in WSL, do some magic to set the Windows clipboard

I maintain a (very bad) Rust crate that uses some of these tricks: https://github.com/rgwood/clipboard-anywhere

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 31, 2023

you could also add if it's a mac, use pbcopy. I can give better syntax later. It may be best to use $nu.os-info.name to check the os.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 1, 2023

you could also add if it's a mac, use pbcopy. I can give better syntax later. It may be best to use $nu.os-info.name to check the os.

thanks for the name of the clipboard command, i've added it right away 👌

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 1, 2023

@rgwood

Cool. A few more cases that might be nice to handle (not saying they need to happen in this PR):

  1. If we're using SSH, try setting the clipboard using OSC 52
  2. If we're in WSL, do some magic to set the Windows clipboard

these are very good and sensible ideas !

could we keep them for another PR?
i can open an issue right after this to track the ssh and wsl clipboard features 😋

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 1, 2023

i've added some tests to make sure the CI passes, but xclip is not installed in the ubuntu branch of the CI 🤔

what should we do?

  • add the xclip command to this system
  • remove the test
  • change the test to make sure it fails

@fdncred fdncred added the A:std-library Defining and improving the standard library written in Nu label Apr 4, 2023
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 4, 2023

what should we do?

I'd just add it I think.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

I'd just add it I think.

there it is in 6404d0e then 😉

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

@fdncred

i had to remove the tests in 6028b98 because of this Can't open display: (null) error...

if you have an idea about that 😇

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 4, 2023

I'm thinking that can't open display problem may be because the DISPLAY env var isn't set? I think it needs to be something like DISPLAY=':0'. But that's just a guess.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

I'm thinking that can't open display problem may be because the DISPLAY env var isn't set? I think it needs to be something like DISPLAY=':0'. But that's just a guess.

but in a non-graphical environment like this? 😮

i'll try on my test repo to avoid messing around here

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

i think it's just that the CI is not a graphical environment and thus xclip won't work 🤔

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 4, 2023

i think it's just that the CI is not a graphical environment and thus xclip won't work 🤔

ya, maybe so. i was just guessing.
https://askubuntu.com/questions/305654/xclip-on-headless-server
https://unix.stackexchange.com/questions/316715/xclip-works-differently-in-interactive-and-non-interactive-shells

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 4, 2023

xsel looked promising but it does not work either when DISPLAY is not set 😕

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 5, 2023

Thanks

@fdncred fdncred merged commit 22142bd into nushell:main Apr 5, 2023
@amtoine amtoine deleted the feature/stdlib/cross-platform-clip-command branch April 5, 2023 20:02
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Apr 5, 2023

Thanks

a bit sad the tests did not go well though 🤔

amtoine added a commit to goatfiles/nu_scripts that referenced this pull request Apr 6, 2023
The clip command has been introduced to the standard library in
nushell/nushell/pull/8695 and has been available since
nushell/nushell/pull/8627.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:std-library Defining and improving the standard library written in Nu

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants