enable deleting history items with keybinding #567
Conversation
There was a problem hiding this comment.
Sounds like an interesting addition.
I can see the concern from fish-shell/fish-shell#9515 that it possibly should have a consistent behavior when not in history traversal mode.
Before landing this we should give it some adverserial testing trying to break it manually.
I assume for the sqlite history you were able to get it to correctly delete? It would be great if we could get it to consistently purge the text file based history as well, I think if we provide the feature the users will have the asssumption that it will delete their secrets consistently.
| .history_cursor | ||
| .delete_item_at_cursor(self.history.as_mut()) |
There was a problem hiding this comment.
Do we need to update what is visible in the traditional Ctrl-r search? (this is not the menu but the reedline::ReedlineEvent::SearchHistory triggered search.)
There was a problem hiding this comment.
If you mean self.update-buffer_from_history(), it doesn't seem like it, as history_search_paint() is used instead of buffer_paint(), and that function doesn't support SubstringSearch.
src/history/file_backed.rs
Outdated
| if self.len_on_disk <= id { | ||
| self.entries.remove(id); | ||
| Ok(()) | ||
| } else { |
There was a problem hiding this comment.
I think this really needs to be covered by tests. The len_on_disk starts to get interesting when we need to trim the buffer and other sessions are potentially having a say during .sync()
|
I still need to do the tests. I made it so it clears the buffer, if not going through history. If it fails to delete a history item, it doesn't clear it, in order to signify that. For the file history, the concern is that if one instance deletes a line, and then another instance tries to delete a (different) line afterwards, its index is going to have changed. I guess the way to solve this would be to reload it and then search for a matching string. I'm thinking add a And, the sqlite did seem to work. Doesn't have a test yet though. |
|
I made it so for the file-backed history, it will delete all occurrences of the referenced entry. This solves the concurrency issue, but changes the behavior of Let me know if that seems good, and I'll make the same change to the sqlite implementation, and then add the tests. |
Yeah I can see the value of your proposed behavior both for purging the history from something secret as well as to suppress a history completion/hint that is no longer relevant. Will ping the rest of the team if they want to advocate for a particular behavior.
Awesome! Thanks for sticking with us and sorry for my radio silence on the PRs |
|
If I remember @fdncred 's feedback correctly the main concern here was the behavior of what happens when you try to delete a history entry when you are not actually on the history but just hit the keybinding. Maybe that is an additional source of confusion and we should just ignore that? |
|
i'm not sure what should be tested with the UX here and how 😕 |
|
Since this is still open I am referencing the above issue as well.... |
Inspired by fish-shell/fish-shell#9454
When a history item is selected, delete it with shift-del (apparently a standard binding, according to the issue linked above).
Will not delete (it just refuses, without showing feedback), when using a file-backed history where the history item being targeted has already been written to the file. It's not trivial to implement for this case, due to the concurrent modification of the text file.
I don't understand the
statements, so those changes in src/engine.rs may not be correct. I see in some places it just does
move_to_line_end.