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

helix-term: allow to backspace out-of the command prompt #9828

Conversation

markus-oberhumer-forks
Copy link
Contributor

This makes helix behave like vim and kakoune.

This makes helix behave like vim and kakoune.
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I forgot Kakoune had this behavior. Thanks!

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Mar 7, 2024
@archseer archseer merged commit 0dc67ff into helix-editor:master Mar 9, 2024
6 checks passed
@markus-oberhumer-forks markus-oberhumer-forks deleted the backspace-out-of-command-prompt branch March 9, 2024 14:10
@woojiq
Copy link
Contributor

woojiq commented Mar 20, 2024

Maybe it would be better to introduce a config option for this? Many people are used to "old" behavior. I can't rename symbol now because I always hold backspace to clear prompt (I know ctrl-w exists)😓

@the-mikedavis
Copy link
Member

It also tripped up my muscle memory for a while but for prompts like rename you can use C-w to delete the current line rather than trying to backspace precisely

@markus-oberhumer
Copy link

Well, basically this is a UX design issue, but I tend to agree that the default for the LSP-rename-prompt should be changed.

Please try this quick hack:

diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs
index a3168dc2..55aa58df 100644
--- a/helix-term/src/commands/lsp.rs
+++ b/helix-term/src/commands/lsp.rs
@@ -1036,7 +1036,7 @@ fn create_rename_prompt(
         prefill: String,
         language_server_id: Option<usize>,
     ) -> Box<ui::Prompt> {
-        let prompt = ui::Prompt::new(
+        let mut prompt = ui::Prompt::new(
             "rename-to:".into(),
             None,
             ui::completers::none,
@@ -1070,6 +1070,7 @@ fn create_rename_prompt(
             },
         )
         .with_line(prefill, editor);
+        prompt.backspace_can_abort = false;

         Box::new(prompt)
     }
diff --git a/helix-term/src/ui/prompt.rs b/helix-term/src/ui/prompt.rs
index d46c1313..d681e183 100644
--- a/helix-term/src/ui/prompt.rs
+++ b/helix-term/src/ui/prompt.rs
@@ -36,6 +36,7 @@ pub struct Prompt {
     pub doc_fn: DocFn,
     next_char_handler: Option<PromptCharHandler>,
     language: Option<(&'static str, Arc<ArcSwap<syntax::Loader>>)>,
+    pub backspace_can_abort: bool,
 }

 #[derive(Clone, Copy, PartialEq, Eq)]
@@ -88,6 +89,7 @@ pub fn new(
             doc_fn: Box::new(|_| None),
             next_char_handler: None,
             language: None,
+            backspace_can_abort: true, // default: make helix behave like vim and kakoune
         }
     }

@@ -544,7 +546,7 @@ fn handle_event(&mut self, event: &Event, cx: &mut Context) -> EventResult {
                 (self.callback_fn)(cx, &self.line, PromptEvent::Update);
             }
             ctrl!('h') | key!(Backspace) | shift!(Backspace) => {
-                if self.line.is_empty() {
+                if self.backspace_can_abort && self.line.is_empty() {
                     (self.callback_fn)(cx, &self.line, PromptEvent::Abort);
                     return close_fn;
                 }

@mo8it
Copy link
Contributor

mo8it commented Mar 25, 2024

This drove me crazy in the last days and I went into the source code just to find what caused it 😟

There are already two keybindings for aborting: ESC and CTRL+C. Why should we add this?

Of course, I get the argument of making the life of people migrating from other editors easier. But Vim/kakoune users will not be that surprised if backspace didn't abort. They see that nothing happens and just press ESC. But people hitting one backspace too much will need to repeat the whole operation…

Of course I am biased because I got used to the old behavior, but even just thinking about the UX, the new behavior is very limiting. People would need to use ESC or CTRL+C anyway if they want an "early" abort. For example, if your cursor is in the middle of the prompt and you want to abort, you can't just spam backspace, you have to use the other keybindings (or go to the end of the prompt and then spam backspace which would mean that Helix supported an inefficient way here instead of just letting people know of ESC or CTRL+C).

Please revert this or at least make it an option where the default is not aborting.

@archseer
Copy link
Member

Hmm I also preferred the original behaviour since it seemed less surprising and it's really annoying when the prompt suddently closes when you backspace too far. I wasn't too opinonated though. If there's a clear preference I wouldn't be opposed to changing back

@markus-oberhumer
Copy link

Well, Vim and Kakoune set the precedent at least for the command : and find / prompts, and it obviously bothered me so much that I spent time writing and submitting this patch.

But then this indeed a UX design issue.

@the-mikedavis
Copy link
Member

I'm not strongly opinionated on this either but I agree that the old behavior was less surprising. I think it would be fine to break with Vim and Kakoune here as @mo8it says we already have other ways of aborting. I definitely don't want inconsistency among the prompts though or a config option for a small behavior this.

@markus-oberhumer
Copy link

I definitely don't want inconsistency among the prompts though or a config option for a small behavior this.

Fully agree on this, so I'll let the project leaders decide. I can live with my private fork.

the-mikedavis added a commit that referenced this pull request Mar 25, 2024
…)"

This reverts commit 0dc67ff.

See the post-merge discussion in #9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
archseer pushed a commit that referenced this pull request Mar 26, 2024
…)" (#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in #9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
Desdaemon pushed a commit to Desdaemon/helix that referenced this pull request Mar 26, 2024
shortc pushed a commit to shortc/helix that referenced this pull request Mar 31, 2024
…ix-editor#9828)" (helix-editor#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in helix-editor#9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
…ix-editor#9828)" (helix-editor#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in helix-editor#9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
…ix-editor#9828)" (helix-editor#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in helix-editor#9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
…ix-editor#9828)" (helix-editor#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in helix-editor#9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 20, 2024
…ix-editor#9828)" (helix-editor#10005)

This reverts commit 0dc67ff.

See the post-merge discussion in helix-editor#9828. The old behavior was less
surprising and we have other ways to abort from a prompt, so let's
revert the behavior change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants