enable deleting history items with keybinding

[samlich]

Inspired by https://github.com/fish-shell/fish-shell/issues/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

self.editor.move_to_start(UndoBehavior::HistoryNavigation);
self.editor.move_to_line_end(UndoBehavior::HistoryNavigation);

statements, so those changes in src/engine.rs may not be correct. I see in some places it just does move_to_line_end.

[samlich]

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 delete: Option<usize> field, which is used by sync(). The delete function would then call sync, if the entry is stored on the file. Does that seem reasonable?

And, the sqlite did seem to work. Doesn't have a test yet though.

[samlich]

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 delete(). I will sometimes rename a script, but it will keep recommending the old name, and since I've run the script many times, there will be many entries, so I like this behavior better.

Let me know if that seems good, and I'll make the same change to the sqlite implementation, and then add the tests.

[sholderbach]

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 delete(). I will sometimes rename a script, but it will keep recommending the old name, and since I've run the script many times, there will be many entries, so I like this behavior better.

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.

Let me know if that seems good, and I'll make the same change to the sqlite implementation, and then add the tests.

Awesome! Thanks for sticking with us and sorry for my radio silence on the PRs

[sholderbach]

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?

cc @amtoine @rgwood for UX comments

[amtoine]

i'm not sure what should be tested with the UX here and how :confused:

[stormasm]

https://github.com/nushell/nushell/issues/11620

https://github.com/nushell/nushell/pull/11629

Since this is still open I am referencing the above issue as well....