Skip to content

Add screen.SetClipboard #562

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add screen.SetClipboard #562

wants to merge 2 commits into from

Conversation

Consolatis
Copy link

Fixes #561

I tested this using the following additional change to always prefer dynamic loading of terminfo data:

diff --git a/tscreen.go b/tscreen.go
index 980f00b..537e21b 100644
--- a/tscreen.go
+++ b/tscreen.go
@@ -50,13 +50,14 @@ func NewTerminfoScreen() (Screen, error) {
 // LookupTerminfo attempts to find a definition for the named $TERM falling
 // back to attempting to parse the output from infocmp.
 func LookupTerminfo(name string) (ti *terminfo.Terminfo, e error) {
-	ti, e = terminfo.LookupTerminfo(name)
-	if e != nil {
-		ti, e = loadDynamicTerminfo(name)
+	ti, e = loadDynamicTerminfo(name)
+	if e == nil {
+		terminfo.AddTerminfo(ti)
+	} else {
+		ti, e = terminfo.LookupTerminfo(name)
 		if e != nil {
 			return nil, e
 		}
-		terminfo.AddTerminfo(ti)
 	}
 
 	return

@Consolatis Consolatis mentioned this pull request Sep 26, 2022
Closed
@Consolatis Consolatis force-pushed the feature/set_clipboard branch from 9bcbde8 to 55866a9 Compare September 26, 2022 10:14
@Consolatis Consolatis force-pushed the feature/set_clipboard branch from 55866a9 to 420210e Compare September 26, 2022 10:15
gdamore
gdamore reviewed Oct 15, 2022
View reviewed changes
Copy link
Owner

@gdamore gdamore left a comment

Choose a reason for hiding this comment

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

This looks like a good start.

A couple of thoughts.

  1. Should we update some of the terminfo descriptions -- it would be good if at least some of the terminal types got this behavior. Certainly I believe at a minimum xterm-256color should have it.

  2. I feel like we might also want the ability to get the contents of the paste buffer, although this might be considered contentious I suppose. But think about an application that wants to have a "paste" -- of course there are parts about this that might be concerning from a security perspective, but honestly I don't think we'd be making the situation any worse as a malicious application could still just encode these sequences directly. I imagine that enforcement of the get (the "?" parameter) would need to be done by the terminal emulator itself.

@gdamore
Copy link
Owner

gdamore commented Oct 15, 2022

Also, perhaps you want to update AUTHORS?

@Consolatis
Copy link
Author

Consolatis commented Oct 16, 2022 •
edited
Loading

Should we update some of the terminfo descriptions -- it would be good if at least some of the terminal types got this behavior. Certainly I believe at a minimum xterm-256color should have it.

I could update terminfo/mkinfo.go to include the Ms value. When generating the built-in terminfo files myself they are slightly different though so it might be best to regenerate them on the same system / distribution that generated the current ones.
Would that be enough?

I feel like we might also want the ability to get the contents of the paste buffer, although this might be considered contentious I suppose. But think about an application that wants to have a "paste" -- of course there are parts about this that might be concerning from a security perspective, but honestly I don't think we'd be making the situation any worse as a malicious application could still just encode these sequences directly. I imagine that enforcement of the get (the "?" parameter) would need to be done by the terminal emulator itself.

I am not sure how we can check for support of that feature dynamically. For setting the clipboard its easy: just use the Ms terminfo entry and if it doesn't exist its not supported (or the user thinks its a good idea to fake their TERM env var). And as this also requires to parse the response from the terminal I am not sure how to deliver the paste asynchronously to the application, maybe we could reuse the bracketed paste event or something along those lines. In any case I think that feature requires some more dev and design work so we could decouple it from just setting the Clipboard which is pretty straight forward.

Also, perhaps you want to update AUTHORS?

Thanks but there is no need for that.

@gdamore
Copy link
Owner

gdamore commented Oct 17, 2022

I want to think on this.

I've been thinking about refactoring some of the "extended" terminal handling, because honestly almost everyone wants all the extra features and they rarely have all the capabilities in terminfo.

@delthas
Copy link
Contributor

delthas commented Dec 2, 2022

As a TUI instant messaging client developer, I'm interested in having a way to read the system clipboard (in order to upload images easily, from the clipboard). IMO it's fine to have this in tcell wrt security, it's up to the terminal emulator whether to allow this or drop the request.

@Consolatis
Copy link
Author

Consolatis commented Jan 9, 2023 •
edited
Loading

Regarding the added example here maybe it would be best to strip that from this PR and instead create a new example which provides a simple menu driven by Up / Down / Return and allows to test multiple features like setting the clipboard / title / other things in the future.

Edit:

Should we update some of the terminfo descriptions -- it would be good if at least some of the terminal types got this behavior. Certainly I believe at a minimum xterm-256color should have it.

Also at least foot and tmux and their -256color and -direct variants.

@gdamore
Copy link
Owner

gdamore commented Aug 15, 2023

I am close to wanting to merge this -- the only thing I really don't like is that the only way that people get this is with the extended query and using the dynamic terminfo. I really believe this should be included by default.

@Consolatis Consolatis mentioned this pull request Aug 29, 2023
Closed
@Consolatis
Copy link
Author

I am close to wanting to merge this -- the only thing I really don't like is that the only way that people get this is with the extended query and using the dynamic terminfo. I really believe this should be included by default.

Well, it could be hardcoded instead but I have absolutely no idea which of the fake TERM=xterm-256color terminals support Ms and which do not. I know foot does support it though. Same for tmux when enabled via config.

Alternative as written above:

I could update terminfo/mkinfo.go to include the Ms value. When generating the built-in terminfo files myself they are slightly different though so it might be best to regenerate them on the same system / distribution that generated the current ones.
Would that be enough?

mkinfo.go btw also uses the extended flag.

@gdamore
Copy link
Owner

gdamore commented Mar 10, 2024

So I'm looking in more depth at this.

This has copying data to the clipboard, but not retrieval. That's fine for now.

I think retrieval is going to require a new event type. Both forms are tied to base64 encoding.

gdamore added a commit that referenced this pull request Mar 10, 2024
@gdamore
fixes #561 Add clipboard support.
78110e3 
This is not supported for Windows or WebAssembly yet.
It's possible for applications to post to the clipboard using
Screen.SetClipboard (any data), and they can retrieve the clipboard
(if permitted) using GetClipboard.  The terminal may well reject either
of these.

Retrieval will arrive as a new EventClipboard, if it can.  (There is
no good way to make this synchronous.)

This work was inspired by a PR submitted by Consolatis (#562), and
has some work based on it, but it was also substantially improved and
now includes both sides of the clipboard access pattern.
@gdamore gdamore mentioned this pull request Mar 10, 2024
Merged
@gdamore
Copy link
Owner

gdamore commented Mar 10, 2024

I have implemented a superset of this functionality in #714. Additional it works across a very much wider set of terminals.

I am not using the Ms sequence to detect this. In fact, I've become disenchanged with terminfo entirely. Instead I look for XT (xterm title title support), or a terminal name starting with "xterm", and if I find it, I assume it supports this capability.

Only POSIX terminal support is done -- no support for this on Windows or WebAssembly at this point.

@gdamore gdamore closed this Mar 10, 2024
@Consolatis Consolatis deleted the feature/set_clipboard branch March 11, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdamore gdamore gdamore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Support for setting clipboard
3 participants
@Consolatis @gdamore @delthas