Implementation golang/term#15 demo of use/benefit fortio/terminal@77eee7b

Change https://go.dev/cl/603957 mentions this issue: x/term: expose History() AddToHistory() and NewHistory() so users of the library can save and restore the history.

I don't know if this is a good proposal or not, but if you add History() it should return iter.Seq[string].

I don't know if this is a good proposal or not, but if you add History() it should return iter.Seq[string].

Thanks for looking. If you look at the code you'll see it creates a copy while holding the lock. An iterator wouldn't be helpful as the copy (snapshot) is needed.

Hey Laurent!

I was about to push for review my patches for almost the same thing, hence would rather not waste everyone's time again and join you here.

I also did a terminal.PopHistory(), which I used when the user was just hitting Enter to clear the screen a bit. I think your terminal.ReplaceLatest() wouldn't cover this use case, of skipping the last command entirely from history, correct?

And what would you think is the best approach to achieve that? A modification to ReplaceLatest() (e.g. return a *string and nil means pop it), or the following that I already did in my patch?

// Pop the last element from the history.
func (s *stRingBuffer) Pop() (string, bool) {
	if s.size == 0 {
		return "", false
	}
	value := s.entries[s.head]
	s.head--
	if s.head < 0 {
		s.head += s.max
	}
	if s.size > 0 {
		s.size--
	}
	return value, true
}
func (t *Terminal) PopHistory() (string, bool) {
	t.lock.Lock()
	defer t.lock.Unlock()

	return t.history.Pop()
}

Or I guess also using AutoHistory(false) and then adding just the non-empty lines to history would work too...

Would you care to include this too, or you'd rather prefer that I'll push a PR after the current code is merged?

Cheers,
-Dragos

Thanks, yes I ended up mostly turning auto history off for real cases and adding depending on conditions, the on behavior is for backward compatibility with current behavior. I was using replace until I hit writing a "history" (and !nn repeat) which shifts the history by one making auto not useable, maybe for minimalism I should remove replace?

Edit: I wouldn't mind replacing Replace by Pop btw if that works better

I was buying though your explanation for what ReplaceLatest() could be useful 😉. Yet one could achieve a similar effect also without, since you're also implementing AddToHistory().

So the question probably is: would AutoHistory(true) really be used beyond backwards-compatibility? If yes, then ReplaceLatest() and Pop() would be useful and worth adding (yet also of limited use, because next someone would want access to other entries, not just the last).

IMHO the usage pattern of these APIs is:

• first users let AutoHistory(true) as default or are oblivious to its existence;
• more advanced users which want more control will set AutoHistory(false);
  • then AddToHistory() would be enough for most scenarios;
  • for the rest you have already built a way to get-all, (modify), clear, set-back-all; so I'd say it's not worth adding more complexity for eventual efficiency savings.

tl;dr - I'm thinking neither ReplaceLatest() and Pop() might be significant additions. There are many ways to get to the same end state. I'd go with the simplest.

And remember that once an API is added, there will always be some using it forever, along with all it's undocumented or unintended side-effects 😛.

So I use replace in the example but it's true I just set auto to false instead in my real code. I can drop it from the proposal.

I just want to say though that I wanted to avoid people clearing the whole history and re-adding to change last entry as even with a relatively small 99 entry slice it'd be wasteful

lastly there is one other reason for replace which is to let users use up arrow and find the last bad entry and correct it

ping, anyone?