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

Clipboard graceful handling #712

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

abusch
Copy link
Contributor

@abusch abusch commented Jan 22, 2024

If the system_clipboard feature is enabled, gracefully handle the case where the system clipboard can't be accessed and default to LocalClipboard in that case instead of panicking. This should help with running nu scripts in a docker container for instance.

@nibon7
Copy link
Contributor

nibon7 commented Jan 22, 2024

This fixes the panic in wayland mode. 👍

@kubouch
Copy link
Contributor

kubouch commented Jan 22, 2024

It seems like adding the system_clipboard feature job to the CI caused the failure. I'm not sure why the Wayland dependency requires a nightly, not how to fix it. Is the extra CI step necessary? It would be great to land this because currently the latest Nushell built from main panics when launched without a window manager.

@Tastaturtaste
Copy link
Contributor

Hey, I had the same problem in nushell. See this comment: nushell/nushell#11535 (comment)

@Tastaturtaste
Copy link
Contributor

and default to LocalClipboard in that case instead of panicking

Please provide a big fat warning for the user in that case, I would be quite surprised if that happened silently to me

@abusch
Copy link
Contributor Author

abusch commented Jan 22, 2024

Is the extra CI step necessary?

No, I thought it was an oversight that the feature had been missed, but I think it might have been on purpose. There are several tests that use the clipboard, so when run in parallel they step on each other's toes and cause failures. The system clipboard is basically a big global mutable state. I think I'll remove the feature from the CI matrix.

If the `system_clipboard` feature is enabled, but we failed to
initialize the `SystemClipboard`, default to `LocalClipboard` instead of
panicking.
@abusch abusch force-pushed the clipboard_graceful_handling branch from e941078 to 40221bc Compare January 22, 2024 22:04
@abusch
Copy link
Contributor Author

abusch commented Jan 22, 2024

and default to LocalClipboard in that case instead of panicking

Please provide a big fat warning for the user in that case, I would be quite surprised if that happened silently to me

I do have a eprintln!("Defaulting to local clipboard!"); when that happens, but I'm not sure if that's the best way.

Also, I rebased my branch on main and now I can't test it with nushell due to conflicting versions of nu-ansi-term. I'll have to wait until that's resolved to see if the warning gets printed.

@nibon7
Copy link
Contributor

nibon7 commented Jan 23, 2024

I do have a eprintln!("Defaulting to local clipboard!"); when that happens, but I'm not sure if that's the best way.

Nushell prints this message every time when XWayland is not started, I think it's quite annoying :(

@abusch
Copy link
Contributor Author

abusch commented Jan 23, 2024

I do have a eprintln!("Defaulting to local clipboard!"); when that happens, but I'm not sure if that's the best way.

Nushell prints this message every time when XWayland is not started, I think it's quite annoying :(

oh, does nushell recreate the whole reedline instance for every prompt? 🤔

Or did you mean every time a new shell is started?

@kubouch
Copy link
Contributor

kubouch commented Jan 23, 2024

Yes, I believe that it does (you can check nu-cli/src/repl.rs in Nushell). I'd also prefer to have this message removed because there seems to be no way to turn it off. I haven't tested it myself, though. @nibon7 does it print the line on every prompt?

Instead of eprintln!, it would be better to be able to somehow query information about Reedline where you could check what is your current clipboard, for example.

@abusch
Copy link
Contributor Author

abusch commented Jan 23, 2024

Yes, I believe that it does (you can check nu-cli/src/repl.rs in Nushell). I'd also prefer to have this message removed because there seems to be no way to turn it off. I haven't tested it myself, though. @nibon7 does it print the line on every prompt?

From what I can tell, it doesn't (the reedline instance seems to be created outside of the repl loop).

Instead of eprintln!, it would be better to be able to somehow query information about Reedline where you could check what is your current clipboard, for example.

To be honest, I don't think I meant to commit that eprintln! 😆 so I'm happy to remove it.

I think the only potentially confusing case would be if you are running nu in a graphical environment but there is something broken in your setup somehow which would make it silently default to the local clipboard. I guess in this case being able to query the clipboard type could be useful...

@abusch
Copy link
Contributor Author

abusch commented Jan 23, 2024

An alternative might be to expose a no_system_clipboard config option / CLI switch in nushell that would make it always use the local clipboard (even if compiled with the system_clipboard feature), which would be how you'd turn the warning message off. But that's likely quite a bit more work 😅

@kubouch
Copy link
Contributor

kubouch commented Jan 23, 2024

From what I can tell, it doesn't (the reedline instance seems to be created outside of the repl loop).

There is Reedline::create() also inside the loop, but I haven't checked the code carefully.

Could you then remove the eprintln! so that we can merge it? We can then iterate on some better diagnostics, but let's have the unwrap fix merged first because it's urgent.

@abusch
Copy link
Contributor Author

abusch commented Jan 23, 2024

@kubouch done.

@kubouch
Copy link
Contributor

kubouch commented Jan 23, 2024

Awesome, thanks!

@kubouch kubouch merged commit 7d08fc8 into nushell:main Jan 23, 2024
6 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Jan 23, 2024

I'm wondering if all this is because of our choice to use arboard vs copypasta? Any guess if copypasta would have the same issues? @Tastaturtaste did you happen to test with both?

@Tastaturtaste
Copy link
Contributor

If by "all of this" you mean having to handle the absence of an available system clipboard and in particular on Linux CI, then I am pretty certain it would be the same with copypasta.
I didn't test with both, but I took a look at both of their CIs and they both specifically exclude Linux. I don't think they do that just for giggles.

@fdncred
Copy link
Collaborator

fdncred commented Jan 23, 2024

ya, sorry i wasn't more clear, "all of this" means the ci failures we're having due to the clipboard mess, like you said. thanks for the info.

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

Successfully merging this pull request may close these issues.

5 participants