From 2a4d3037d7b669cc7b6d5c89e50a3d1eb5a5c8e5 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Thu, 20 Mar 2025 23:16:17 -0700 Subject: [PATCH 01/11] x/term: exposing history based on https://github.com/golang/go/issues/68780#issuecomment-2737508310 --- terminal.go | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/terminal.go b/terminal.go index 14f8947..f7b5195 100644 --- a/terminal.go +++ b/terminal.go @@ -36,6 +36,16 @@ var vt100EscapeCodes = EscapeCodes{ Reset: []byte{keyEscape, '[', '0', 'm'}, } +type History interface { + // adds a new line into the history + Add(string) + + // retrieves a line from history + // 0 should be the most recent entry + // ok = false when out of range + At(idx int) (string, bool) +} + // Terminal contains the state for running a VT100 terminal that is capable of // reading lines of input. type Terminal struct { @@ -86,9 +96,10 @@ type Terminal struct { remainder []byte inBuf [256]byte - // history contains previously entered commands so that they can be + // History allows to replace the default implementation of the history + // which contains previously entered commands so that they can be // accessed with the up and down keys. - history stRingBuffer + History History // historyIndex stores the currently accessed history entry, where zero // means the immediately previous entry. historyIndex int @@ -111,6 +122,7 @@ func NewTerminal(c io.ReadWriter, prompt string) *Terminal { termHeight: 24, echo: true, historyIndex: -1, + History: &stRingBuffer{}, } } @@ -497,7 +509,7 @@ func (t *Terminal) handleKey(key rune) (line string, ok bool) { t.pos = len(t.line) t.moveCursorToPos(t.pos) case keyUp: - entry, ok := t.history.NthPreviousEntry(t.historyIndex + 1) + entry, ok := t.History.At(t.historyIndex + 1) if !ok { return "", false } @@ -516,7 +528,7 @@ func (t *Terminal) handleKey(key rune) (line string, ok bool) { t.setLine(runes, len(runes)) t.historyIndex-- default: - entry, ok := t.history.NthPreviousEntry(t.historyIndex - 1) + entry, ok := t.History.At(t.historyIndex - 1) if ok { t.historyIndex-- runes := []rune(entry) @@ -781,7 +793,7 @@ func (t *Terminal) readLine() (line string, err error) { if lineOk { if t.echo { t.historyIndex = -1 - t.history.Add(line) + t.History.Add(line) } if lineIsPasted { err = ErrPasteIndicator @@ -942,7 +954,7 @@ func (s *stRingBuffer) Add(a string) { // If n is zero then the immediately prior value is returned, if one, then the // next most recent, and so on. If such an element doesn't exist then ok is // false. -func (s *stRingBuffer) NthPreviousEntry(n int) (value string, ok bool) { +func (s *stRingBuffer) At(n int) (value string, ok bool) { if n < 0 || n >= s.size { return "", false } From c4cef2d6e4cbacb44eec678671212ddcbda33e8c Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Thu, 20 Mar 2025 23:35:28 -0700 Subject: [PATCH 02/11] Update NthPreviousEntry->At in godoc too --- terminal.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terminal.go b/terminal.go index f7b5195..4e8ad28 100644 --- a/terminal.go +++ b/terminal.go @@ -950,7 +950,7 @@ func (s *stRingBuffer) Add(a string) { } } -// NthPreviousEntry returns the value passed to the nth previous call to Add. +// At returns the value passed to the nth previous call to Add. // If n is zero then the immediately prior value is returned, if one, then the // next most recent, and so on. If such an element doesn't exist then ok is // false. From c892a3ae102d594e2b002108fb59deb9e322105c Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Fri, 21 Mar 2025 00:38:45 -0700 Subject: [PATCH 03/11] Need unlock before History.Add() --- terminal.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/terminal.go b/terminal.go index 4e8ad28..e5b2aee 100644 --- a/terminal.go +++ b/terminal.go @@ -793,7 +793,9 @@ func (t *Terminal) readLine() (line string, err error) { if lineOk { if t.echo { t.historyIndex = -1 - t.History.Add(line) + t.lock.Unlock() + t.History.Add(line) // so this can output without deadlock. + t.lock.Lock() } if lineIsPasted { err = ErrPasteIndicator From 93da400bfba6ae3dd2eaa83d6bd09ef656bd5f05 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Fri, 21 Mar 2025 01:29:08 -0700 Subject: [PATCH 04/11] Tweak the godoc --- terminal.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/terminal.go b/terminal.go index e5b2aee..2a418df 100644 --- a/terminal.go +++ b/terminal.go @@ -36,13 +36,18 @@ var vt100EscapeCodes = EscapeCodes{ Reset: []byte{keyEscape, '[', '0', 'm'}, } +// The History interface provides a way to replace the default automatic +// 100 slots ringbuffer implementation. type History interface { - // adds a new line into the history - Add(string) - - // retrieves a line from history - // 0 should be the most recent entry - // ok = false when out of range + // Add will be called by Terminal to add a new line into the history. + // An implementation may decide to not add every lines by ignoring calls + // to this function (from Terminal.ReadLine) and to only add validated + // entries out of band. + Add(entry string) + + // At retrieves a line from history. + // idx 0 should be the most recent entry. + // return false when out of range. At(idx int) (string, bool) } @@ -99,6 +104,7 @@ type Terminal struct { // History allows to replace the default implementation of the history // which contains previously entered commands so that they can be // accessed with the up and down keys. + // It's not safe to call ReadLine and methods on History concurrently. History History // historyIndex stores the currently accessed history entry, where zero // means the immediately previous entry. From c52ccb7be0be48ec8a4086516480f5be02b5fc54 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Fri, 21 Mar 2025 09:03:27 -0700 Subject: [PATCH 05/11] Also allow output in History.At - and protect from panic in History methods --- terminal.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/terminal.go b/terminal.go index 2a418df..34a11ee 100644 --- a/terminal.go +++ b/terminal.go @@ -468,6 +468,20 @@ func visualLength(runes []rune) int { return length } +// histroryAt unlocks the terminal and relocks it while calling History.At. +func (t *Terminal) historyAt(idx int) (string, bool) { + t.lock.Unlock() + defer t.lock.Lock() + return t.History.At(idx) +} + +// historyAdd unlocks the terminal and relocks it while calling History.Add. +func (t *Terminal) historyAdd(entry string) { + t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer. + defer t.lock.Lock() // panic in Add protection. + t.History.Add(entry) +} + // handleKey processes the given key and, optionally, returns a line of text // that the user has entered. func (t *Terminal) handleKey(key rune) (line string, ok bool) { @@ -515,7 +529,7 @@ func (t *Terminal) handleKey(key rune) (line string, ok bool) { t.pos = len(t.line) t.moveCursorToPos(t.pos) case keyUp: - entry, ok := t.History.At(t.historyIndex + 1) + entry, ok := t.historyAt(t.historyIndex + 1) if !ok { return "", false } @@ -534,7 +548,7 @@ func (t *Terminal) handleKey(key rune) (line string, ok bool) { t.setLine(runes, len(runes)) t.historyIndex-- default: - entry, ok := t.History.At(t.historyIndex - 1) + entry, ok := t.historyAt(t.historyIndex - 1) if ok { t.historyIndex-- runes := []rune(entry) @@ -799,9 +813,7 @@ func (t *Terminal) readLine() (line string, err error) { if lineOk { if t.echo { t.historyIndex = -1 - t.lock.Unlock() - t.History.Add(line) // so this can output without deadlock. - t.lock.Lock() + t.historyAdd(line) // so this can output without deadlock. } if lineIsPasted { err = ErrPasteIndicator From a06858121c649135e3ec6cce465a861b02371a5f Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Wed, 2 Apr 2025 14:25:53 -0700 Subject: [PATCH 06/11] Adding Len() per latest comment on proposal - even though it's a bit odd to add/expose something that isn't actually used/needed --- terminal.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/terminal.go b/terminal.go index 34a11ee..196ce4a 100644 --- a/terminal.go +++ b/terminal.go @@ -45,6 +45,9 @@ type History interface { // entries out of band. Add(entry string) + // Len returns the current number of entries in the history. + Len() int + // At retrieves a line from history. // idx 0 should be the most recent entry. // return false when out of range. @@ -970,6 +973,10 @@ func (s *stRingBuffer) Add(a string) { } } +func (s *stRingBuffer) Len() int { + return s.size +} + // At returns the value passed to the nth previous call to Add. // If n is zero then the immediately prior value is returned, if one, then the // next most recent, and so on. If such an element doesn't exist then ok is From ba6dd5cfb0cd3b72adac396893918a276c23c6b7 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Wed, 16 Apr 2025 13:47:15 -0700 Subject: [PATCH 07/11] Update to accepted version - At panics() --- terminal.go | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/terminal.go b/terminal.go index 196ce4a..7d9eca2 100644 --- a/terminal.go +++ b/terminal.go @@ -6,6 +6,7 @@ package term import ( "bytes" + "fmt" "io" "runtime" "strconv" @@ -38,20 +39,27 @@ var vt100EscapeCodes = EscapeCodes{ // The History interface provides a way to replace the default automatic // 100 slots ringbuffer implementation. +// A History provides a (possibly bounded) queue of input lines read by [ReadLine]. type History interface { + // Add adds a new, most recent entry to the history. + // It is allowed to drop any entry, including + // the entry being added (e.g.,, if it's a whitespace-only entry), + // the least-recent entry (e.g., to keep the history bounded), + // or any other entry. // Add will be called by Terminal to add a new line into the history. // An implementation may decide to not add every lines by ignoring calls // to this function (from Terminal.ReadLine) and to only add validated // entries out of band. Add(entry string) - // Len returns the current number of entries in the history. + // Len returns the number of entries in the history. Len() int - // At retrieves a line from history. - // idx 0 should be the most recent entry. - // return false when out of range. - At(idx int) (string, bool) + // At returns an entry from the history. + // Index 0 is the most-recently added entry and + // index Len()-1 is the least-recently added entry. + // If index is < 0 or >= Len(), it panics. + At(idx int) string } // Terminal contains the state for running a VT100 terminal that is capable of @@ -475,7 +483,10 @@ func visualLength(runes []rune) int { func (t *Terminal) historyAt(idx int) (string, bool) { t.lock.Unlock() defer t.lock.Lock() - return t.History.At(idx) + if idx < 0 || idx >= t.History.Len() { + return "", false + } + return t.History.At(idx), true } // historyAdd unlocks the terminal and relocks it while calling History.Add. @@ -981,15 +992,15 @@ func (s *stRingBuffer) Len() int { // If n is zero then the immediately prior value is returned, if one, then the // next most recent, and so on. If such an element doesn't exist then ok is // false. -func (s *stRingBuffer) At(n int) (value string, ok bool) { +func (s *stRingBuffer) At(n int) string { if n < 0 || n >= s.size { - return "", false + panic(fmt.Sprintf("stRingBuffer: index [%d] out of range [0,%d)", n, s.size)) } index := s.head - n if index < 0 { index += s.max } - return s.entries[index], true + return s.entries[index] } // readPasswordLine reads from reader until it finds \n or io.EOF. From 5e07c1517b2728f32a143497777d25a6334cb9db Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Wed, 16 Apr 2025 15:01:51 -0700 Subject: [PATCH 08/11] Review updates --- terminal.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/terminal.go b/terminal.go index 7d9eca2..63cf80e 100644 --- a/terminal.go +++ b/terminal.go @@ -481,8 +481,8 @@ func visualLength(runes []rune) int { // histroryAt unlocks the terminal and relocks it while calling History.At. func (t *Terminal) historyAt(idx int) (string, bool) { - t.lock.Unlock() - defer t.lock.Lock() + t.lock.Unlock() // Unlock to avoid deadlock if History methods use the output writer. + defer t.lock.Lock() // panic in At (or Len) protection. if idx < 0 || idx >= t.History.Len() { return "", false } @@ -827,7 +827,7 @@ func (t *Terminal) readLine() (line string, err error) { if lineOk { if t.echo { t.historyIndex = -1 - t.historyAdd(line) // so this can output without deadlock. + t.historyAdd(line) } if lineIsPasted { err = ErrPasteIndicator @@ -994,7 +994,7 @@ func (s *stRingBuffer) Len() int { // false. func (s *stRingBuffer) At(n int) string { if n < 0 || n >= s.size { - panic(fmt.Sprintf("stRingBuffer: index [%d] out of range [0,%d)", n, s.size)) + panic(fmt.Sprintf("term: history index [%d] out of range [0,%d)", n, s.size)) } index := s.head - n if index < 0 { From 57b351e5ca74810484dc0d94d9d595f70a9f53e9 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Thu, 17 Apr 2025 10:16:10 -0700 Subject: [PATCH 09/11] Updated comments - thanks to reviewers --- terminal.go | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/terminal.go b/terminal.go index 63cf80e..3350de5 100644 --- a/terminal.go +++ b/terminal.go @@ -37,19 +37,18 @@ var vt100EscapeCodes = EscapeCodes{ Reset: []byte{keyEscape, '[', '0', 'm'}, } -// The History interface provides a way to replace the default automatic -// 100 slots ringbuffer implementation. -// A History provides a (possibly bounded) queue of input lines read by [ReadLine]. +// A History provides a (possibly bounded) queue of input lines read by [Terminal.ReadLine]. +// +// The default implementation of History provides a simple ring buffer limited +// to 100 slots. Clients can provide alternate implementations of History to +// change this behavior. type History interface { - // Add adds a new, most recent entry to the history. + // Add adds will be called by [Terminal.ReadLine] to add + // a new, most recent entry to the history. // It is allowed to drop any entry, including - // the entry being added (e.g.,, if it's a whitespace-only entry), + // the entry being added (e.g., if it's deemed an invalid entry), // the least-recent entry (e.g., to keep the history bounded), // or any other entry. - // Add will be called by Terminal to add a new line into the history. - // An implementation may decide to not add every lines by ignoring calls - // to this function (from Terminal.ReadLine) and to only add validated - // entries out of band. Add(entry string) // Len returns the number of entries in the history. @@ -112,10 +111,12 @@ type Terminal struct { remainder []byte inBuf [256]byte - // History allows to replace the default implementation of the history - // which contains previously entered commands so that they can be - // accessed with the up and down keys. - // It's not safe to call ReadLine and methods on History concurrently. + // History records and retrieves lines of input read by [ReadLine] which + // a user can retrieve and navigate using the up and down arrow keys. + // + // It is not safe to call ReadLine concurrently with any methods on History. + // + // [NewTerminal] sets this to an implementation that records the last 100 lines of input. History History // historyIndex stores the currently accessed history entry, where zero // means the immediately previous entry. From d9f673b367ed32531331def311e3d9f3b58ebf7f Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Thu, 17 Apr 2025 10:26:08 -0700 Subject: [PATCH 10/11] opsa double add --- terminal.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terminal.go b/terminal.go index 3350de5..f419bf3 100644 --- a/terminal.go +++ b/terminal.go @@ -43,7 +43,7 @@ var vt100EscapeCodes = EscapeCodes{ // to 100 slots. Clients can provide alternate implementations of History to // change this behavior. type History interface { - // Add adds will be called by [Terminal.ReadLine] to add + // Add will be called by [Terminal.ReadLine] to add // a new, most recent entry to the history. // It is allowed to drop any entry, including // the entry being added (e.g., if it's deemed an invalid entry), From 621281355fd577da77f135c4e15552eba3ead584 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Fri, 18 Apr 2025 11:42:52 -0700 Subject: [PATCH 11/11] Review comments --- terminal.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/terminal.go b/terminal.go index f419bf3..13e9a64 100644 --- a/terminal.go +++ b/terminal.go @@ -38,10 +38,6 @@ var vt100EscapeCodes = EscapeCodes{ } // A History provides a (possibly bounded) queue of input lines read by [Terminal.ReadLine]. -// -// The default implementation of History provides a simple ring buffer limited -// to 100 slots. Clients can provide alternate implementations of History to -// change this behavior. type History interface { // Add will be called by [Terminal.ReadLine] to add // a new, most recent entry to the history. @@ -116,7 +112,8 @@ type Terminal struct { // // It is not safe to call ReadLine concurrently with any methods on History. // - // [NewTerminal] sets this to an implementation that records the last 100 lines of input. + // [NewTerminal] sets this to a default implementation that records the + // last 100 lines of input. History History // historyIndex stores the currently accessed history entry, where zero // means the immediately previous entry.