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

LSP Autocompletion does not insert, it overwrites #4380

Closed
David-Else opened this issue Oct 20, 2022 · 5 comments · Fixed by #5728
Closed

LSP Autocompletion does not insert, it overwrites #4380

David-Else opened this issue Oct 20, 2022 · 5 comments · Fixed by #5728
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements

Comments

@David-Else
Copy link
Contributor

David-Else commented Oct 20, 2022

Summary

Helix

helix.webm

Neovim

neovim.webm

Not only does Helix wrongly overwrite width it is also one character short of the end so when immediately typing . it becomes rectangl.e

Reproduction Steps

na

Helix log

na

Platform

Linux

Terminal Emulator

KItty

Helix Version

helix 22.08.1 (418a622)

pub struct Rectangle {
    pub width: usize,
    pub height: usize,
    pub position: Point2d,
}

fn draw_rectangle(
    rectangle: &Rectangle,
    content: &style::StyledContent<String>,
    stdout: &Stdout,
) -> Result<()> {
    for y in 0..width {
        for x in 0..rectangle.height {
            if (y == 0 || y == rectangle.width - 1) || (x == 0 || x == rectangle.height - 1) {
                draw_content(
                    stdout,
                    Point2d {
                        x: rectangle.position.x + y,
                        y: rectangle.position.y + x,
                    },
                    content,
                )?;
            }
        }
    }

    Ok(())
}
@David-Else David-Else added the C-bug Category: This is a bug label Oct 20, 2022
@David-Else David-Else changed the title Autocompletion does not insert, it overwrites LSP Autocompletion does not insert, it overwrites Oct 20, 2022
@the-mikedavis
Copy link
Member

I think this is fixed by #1819

@the-mikedavis the-mikedavis added the A-helix-term Area: Helix term improvements label Oct 20, 2022
@pascalkuthe pascalkuthe added C-enhancement Category: Improvements and removed C-bug Category: This is a bug labels Jan 10, 2023
@pascalkuthe
Copy link
Member

This is actually not a bug at all but rather a missing feature in helix.
LSP allows two of operation: either insert text at the cursor and keep the rest of the word or replace the entire word.

The LSP spec let's the editor decide here and suggests a config option.
Both VSCode and nvim default to inserting but offer an option to replace instead.
See

// TODO: support using "insert" instead of "replace" via user config

@David-Else
Copy link
Contributor Author

@pascalkuthe I see you are looking at the LSP at the moment with #5711 , can you do anything about this issue? In my experience replace behaves more like a bug than a missing feature:

  • Do you know why Helix is using replace rather than insert? What is the advantage?
  • Why not just switch it to act like VSCode and Nvim?

@the-mikedavis What do you think?

@pascalkuthe
Copy link
Member

It's just a missing config option. Both VSCode and nvim can be configured to act the same way as helix and the LSP standard specifically acommedatde both options. It's just a case of helix having a different default.

As I said in #5676 I also prefer insert as the default behavior. Not sure what @archseer thinks about changing the default.

I will look into adding a config option. I am travelling this weekend but I can look I to this next week

@pascalkuthe
Copy link
Member

#5728 adds the option I described (and fixes some other completion related things). I changed the default in that PR and laid out why I think it's a better/more consistent default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants