[term] term: Allow multi-line bracketed paste to not create single line with LF entry
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
term: Allow multi-line bracketed paste to not create single line with LF entry
Treat `\n` (LF) like "Enter"
Avoids when pasting 3 lines (with a terminal like kitty, ghostty etc.. that doesn't change the clipboard in bracketed paste mode):
```
Test> line one
line two
line three
```
Fixes golang/go#74600
Change diff
diff --git a/terminal.go b/terminal.go
index 13e9a64..a22a7aa 100644
--- a/terminal.go
+++ b/terminal.go
@@ -146,6 +146,7 @@
keyCtrlD = 4
keyCtrlU = 21
keyEnter = '\r'
+ keyLF = '\n' // technically not a key (unless a user uses Ctrl+J), but needed for bracketed paste mode with `\n`s.
keyEscape = 27
keyBackspace = 127
keyUnknown = 0xd800 /* UTF-16 surrogate area */ + iota
@@ -567,7 +568,7 @@
t.setLine(runes, len(runes))
}
}
- case keyEnter:
+ case keyEnter, keyLF:
t.moveCursorToPos(len(t.line))
t.queue([]rune("\r\n"))
line = string(t.line)
Change information
- M terminal.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. The first word in the commit title after the package should be a lowercase English word (usually a verb).
2. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 130 character line.
3. Are you using markdown? Markdown should not be used to augment text in the commit message.
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Pratt
| Commit-Queue | +1 |
1 comment
Test> line one
..............line.two
......................line.threeThis issue reminds me of all the annoyances I used to have using screen with serial connections with microcontrollers. :)
----
Do we have any documentation on the specific standard (or reference implementation) is meant to match? The immediate documentation just says "VT100", which seems a bit vague to me (but maybe it's not, TBH I'm not super familiar with terminal details).
For example, is this intended to behave like the Unix 98 psuedoterminal you get from posix_openpt (https://man7.org/linux/man-pages/man7/pty.7.html) with the default termios (https://man7.org/linux/man-pages/man3/termios.3.html)? That seems like a reasonable target to me, but I'm not sure if that is the intention.
Strictly speaking, I believe the standard definition is that line feed moves to the next line, without changing horizontal position. And carriage return moves the cursor to the leftmost column, without changing vertical position. Hence CRLF to go to the beginning of the next line. It sounds like this is the current behavior of Terminal.
But many terminal implementations do modify this behavior.
On my machine, it looks like the default termios are `ioctl(0, TCGETS, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0`.
`ICRNL` is "Translate carriage return to newline on input", which literally replaces an input `\r` with `\n` [1].
`ONLCR` is "Map NL to CR-NL on output", which prints `\r\n` when outputting `\n`.
Combined, I believe these options yield the same behavior as you've implemented in this CL [2].
Still, without knowing exactly what kind of terminal Terminal is supposed to emulate [3], it's hard to say if this CL is OK, or if it would break users.
[1] gVisor has a reasonably complete psuedoterminal implementation, which I find much more readable as a reference than Linux itself: https://github.com/google/gvisor/blob/master/pkg/sentry/fsimpl/devpts/line_discipline.go#L453-L456
[2] I think it might be easier to see this equivalence if this CL was reframed in the same terms as replacing input `\r` with `\n`. I'm not sure how difficult that would be.
[3] We can see that https://cs.opensource.google/go/x/term/+/master:terminal.go;l=570-572;drc=a809085bff595080269599dbe788dd7fdf616ab4 hard-codes the `ONLCR` behavior.
(Apologies in advance, I'm sure I mixed up LF and CR somewhere above)
Related details
- Austin Clements
- Jorropo
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 2 comments![]( "Open in Gerrit")
thanks a lot for having looked, here are some notes/lmk next steps
Test> line one
..............line.two
......................line.threeThis issue reminds me of all the annoyances I used to have using screen with serial connections with microcontrollers. :)
----
Do we have any documentation on the specific standard (or reference implementation) is meant to match? The immediate documentation just says "VT100", which seems a bit vague to me (but maybe it's not, TBH I'm not super familiar with terminal details).
For example, is this intended to behave like the Unix 98 psuedoterminal you get from posix_openpt (https://man7.org/linux/man-pages/man7/pty.7.html) with the default termios (https://man7.org/linux/man-pages/man3/termios.3.html)? That seems like a reasonable target to me, but I'm not sure if that is the intention.
Strictly speaking, I believe the standard definition is that line feed moves to the next line, without changing horizontal position. And carriage return moves the cursor to the leftmost column, without changing vertical position. Hence CRLF to go to the beginning of the next line. It sounds like this is the current behavior of Terminal.
But many terminal implementations do modify this behavior.
On my machine, it looks like the default termios are `ioctl(0, TCGETS, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0`.
`ICRNL` is "Translate carriage return to newline on input", which literally replaces an input `\r` with `\n` [1].
`ONLCR` is "Map NL to CR-NL on output", which prints `\r\n` when outputting `\n`.
Combined, I believe these options yield the same behavior as you've implemented in this CL [2].
Still, without knowing exactly what kind of terminal Terminal is supposed to emulate [3], it's hard to say if this CL is OK, or if it would break users.
[1] gVisor has a reasonably complete psuedoterminal implementation, which I find much more readable as a reference than Linux itself: https://github.com/google/gvisor/blob/master/pkg/sentry/fsimpl/devpts/line_discipline.go#L453-L456
[2] I think it might be easier to see this equivalence if this CL was reframed in the same terms as replacing input `\r` with `\n`. I'm not sure how difficult that would be.
[3] We can see that https://cs.opensource.google/go/x/term/+/master:terminal.go;l=570-572;drc=a809085bff595080269599dbe788dd7fdf616ab4 hard-codes the `ONLCR` behavior.
(Apologies in advance, I'm sure I mixed up LF and CR somewhere above)
So the issue shows up in the rather new-ish (as in VT100 didn't have "paste" capabilities that I am aware of) mode of bracketed paste - the goal of which is to preserve whatever it is being pasted, think of it for instance as
```
cat > myfile
```
which could have anything including in theory binary/NULs and characters shouldn't be changed. In that context a multi line paste does preserve the \n (on machines where LF is the line terminator)
The slight change of behavior with this CL is that now if you hit "Ctrl-J" while term.ReadLine() is running it will act like if it was a CR / Enter - which I don't think should possibly break anything (but cue in the space bar workflow xkcd)
I have a program that I wrote to debug / step on this issue (of difference of behavior between apple's terminal and newer ones like ghostty)
Before this CL LF is just ignored and multi line commands are not a possibility anyway - outside of embedded CR+LF (from a paste)
Re reframing - you want to me update the title or is
> Treat "\n" (LF) like "Enter" (CR)
enough in the decsription?
Related details
- Austin Clements
- Jorropo
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
Also [3] is just that in raw mode you need CR+LF that's about output not input
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Austin Clements
- Jorropo
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt added 1 comment![]( "Open in Gerrit")
Test> line one
..............line.two
......................line.threeI think I understand the problem you are having, and I agree that the new behavior seems nicer, but it's still not clear to me if we should change behavior unilaterally like this.
e.g., this type has explicit support for bracketed paste (https://go.dev/cl/171330043). Should we be changing behavior only when paste is active (`t.pasteActive`)?
Related details
- Austin Clements
- Jorropo
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
Laurent Demailly added 1 comment![]( "Open in Gerrit")
Yes, we could limit it to paste mode only with a few more ifs/checks, but what is the downside of treating LF/CR(and CR+LF) all the same when the api is about reading a single line? It seems to me it's more likely to work in all cases, with all keyboards and possible new lines in input / terminal / if caller are forgetting to set paste mode.
Is it a positive feature that without this change, on unix for instance, if I hit ctrl-J (ie send a LF) nothing happens (it's ignored/skipped/not added to the line)
Let me add tests that are missing.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Austin Clements
- Jorropo
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Austin Clements
- Jorropo
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Laurent Demailly (Gerrit)
@mpr...@google.com did I clarify, do the tests I added help? next steps?
Thanks
Related details
- Austin Clements
- Jorropo
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Pratt
| Code-Review | +2 |
| Commit-Queue | +1 |
1 comment
Test> line one
..............line.two
......................line.threeI am OK with the CL as-is, the behavior does seem better. However I'm still not 100% confident that we won't hear from someone broken with a case we aren't thinking of. So perhaps we'll need to change this, but we can play it by ear.
Thanks for adding the tests.
Related details
- Austin Clements
- Jorropo
- Laurent Demailly
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Austin Clements
- Jorropo
- Laurent Demailly
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Pratt (Gerrit)
Michael Pratt submitted the change![]( "Open in Gerrit")
Change information
term: allow multi-line bracketed paste to not create single line with verbatim LFs
Treat "\n" (LF) like "Enter" (CR)
Avoids that when pasting 3 lines
(with a terminal like kitty, ghostty, alacritty that do not change the clipboard
in bracketed paste mode)
it turns into 1 prompt looking like:
Test> line one
..............line.two
......................line.three
Fixes golang/go#74600
- M terminal.go
- M terminal_test.go
Code-Review: +2 by Michael Pratt, +1 by Michael Knyszek
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |