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

history pager: delete selected history entry with Shift-Delete #9515

Merged
merged 3 commits into from Jul 4, 2023

Conversation

krobelus
Copy link
Member

@krobelus krobelus commented Jan 29, 2023

After accidentally running a command that includes a pasted password, I want
to delete that command from history. Today we need to recall or type (part of)
that command and type "history delete". Let's maybe add a shortcut to do
this from the history pager.

The current shortcut is Shift+Delete. I don't think that's very discoverable,
maybe we should use Delete instead (but only if the cursor is at the end of
the commandline, otherwise delete a char).

Closes #9454

@faho
Copy link
Member

faho commented Feb 1, 2023

This seems like a good idea. My question is if we could make the bind function more general and have it delete the current commandline if the pager isn't open?

I don't think that's very discoverable,
maybe we should use Delete instead

I'm kind of wary of adding something permanent like this to a very simple key press that you might just use to edit. So I think I'd prefer shift-delete for now.

@faho faho added this to the fish 3.7.0 milestone Feb 1, 2023
@mqudsi
Copy link
Contributor

mqudsi commented Feb 15, 2023

shift-delete matches the behavior of native Windows autocomplete inputs w/ history enabled, which was later adopted by Chrom(e|ium) and, I think, Firefox. I'm not comfortable with a plain delete binding, either.

@krobelus
Copy link
Member Author

sounds good, let's stick to Shift+Delete

My question is if we could make the bind function more general and have it delete the current commandline if the pager isn't open?

Yeah we should make it work at least during an up-arrow search (where Shift+Delete shall switch to the next older match).
If no search is active, it should maybe show an interactive prompt for selecting commands to delete, based on the current commandline. That's me spitballing, leaving this case for later.

There's a bug left, deletion does not work for space-prefixed commands. No big deal but would be nice to fix.

@zanchey
Copy link
Member

zanchey commented Jul 4, 2023

Are you happy to merge this for now without that fix?

…binding

The tentative binding for the upcoming "history-pager-delete" is

    bind -k sdc history-pager-delete or backward-delete-char

When Shift+Delete is pressed while the history pager is active,
"history-pager-delete" succeeds. In this case, the "or" needs to kick the
"backward-delete-char" out of the input queue.
After doing so, it continues reading, but interprets the input as
single-char binding. This breaks when the next key emits a multi-char sequence,
like the arrow keys.

Fix this by reading a full sequence, which means we need to run "read_char()"
instead of "read_ch()" (confusing, right?).

I'm still working on writing a test. Somehow this only reproduces in the
history pager where Shift+Delete followed by down arrow emits "[B" (since
we swallowed the leading escape char).  Confusingly, it doesn't do that in
the commandline or the completion search field.
After accidentally running a command that includes a pasted password, I want
to delete command from history. Today we need to recall or type (part of)
that command and type "history delete".  Let's maybe add a shortcut to do
this from the history pager.

The current shortcut is Shift+Delete. I don't think that's very discoverable,
maybe we should use Delete instead (but only if the cursor is at the end of
the commandline, otherwise delete a char).

Closes fish-shell#9454
@krobelus
Copy link
Member Author

krobelus commented Jul 4, 2023

yeah; actually I can't reproduce the problem with space-prefixed ("ephemeral") commands anymore. Looks like we only ever keep one of them in memory, for simplicity of implementation.

@krobelus krobelus merged commit 052823c into fish-shell:master Jul 4, 2023
6 of 7 checks passed
@faho faho modified the milestones: fish next-3.x, fish 3.7.0 Oct 9, 2023
@bentolor
Copy link

bentolor commented Feb 16, 2024

After #2110 has been resolved I was looking into this, but I'm not sure if i get this right:

Shift+Del should delete a whole history entry if the cursor is at EOL, right? If i du an Arrow+Up navigation and press Shift+Del it only deletes the last character, but not the line.

Anything I'm missing @krobelus ?

Update: I just saw [redacted] which mentions something about Shift+Del not working for file-written history entries. But I just stumbled over this issue with a command entered seconds prior. I'm confused. Wrong project.

@faho
Copy link
Member

faho commented Feb 16, 2024

Shift+Del should delete a whole history entry if the cursor is at EOL, right?

No.

It should delete the history entry you are selecting in the history pager, the thing you start with ctrl-r.

We've decided not to do it in the commandline for now.

@krobelus
Copy link
Member Author

I still think we should try to make it work in the commandline but it's not clear to me what the UI should look like.
Echoing the deleted command is a possibility but isn't it a bit un-fishy? Not sure

@bentolor
Copy link

Thanks for clarifying, @faho and @krobelus !

krobelus added a commit that referenced this pull request Apr 13, 2024
Popular operating systems support shift-delete to delete the selected item
in an autocompletion widgets.  We already support this in the history pager.
Let's do the same for up-arrow history search.

Related discussion: #9515
vertesians pushed a commit to vertesians/fish-shell that referenced this pull request Apr 17, 2024
Popular operating systems support shift-delete to delete the selected item
in an autocompletion widgets.  We already support this in the history pager.
Let's do the same for up-arrow history search.

Related discussion: fish-shell#9515
ridiculousfish pushed a commit to ridiculousfish/fish-shell that referenced this pull request Apr 17, 2024
ridiculousfish pushed a commit that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete an entry from Ctrl-R history
5 participants