Skip to content
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

enable deleting history items with keybinding #567

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

samlich
Copy link
Contributor

@samlich samlich commented Apr 6, 2023

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

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.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +675 to +770
.history_cursor
.delete_item_at_cursor(self.history.as_mut())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/engine.rs Outdated Show resolved Hide resolved
src/engine.rs Outdated Show resolved Hide resolved
src/engine.rs Outdated Show resolved Hide resolved
Comment on lines 200 to 203
if self.len_on_disk <= id {
self.entries.remove(id);
Ok(())
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

src/history/cursor.rs Outdated Show resolved Hide resolved
@samlich
Copy link
Contributor Author

samlich commented Apr 12, 2023

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
Copy link
Contributor Author

samlich commented Apr 29, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

amtoine commented May 4, 2023

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

@stormasm
Copy link
Contributor

stormasm commented Jan 24, 2024

nushell/nushell#11620

nushell/nushell#11629

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants