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

Snippets insert literal text #6259

Closed
rcorre opened this issue Mar 10, 2023 · 6 comments · Fixed by #6594
Closed

Snippets insert literal text #6259

rcorre opened this issue Mar 10, 2023 · 6 comments · Fixed by #6594

Comments

@rcorre
Copy link
Contributor

rcorre commented Mar 10, 2023

Given:

std::vector<int> i;
i.at|<Tab>

You get

std::vector<int> i;
i.at(size_type n|)

Typing inserts text after n, but the size_type n is left in place.

This started as of d2af31b and is still present on master as of today.

https://asciinema.org/a/UidjhxmHTQmLsUHO2IKPawgwT

@the-mikedavis
Copy link
Member

Do you have the log line for the completion response? I'm pretty sure that the text is the snippet placeholders and you should be able to hit Enter to dismiss/delete them

@pascalkuthe
Copy link
Member

pascalkuthe commented Mar 10, 2023

yeah to expand on this: if you press esc the placeholders stay they are only removed if the completion is confirmed with enter

@rcorre
Copy link
Contributor Author

rcorre commented Mar 11, 2023

Ah, you're correct. enter does remove the placeholder. Is there any way to remove it as you start typing? Or just disable it for now? I usually hit Tab and just start typing right away. Having to press enter as well feels a bit clumsy.

@pascalkuthe
Copy link
Member

pascalkuthe commented Mar 11, 2023

Ah I see there is a bug here in a way: For non-snippet completions you can hit tab and keep typing to:

  • further narrow down the completions (if the selected completion is a prefix of a different one)
  • automatically dismiss the completion popup (and therefore essentially accept the completion)

Case 1 actually can't work for snippets (a snippet can't be a prefix of a different completion). You are essentially using case 2. Case 2 isn't really accepting a completion for real but just keeping the preview we showed and keep editing. It's actually different to accpeting the completion in other ways too. The LS can send additional edits together with a completion that are supposed to be applied when a completion is accepted. This only happens on enter aswell. So that means you are also missing these additional edits. These edits are often used for autoimports or similar features.

In the longterm we want to use virtual text for placeholders (which would be automatically removed). I think we do actually want to activate/show these when a completion is selected with tab. I think we could then also apply the additional edits in that case (it's what vscode does too). This is likely some effort and a bit further down the line altough we have all completions in helix now.

Something slightly offtopic I noticed while looking into this: RA currently abuses the additional edits to make postfix completions work. For example with RA if you place the cursor after data. in the text below

	let foo = data.

and select the box (postfix) completion you get the following result because the additional edit's don't get applied:

	let foo = data.Box::new(data)

Only once you confirm with <ret> do the additional edits get applied (and data. removed):

	let foo = Box::new(data);

Even worse this messes up multicursors, because this additional edit only get applied once, so for secondary cursors data. would not be removed (and it can also really mess up the text/cause crashes because helix doesn't map additional edits trough the completion transaction).

Precisely for this reasons such uses of additional edits are advised against by the LSP standard:

  • Additional text edits should be used to change text unrelated to the
    * current cursor position (for example adding an import statement at the
    * top of the file if the completion item will insert an unqualified type).
    */

I wonder why RA does it does way, hopefully patching this won't be too hard

@rcorre
Copy link
Contributor Author

rcorre commented Mar 11, 2023

For functions with more than 1 argument, enter clears the arguments but leaves a , between them.

CSteamID steam_id = SteamMatchmaking()->GetLobbyMemberByIndex|
<TAB>
CSteamID steam_id = SteamMatchmaking()->GetLobbyMemberByIndex(|CSteamID steamIDLobby, int iMember)
<ENTER>              
CSteamID steam_id = SteamMatchmaking()->GetLobbyMemberByIndex(|,)

helix.log

@pascalkuthe
Copy link
Member

This is on purpose and just part of the snippet. Vscode does the same. Just that vscode allows you to automatically jump between arguments (tabstops) which is currently not implemented

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