-
Notifications
You must be signed in to change notification settings - Fork 270
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
feat(textinput): store and recover history #631
base: master
Are you sure you want to change the base?
Conversation
This adds the history functionality to the textinput model. Specific actions can push a new value into the history and they can be switched back to via key presses. This is done in relation to nobe4/gh-not#190 I was considering implementing that in my project, and realized that it could just be part of bubbles instead. Looking forward to getting some feedback on this. Especially the comments below.
// XXX: It seems that doing `append(m.history, m.value)` copies a reference | ||
// to the value, which leads to all history items being similar. | ||
// This is quite surprising given this other test: | ||
// https://go.dev/play/p/MZGpcUzSVch | ||
newRecord := make([]rune, len(m.value)) | ||
for i, r := range m.value { | ||
newRecord[i] = r | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels suboptimal, but I didn't find any alternative solution.
// TODO: discuss moving `down` and `up` to this instead of Next/PrevSuggestion | ||
NextHistory: key.NewBinding(key.WithKeys("down")), | ||
PrevHistory: key.NewBinding(key.WithKeys("up")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up and Down feel more natural to me, as a shell/vim user than updating completion. However this might break existing applications and change the default behavior, so I'd be happy not to change it.
Which keys would you recommend?
// TODO: rename getSuggestions to getStrings | ||
return m.getSuggestions(m.history) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The m.getSuggestions
really just transforms a [][]rune
into a []string
, so the name could be updated to reflect this.
Thanks for the PR! So right off the cuff, I sort of wonder if this should be implemented externally, possibly with an interface. That would allow to, for example, store large amounts of history via sqlite, or store and fetch history over the network. |
text input already have the suggestions feature, which can be used as a history. fwiw we also have the same feature in the works for textarea. if you don't like how suggestions work, you can also disable the keybinding and wrap textinput, setting the actual contents of the history as the value of the textinput (instead of ghost text). |
Possibly! Do you think this would be more worthwhile? If so, would the interface be a member of the I like the idea of decoupling the two. The interface could look like: type HistoryManager interface {
Save(string) error
Get() (string, error)
Next()
Previous()
}
It can, but if I want to use both suggestion and history, then I would be stuck :) Coming from (Neo)Vim, having the completion + the history is incredibly useful to repeat command. |
This adds the history functionality to the textinput model. Specific actions can push a new value into the history and they can be switched back to via key presses.
This is done in relation to nobe4/gh-not#190 I was considering implementing that in my project, and realized that it could just be part of bubbles instead.
Looking forward to getting some feedback on this. Especially the comments below.
Example usage