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

Disable inline completions in Vim normal mode #22439

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Dec 27, 2024

This is harmful for user experience and at best requires a user setting. This was committed as part of #21739 however that change had no relevant release notes and no relevant settings.

The issue #22343 shows how this can result in user experience regression: deleting a text fragment can reinsert it back, and it's thus unclear if the deletion has even worked. Maybe this can be reenabled in some very restrictive setup, and put behind a setting, but it can't be unconditional. Completions should activate when the user signals intent of entering code - for example, if instead of de to delete a fragment, I press ce to replace it, I would naturally expect inline completions to show up.

Note: The linked PR added more code in vim crate to refresh inline completions in normal mode. I'm keeping that code around in this commit, so that this can be the minimal fix to the linked issue -- with the assumption that maybe there's some way in the future to reenable this in a subset of cases that don't result in confusing / broken UX. If that is not true the code might need further cleanup. Let me know if you'd rather see removal of those changes in this PR as well.

Closes #22343.

Release Notes:

  • Fixes inline completions showing up in Vim normal mode.

This is harmful for user experience and at best requires a user setting.
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Dec 27, 2024
@zeux
Copy link
Contributor Author

zeux commented Dec 27, 2024

For posterity, one other example of how this breaks UX: in this context, I can not move the cursor down (pressing the button does not move it from it's current position). This one is probably solvable but the current state is that the navigation flow is impeded. (this is not a problem in insert mode)

image

@ConradIrwin ConradIrwin added this pull request to the merge queue Jan 6, 2025
@ConradIrwin
Copy link
Member

Thanks!

cc @mrnugget

Merged via the queue into zed-industries:main with commit 76d18f3 Jan 6, 2025
14 checks passed
@mrnugget
Copy link
Member

mrnugget commented Jan 7, 2025

Yeah, that's a bummer, because I disagree that they should never be available in normal mode. They should be better, but not disabled. In Cursor/VSCode it's helpful to get inline completions when in normal mode, because I'm basically never in insert, except when I'm typing.

@zeux
Copy link
Contributor Author

zeux commented Jan 7, 2025

Sure, that's why I just disabled the single line as noted in the PR. I am not sure VSCode shows completions in normal mode - I was not able to get any in a few minutes of trying to force it in a code segment where insert mode readily suggested one; Cursor definitely does but my recollection is that their behavior is fairly different here to begin with as it will suggest line rewrite completions at times (that are not limited to the current cursor position). Can't check this because I stopped using Cursor last year after switching to Zed :)

But regardless I definitely assume there's a better version of this that can be enabled - that's why the change is a minimal change to fix this. I hope if this comes back it has a setting and release notes.

@mrnugget
Copy link
Member

mrnugget commented Jan 7, 2025

I've got a version locally and I want to finish tomorrow. It would reenable it in normal mode but fix the navigation issues that you ran into — do you think that would work for you?

@zeux
Copy link
Contributor Author

zeux commented Jan 7, 2025

Anything that has a setting would work for me :D I only ran into two specific issues that were both problematic - the navigation issue mentioned in this PR, and the suggestion-post-deletion which I filed in the linked issue - but I've been using Zed much less over the holidays when this change got introduced, so maybe there's more.

@duffytilleman
Copy link

Thanks for this fix. With the bug zed had been unusable for me.

I would second having this be a setting. Even if navigation were not broken, normal mode does not imply code editing intent, and having to navigate through many lines of inline completion would be annoying.

As an aside, the original PR (#21739) makes me concerned about the development and quality process. There's over 100 commits (many with message "WIP" or "Checkpoint"), no description, and no discussion.

@mrnugget
Copy link
Member

mrnugget commented Jan 8, 2025

I would second having this be a setting. Even if navigation were not broken, normal mode does not imply code editing intent, and having to navigate through many lines of inline completion would be annoying.

I disagree: if I dd a line with, say, a struct field, I'd love for inline assistant to suggest deleting the field in the constructor too. Example:

screenshot-2025-01-08-09.47.17.mp4

dd, p, df, x, ... — all things that keep me in normal mode but are clearly editing.

Maybe the problem is that Copilot/Supermaven don't offer non-local edits in their implementation here, so it doesn't make sense.

As an aside, the original PR (#21739) makes me concerned about the development and quality process. There's over 100 commits (many with message "WIP" or "Checkpoint"), no description, and no discussion.

There's been a lot of internal discussions and descriptions. What should we have done? Involve all of the community in a feature that we want to try out? I think that would've been too early. We're going into beta for the feature soon, which is when everybody can try it and give feedback on which we can then iterate.

mrnugget added a commit that referenced this pull request Jan 8, 2025
This fixes the issue described in this comment: #22439 (comment)

Essentially, we'd clip in the wrong direction when there were multi-line
inlay hints.
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2025
This fixes the issue described in this comment:
#22439 (comment)

Essentially, we'd clip in the wrong direction when there were multi-line
inlay hints.

It also fixes inline completions for non-Zeta-providers showing up in
normal mode.

Release Notes:

- N/A
@zeux
Copy link
Contributor Author

zeux commented Jan 8, 2025

@mrnugget The problem is not with whatever new internal completion feature you guys are trialing. I don't think you owe the community to gather any feedback before you think the feature is ready.

The problem is that as part of the PR introducing the feature, the existing workflow was broken.

It was broken silently - there was zero description in the PR, zero release notes, zero settings.
It was broken before the holiday season which did not help, because this resulted in a usability regression that could not be fixed quickly.

I discovered this during editing and was just confused - it sort of looked like deletion didn't work. Then I realized that it does work but deletion shows Copilot hints now. I thought I saw something related to completion in the last release notes, so I reread the last two release notes a few times but couldn't find it. Then I had to bisect this to a completely unrelated commit; this felt weird, why would a staff-only feature affect anyone else? Then I had to read the entire diff of that patch, to find the problem.

This is what should not have happened, not the PR with new not-ready-for-community features that only activate internally, that's completely fine and part of running a complex open-source project.

I also worry that if you internally are testing a different completion provider, you just... aren't using the same workflows. Which is how from your perspective normal mode should display inline completions because you are using a provider that does not suggest re-adding text that was just removed, and in general just has a higher accuracy of completions, whereas for the rest of us it is just broken. Copilot works well only when it does not disrupt the editing flow; I could not use Copilot in VS Code for the first ~two years of its existence, because the combination of weaker model, worse editor context, worse heuristics on when to suggest a completion, just led to the editing flow being constantly interrupted by bad suggestions.

@zeux zeux deleted the fix-vim-completions branch January 8, 2025 18:37
@mrnugget
Copy link
Member

mrnugget commented Jan 9, 2025

Got it, sorry for introducing a bug!

@lougreenwood
Copy link

Happy to see this fixed, although it seems that it's still an issue in the assistant panel for me.

Anyone else still have the issue in the assistant panel?

@mrnugget
Copy link
Member

mrnugget commented Jan 9, 2025

Anyone else still have the issue in the assistant panel?

Can you show a screenshot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline completions show up in vim during normal mode
5 participants