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

Add a snippet system #9801

Merged
merged 6 commits into from
Dec 17, 2024
Merged

Add a snippet system #9801

merged 6 commits into from
Dec 17, 2024

Conversation

pascalkuthe
Copy link
Member

This PR finally implements a real snippet system that can scale Fo fully support all features that you expect from snippets. It has been moved to helix core to reflect that these snippets can be sourced from other sources besides LSPs in the future.

I kept the snippet parsers and some of the logic around generating transactions but basically had to fully rewrite the snippet rendering since there were a ton of cases that weren't covered. Our old code around handling snippet indentation was also needlessly complicated, inefficient and incorrect which I fixed here too.

The actual tabstop handling (and its interaction with multi cursor) turned out to be quite tricky too. I created the notion of an active snippet which tracks the currently active tabstops on a document and maps them trough changes as needed. A snippet stays valid as long as all multi cursors are within the entire body of any snippet. I always hated how some vim snippet plugins made snippets way too sticky (so I would jump somewhere else, do something else hit tab and be back at the other end of a file at a renegade tabstop). This approach prevents that while still allowing you to enter normal mode and perform other edits on the snippet without immidietly discarding tabstops.

One you jump to a tabstop the placeholder will be selected, if you are in insertmode hitting any key not bound to a command will delete the placeholder text before inserting the corresponding character. This reuses (and expanded version) of our on_next_key mechanism.

Commits are best reviewed separately otherwise the diff may be hard to follow.

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements A-language-server Area: Language server client E-hard Call for participation: Experience needed to fix: Hard / a lot A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer. S-needs-testing Status: Needs to be tested out in order to discover potential bugs. labels Mar 4, 2024
@pascalkuthe
Copy link
Member Author

I am not yet sure I am happy with the keybindings, there is no way to jump back to a previous tabstop, the interaction with smart tab is hardcoded and normal mode bindings are missing. All of this somewhat conflicts with the jump to parent node end stuff. What are your thoughts @dead10ck ?

@dead10ck
Copy link
Member

dead10ck commented Mar 4, 2024

Hard coding the interaction with smart tab doesn't seem wrong on the face of it. The desired behavior is for tab to do different things in different situations, and this seems like a new situation.

That said, that doesn't solve going back. This kind of seems like a feature that deserves its own special mode, like view mode. In this mode, we could have tab / shift-tab in addition to a set of duplicate bindings for non-kitty protocol terminals. And it would also help avoid having to write a smart-untab command.

@pascalkuthe
Copy link
Member Author

I thought we couldn't bins-tab at all? Or is that only because of the conflict with c-i? In insertmode c-i shouldn't be bound either so if we can create shift-tab bindings then that may work too..

One concern I had about hardworking tab is that I sometimes overshoot my target with tabstops and then endup doing a pretty for jump with the TS motion. So I guess I am not too much of a fan of having multiple things bound to the same key. Maybe that's just me but I guess having some configurability for smart tab could be nice.

@archseer
Copy link
Member

archseer commented Mar 4, 2024

When I started on this I figured snippets could be a separate sticky mode similar to how the debugger functions. An overlay on top of the regular keymap. Tabbing through all the tabstops would exit the mode too

@dead10ck
Copy link
Member

dead10ck commented Mar 4, 2024

Yeah, exactly my thought too. If it's a special mode / overlay, then we don't have to worry about conflicts or interacting with smart tab.

Also @pascalkuthe tab does conflict with Ctrl-i, so that couldn't be a default binding in normal mode. We'll have to figure out a different keymap for non-kitty terminals anyway though. Mod-tab only works in kitty terminals.

@dead10ck
Copy link
Member

dead10ck commented Mar 4, 2024

We could save the special mode for a follow-up PR though, and for now just come up with the bindings for non-kitty terminals. It will just probably not be the tab key.

@pascalkuthe
Copy link
Member Author

yeah I think having it be mode based would be nice. It's entirely possible to bind <S-tab> in insert mode it just bound to insert_tab by default which would be changed by the mode.

I think minor modes need some love in general. That would be a larger change tough so I think its best to just push that to follow-up work and only keep the very basic smart tab binding we have right now

@pascalkuthe pascalkuthe force-pushed the snippet_placeholder branch from 8138b01 to 87fd778 Compare March 8, 2024 15:23
Copy link
Contributor

@matoous matoous Mar 11, 2024

Choose a reason for hiding this comment

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

question: (unrelated to the PR) if we will already have these inside helix, would it eventually make sense to offer commands for transforming strings between different casing? Initially it seemed like a functionality better left for a plugin but with this only a plumbing in between would be what's missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not categorically opposed to it. We need the functionality for snippets since its part of the LSP snippet syntax. It's definitely not a priority tough

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding but Helix already has a way to transform the selected text to uppercase. Maybe lowercase too, I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, there is an open issue about this: #5197. Helix does have lower<->upper conversion, but not snake/pascal/etc.

use std::ops::Index;
use std::sync::Arc;

use anyhow::{anyhow, Result};
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about avoiding anyhow in helix-core? To me anyhow seems useful for stuff like helix-term and commands but for library-like stuff like helix-core it makes it easy to not introduce custom Error types. Since we only have one usage here it would be easy to drop and replace with something thin like a struct + thiserror

Copy link
Member Author

@pascalkuthe pascalkuthe Mar 20, 2024

Choose a reason for hiding this comment

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

Sounds good, makes it easier to figure out what could go wrong and where

@pascalkuthe pascalkuthe force-pushed the snippet_placeholder branch 3 times, most recently from 3b55412 to de9f66e Compare April 1, 2024 23:21
@pascalkuthe
Copy link
Member Author

pascalkuthe commented Apr 1, 2024

rebased on master, fixed @the-mikedavis comments from above plus a bug he mentioned to me where accepting a (snippet) completion by continuing typing a word char would not correctly replace the placeholder

@noor-tg
Copy link

noor-tg commented Apr 6, 2024

Can I use this feature if I build helix from source ?

@zetashift
Copy link
Contributor

Can I use this feature if I build helix from source ?

If you build from the right branch yes!
This is the branch you want to checkout: https://github.com/helix-editor/helix/tree/snippet_placeholder

And if you're a nix user you can try it out in a single command:
nix profile install github:helix-editor/helix#snippet_placeholder

@noor-tg
Copy link

noor-tg commented Apr 7, 2024

Is there any docs about making a snippet ?
Or is it simple like vs code snippets ? In json format ?

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Apr 7, 2024

it's vscode snippets but this doesn't handle loading user snippets, it only handles snippets send by the lsp

@TornaxO7 TornaxO7 mentioned this pull request Apr 7, 2024
@schlich
Copy link

schlich commented Aug 20, 2024

here's an updated flake that loads both the patched version of helix and the snippets LSP using home manager:

(i'm new at nix so feedback welcome, esp. around naming conventions)

~/.config/home-manager/flake.nix

{
  description = "Home Manager configuration of schlich";

  inputs = {
    # Specify the source of Home Manager and Nixpkgs.
    nixpkgs.url = "github:nixos/nixpkgs/nixos-unstable";
    home-manager = {
      url = "github:nix-community/home-manager";
      inputs.nixpkgs.follows = "nixpkgs";
    };
    helix.url = "github:helix-editor/helix?ref=snippet_placeholder";
    scls.url = "github:estin/simple-completion-language-server";
    
  };

  outputs = { nixpkgs, home-manager, helix, scls, ... }:
    let
      # change system for your architecture
      system = "aarch64-darwin";
      pkgs = nixpkgs.legacyPackages.${system};
      helix-dev = helix.packages.${system};
      scls-dev = scls.defaultPackage.${system};
    in {
      homeConfigurations."schlich" = home-manager.lib.homeManagerConfiguration {
        inherit pkgs;

        # Specify your home configuration modules here, for example,
        # the path to your home.nix.
        modules = [ ./home.nix ];

        # Optionally use extraSpecialArgs
        # to pass through arguments to home.nix
        extraSpecialArgs = { helixpkg = helix-dev; sclspkg = scls-dev; };
      };
    };
}

~/.config/home-manager/home.nix

{ pkgs, helixpkg, sclspkg, ... }:

{
  ...
  home.packages = [
    ...
    helixpkg.helix
    sclspkg
  ];
  ...
}

relevant documentation for smart-tab

are y'all enabling the supercede-menu option or something else? Should we be going with the keybindings suggested in that section?

@UltraBlackLinux
Copy link

any updates on this? No new commits in months, no new comments in weeks - What is holding this PR back? A Lack of activity? Are there any issues or uncertainties?

@Philipp-M
Copy link
Contributor

FWIW, I merged current master (after #2608 was merged) into this branch here: https://github.com/Philipp-M/helix/tree/snippet_placeholder
I have tried to isolate the changes that are relevant to solve the merge conflicts here: Philipp-M@18c0d36
The diff is a little bit annoying, as not much happened semantically, but since code was moved and git hasn't recognized this, it's larger than justified.

I'm not sure if I find time for a more in-depth review, but after a skim it looks solid (as usual).
As I think it was tested rather exhaustively by various people (and includes quite a few tests), maybe merge this to increase testing-surface and iterate on it in follow-up PRs (if necessary)?

@the-mikedavis
Copy link
Member

the-mikedavis commented Nov 22, 2024

I want to give this another look in the next few days. I've been using it locally for a while but I've seen some bugs where the tabstop highlight doesn't show up or stick around consistently. (It's possible that I just messed up a merge in my branch though - Pascal was unable to reproduce what I was seeing.) I'll give it a rebase when I give it another look.

There was some discussion above about maybe using a minor mode instead of some of the new code for handling the tab keypress while within a snippet that I'd like to consider again. Chatting with @pascalkuthe it sounds like this may be possible but might need more changes to the keymap module.

I definitely want this merged sooner rather than later though since it's a really big improvement to the completions UX.

@pascalkuthe pascalkuthe force-pushed the snippet_placeholder branch 2 times, most recently from 3db6efd to 79c88b8 Compare December 7, 2024 15:58
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 took another look through this and it looks great! The main part that I was worried about - the extra keymap handling code for tabstops - is very minimal with this PR. I'll add an issue to replace that as we refactor minor modes (as mentioned above).

@the-mikedavis the-mikedavis merged commit 1badd9e into master Dec 17, 2024
6 checks passed
@filipdutescu
Copy link
Contributor

Amazing work done here! Congratulations to everyone involved, I genuinely think this will be an incredibly useful feature and will improve quality of life by a lot! Very excited to try it out!

@dpc
Copy link
Contributor

dpc commented Dec 18, 2024

With a bar cursor-shape, switching to next snippet tabstop displays kind of weirdly...

The first tabstop is OK:

image

but after typing the text and using tab to jump to the next one it shows like this:

image

same for all the following ones.

@dpc
Copy link
Contributor

dpc commented Dec 18, 2024

Also - with inability to fully customize tab handling here, a new keybinding that would close the completion without changing the mode would be great. Then if I'd want to jump to next tabstop while the completion is opened, I would just close it and press tab. Currently pressing escape closes it, but also quits inserting / snippet mode altogether. It seems to me that a new command to just quit completion here: https://docs.helix-editor.com/keymap.html#completion-menu would be easy to implement.

@TornaxO7
Copy link
Contributor

TornaxO7 commented Dec 18, 2024

With a bar cursor-shape, switching to next snippet tabstop displays kind of weirdly...

The first tabstop is OK:

image

but after typing the text and using tab to jump to the next one it shows like this:

image

same for all the following ones.

I'd create a new issue for that since this seems to be a bug and this PR has already been merged.

@gabydd
Copy link
Member

gabydd commented Dec 18, 2024

Bit confused by this, you can just press enter that will accept the completion and close the menu while keeping the placeholders

Also - with inability to fully customize tab handling here, a new keybinding that would close the completion without changing the mode would be great. Then if I'd want to jump to next tabstop while the completion is opened, I would just close it and press tab. Currently pressing escape closes it, but also quits inserting / snippet mode altogether. It seems to me that a new command to just quit completion here: https://docs.helix-editor.com/keymap.html#completion-menu would be easy to implement.

@gabydd
Copy link
Member

gabydd commented Dec 18, 2024

Also there is already an issue #1977 which I think should cover this behaviour

@dpc
Copy link
Contributor

dpc commented Dec 18, 2024

Bit confused by this, you can just press enter that will accept the completion and close the menu while keeping the placeholders

I don't want to accept the completion though. E.g. I typed something, completions showed up, I decided I can't fill this tabstop ATM, so I want to jump to the next one.

Also there is already an issue #1977 which I think should cover this behaviour

It does seem inconsistent that the first tabstop is selected differently than other ones.

Correction: after trying it couple more times, it seems like the exact direction of the tabstop selection is just not reset, so sometimes it's forwards and sometimes backwards, probably just the last direction that was there. Probably a good idea to set it, just so it looks consistently.

@Iorvethe
Copy link
Contributor

Iorvethe commented Dec 18, 2024

For closing the completion menu, it seems that Control-C is what you’re looking for (from the master docs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements A-language-server Area: Language server client C-enhancement Category: Improvements E-hard Call for participation: Experience needed to fix: Hard / a lot S-needs-testing Status: Needs to be tested out in order to discover potential bugs. S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.