Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

LSP snippets #288

Closed
wants to merge 81 commits into from
Closed

LSP snippets #288

wants to merge 81 commits into from

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Feb 7, 2019

NOTE: Ready for review


Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Redoes a lot of stuff. Basic idea is to comply with the VS Code spec (based on TextMate spec; also used by Sublime Text) as much as possible, because that's the spec language servers are supposed to use. With this, we'll fully support well formed LSP snippets. The added features are also useful anyway, and following an existing spec means we don't have to define our own (though I believe it's all based off of TextMate anyway).

Putting this up now for feedback and trying it out while it's still being worked on.

Added

  • Support for variables
  • Support for replacement if / else conditionals based on capture group match
  • Support for choices (parses the syntax; actual choice popup deferred to later PR)
  • Support for LSP transformation syntax
  • Service for custom variable resolvers (not in the spec, but I like this idea a lot)
  • Implicit end tab stops when one was not already defined (togglable in settings)
  • Forced undo breaks at each tabstop

Changed

  • Removed implicit g flag from transformation regex. If users want it, they can add it explicitly. And now, if they don't want it, they have a choice.
  • The parser has been rewritten, but still creates a tree compatible with the existing logic when using existing features. A notable change is that \ only escapes the characters $, }, and \ (and a few others depending on context).
  • Specs have been updated to add the g flag where the implicit one was relied upon.
  • Snippet bodies and tab stops are now formed on demand, and are no longer members of the snippet. This is necessary, given the dynamic nature of variables and their transformations. They are now made in the SnippetExpansion, which is a temporary representation for the expanded snippet.
  • Refactoring the existing snippet.getTabStops into something more modular. Currently renamed to toString, but it will make the insertion string and tab stops.
  • Implicit end tab stops (configurable in Settings)
  • End tab stops now break out of the snippet, so you will no longer be able to tab backwards from one (this is in line with their purpose of marking where to put the cursor when the snippet finishes).
  • Redid a lot of editor.transact calls; these appeared to only make the undo behaviour fail in strange ways.

Open Questions

  • [ ] Should \ escape everything? It would make it easier to remember what it does, but subtly break VS Code compatibility (which is slightly different in a few areas anyway). It would also simplify the grammar.
    • Am going with no in this PR. May be possible to make configurable in future (especially if users complain), by passing an argument to the parser.

Alternate Designs

Would have removed \u and co flags entirely, but they appear to be supported by TextMate too and so are worth keeping.

Benefits

99% backwards compatible, and adds much more advanced and dynamic capabilities. With custom variable resolvers, users can do pretty much anything with the provided text editor (and any other relevant params).

Possible Drawbacks

Bodies that contained text like ${1|one,two,three|} will now be interpreted, instead of being a literal. But 1. the fix is to escape the $ like this \\${1|one,two,three|} (as the raw CSON value), and 2. no one* would be doing that unless they wanted a choice placeholder anyway.

*me

Applicable Issues

Closes #287 (this also contains the decaffeination changes)

Closes #285

Also closes #41 I think

Closes atom/apm#213. That conversion uses alternate replacement syntax (?1:=:=) that now has support added here too.

@Aerijo
Copy link
Contributor Author

Aerijo commented Feb 7, 2019

The date and time specification in this comment is interesting. It's possible to do with custom resolvers, but quite clunky (a resolver-variable pair per format). Also possible to do purely with regex transformations?

Could be worth adding direct support for though.

@Aerijo
Copy link
Contributor Author

Aerijo commented Feb 7, 2019

Here's a fun test snippet for all the official variables

"*":
  test:
    prefix: "foo"
    body: """
    - CLIPBOARD: $CLIPBOARD
    - TM_SELECTED_TEXT: $TM_SELECTED_TEXT
    - TM_CURRENT_LINE: $TM_CURRENT_LINE
    - TM_CURRENT_WORD: $TM_CURRENT_WORD
    - TM_LINE_INDEX: $TM_LINE_INDEX
    - TM_LINE_NUMBER: $TM_LINE_NUMBER
    - TM_FILENAME: $TM_FILENAME
    - TM_FILENAME_BASE: $TM_FILENAME_BASE
    - TM_DIRECTORY: $TM_DIRECTORY
    - TM_FILEPATH: $TM_FILEPATH

    - CURRENT_YEAR: $CURRENT_YEAR
    - CURRENT_YEAR_SHORT: $CURRENT_YEAR_SHORT
    - CURRENT_MONTH: $CURRENT_MONTH
    - CURRENT_MONTH_NAME: $CURRENT_MONTH_NAME
    - CURRENT_MONTH_NAME_SHORT: $CURRENT_MONTH_NAME_SHORT
    - CURRENT_DATE: $CURRENT_DATE
    - CURRENT_DAY_NAME: $CURRENT_DAY_NAME
    - CURRENT_DAY_NAME_SHORT: $CURRENT_DAY_NAME_SHORT
    - CURRENT_HOUR: $CURRENT_HOUR
    - CURRENT_MINUTE: $CURRENT_MINUTE
    - CURRENT_SECOND: $CURRENT_SECOND

    - BLOCK_COMMENT_START: $BLOCK_COMMENT_START
    - BLOCK_COMMENT_END: $BLOCK_COMMENT_END
    - LINE_COMMENT: $LINE_COMMENT
    """

TM_DIRECTORY and TM_FILEPATH both are supposed to return undefined when the file has not been saved to disk yet. VS Code sets TM_FILEPATH to the temp file name, but IMO that's a bug.

TM_SELECTED_TEXT refers to the text selected by each individual cursor. I believe it's only really used when selecting arbitrary text and running the command to expand a specific snippet (so the prefix doesn't need to match).

I'm not sure if I'm getting the cursor word right (using language specific word characters) so some input on that would help.

The various date formats are as VS Code does them, at least in English. Localisation is possible, but is currently hardcoded to en-us. I don't know if removing that will let it use the user's localisation, or if it needs to be detected somehow else (if at all).

Finally, the BLOCK_COMMENT_START and friends are more like placeholders right now, while atom/atom#18812 is in progress. It can be marginally improved, to detect the declared block or line delims, but complete support requires the grammar to declare all it's delims.


escapedForwardSlash = pair:'\\/' { return pair; }
// Transform is applied when tabbed off
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment accurate? Looks like the transformation code is largely unchanged, and current behavior is to apply the transform immediately and upon every change to the input text.

Copy link
Contributor Author

@Aerijo Aerijo Feb 8, 2019

Choose a reason for hiding this comment

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

VS Code applies when tabbed off. But I guess leaving the existing behaviour also works.

I think I left that comment to remind me that whatever you're typing while filling it out doesn't matter, as only what you leave it as matters.

@Aerijo
Copy link
Contributor Author

Aerijo commented Feb 8, 2019

Proposed way to handle choices:

Register as an autocomplete provider (ideally as needed, and not constantly). When going to show choices, select the default value (cursor to right) and show all options in an autocomplete popup (with some nice looking unique icons). The provider will set priority to a very high value and disable lower priority suggestions to keep the popup clear.

The user can then

  1. Tab to next stop (should first tab confirm existing selection though? There would be a popup active anyway),
  2. Use arrow keys to pick value, or
  3. Start typing. Deletes placeholder text and autocomplete will present the matches like normal, but the user can also tab off at any point with what they've typed (shortcut values may need some work; I use tab to expand autocomplete itself)

I can't think of a better way right now. It should also work when there is no autocomplete consumer though, but that can be added later. edit: decided this is inextricably tied to a popup, and we can fail gracefully if none is available

I'll update when I get a working demo

@Aerijo
Copy link
Contributor Author

Aerijo commented Feb 8, 2019

For the following snippet

"*":
  t1:
    prefix: "bar"
    body: """
      Hello ${1|one,two,three|} world $0
    """

it currently gives
choice

but it's all smoke and mirrors. Current issues:

  1. Hard coded dependency on the autocomplete-plus package for it's activation command. This is not good. This should support any package that implements the autocomplete service.
  2. Manual delay in activation to "solve" race condition. When expanding the snippet, an autocomplete menu is closing. If a choice is the first option, the opened menu will be closed by the original's close process (yes, that was very confusing at first and hard to track down). IMO this is a bug in autocomplete-plus not supporting rapid popup swapping.
  3. Will not display menu when deleting all placeholder text. This is because autocomplete plus detects the buffer change, but no path leads to keeping the popup open.
  4. Suggestions are not sorted and filtered. Easily fixed with the filterSuggestions property, but this doesn't support non word characters and so should be avoided. Still relatively easy compared to the above.
  5. The provider aggressively overrides everything else, and does not always give back control (e.g., changed tabs when the snippet expansion is still in progress). Could be an easy fix, or could be the root of a lot of edge cases.

In summary, autocomplete-plus is not suited for being driven by a provider. The best solution IMO would be to support that first, and then use it here.

@savetheclocktower
Copy link
Contributor

I’m curious to know whether there’s prior art in VSCode (or elsewhere) on what to do with the “choice” syntax when no autocompleter is present. The bare minimum, I think, would be to treat the first choice as the placeholder and ignore the others. I consider the choice syntax to be an annotation for convenience and that a snippet can degrade gracefully if the IDE doesn’t know a way to present the choices to the user, but maybe there are those who think it should be a stronger contract.

@savetheclocktower
Copy link
Contributor

(should first tab confirm existing selection though? There would be a popup active anyway)

I think this is a concern for autocomplete-plus (or a theoretical alternative autocompleter implementation). There‘s a package setting that controls which keys are treated as confirmations of the highlighted selection — either Tab, Enter, both, or neither. And there‘s a setting for “Tab always, Enter when suggestion explicitly selected,” which distinguishes between a choice that’s active because it’s the default and one that the user opted into through a mouse click or the arrow keys.

@savetheclocktower
Copy link
Contributor

Just now noticed this in your checklist:

Removed implicit g flag from transformation regex. If users want it, they can add it explicitly. And now, if they don't want it, they have a choice.

Somehow this didn't click until one of my own snippets broke today.

I have a snippet for banner comments:

'source':
  'banner':
    'prefix': 'banner'
    'body': '// $1\n// ${1/./=/}'

The intent is to produce a row of =s exactly as long as what gets typed at tabstop 1. This snippet works fine on master, but transforms only the first character when I'm running Atom in dev mode linked to this PR.

But the latter behavior is correct. To achieve what I want, I need a g flag on the transform: // $1\n// ${1/./=/g}. But I overlooked this when I added support for transforms a year and a half ago — I treated all transforms as global.

To confirm my mistake, I tried this snippet in TextMate 2; it needed the g flag. (Tried it in VSCode to see what it'd do, but I can't get that snippet to work in VSCode, with or without the g flag. At the time I added this to Atom, it wasn't yet supported in VSCode, and now I have no idea what state it's in. It's claimed to be supported in their docs, but some people are having trouble with it, and I can't find a single example in the wild that works in VSCode with this particular syntax.)

Anyway, you know all of this, @Aerijo, and I'm just catching up. But to think out loud for a minute, I see a couple problems:

  1. Anyone who tried to use a placeholder transform without /g since Atom 1.25.0 (when transforms were introduced) would've discovered that this package was treating those transforms as though they had the /g flag. I think some such snippets (but not all) could've been rewritten to work in global mode as originally intended, but there's no guarantee that the affected users would've known how to do so. When these changes are released, these users won't necessarily know that the original syntax they tried will actually work properly if they try it again.
  2. Anyone who tried to use a placeholder transform with /g since Atom 1.25.0 would've discovered that they didn't parse properly. To make their snippet work, they'd have to remove the g from the snippet, at which point it would behave the way they wanted. When these changes are released, these users will find that the snippets they managed to get working by removing the g flag actually need it again.

I don't know how large these two groups of affected users are. Probably pretty small, since we didn't get any bugs filed on this behavior. There is a third group of affected users, and it is exactly one person large: me. The person wrote several snippets relying on this faulty transformation behavior but didn't realize it was faulty because he was the one who wrote the feature.

Anyway, (a) this is a break of backward compatibility; (b) it should be, because the pain of breaking backward compatibility is larger than the pain of deviating from the standard behavior; (c) there's not much we can do to cushion the (probably few) users who will be affected by this behavior change, since we won't be able to fix old snippets automatically. I suppose all there is to do is respond promptly with the fix if someone files a bug about the new correct behavior.

(I had regretted that I never added any Flight Manual docs for the transform feature; now I'm regretting that less, since it would've introduced more people to a feature that was subtly broken. My penance, I think, will be to write that documentation in anticipation of the next release.)

@Aerijo
Copy link
Contributor Author

Aerijo commented Jun 23, 2019

@savetheclocktower I can add a setting for that.

@savetheclocktower
Copy link
Contributor

It's up to you, and I'd give my 👍 (for whatever that's worth) whether or not you implemented a setting for it, but my vote would be no. If it were a setting, we'd have to support that wrong behavior indefinitely. A “use legacy transform behavior” setting would help group 2, but not group 1, and the only way it would help group 2 is if we defaulted the setting to true — which would go a long way toward negating your efforts to fix that bug and deliver consistent cross-editor snippet behavior.

@Aerijo
Copy link
Contributor Author

Aerijo commented Jun 23, 2019

*I’ll write some documentation instead

}


// TODO: Use correct locale
Copy link
Contributor

Choose a reason for hiding this comment

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

What is VSCode's behavior here? I can't think of anyplace in Atom that envisions non-English locales, but I'm curious about what would need to happen for this TODO to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really, I have no idea how any locale stuff works. So that was to make sure any final result is intentional, not accidental

savetheclocktower added a commit to savetheclocktower/flight-manual.atom.io that referenced this pull request Jun 25, 2019
Describes functionality that will be present once atom/snippets#288 lands.
@savetheclocktower
Copy link
Contributor

Here's my first draft of some comprehensive docs for the new stuff.

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 1, 2019

I just added support for an alternative if-else format string syntax. Now I'm more familiar with the TextMate docs, it looks like their format string definition is more flexible than ours; it allows recursive variables and such. I also think the if and else rules are supposed to be recursively resolved, and not just treated as plain text.

I don't know if it's worth working on that for this PR, or can be left for a later PR. The LSP doesn't seem to require it, so I'm happy not including it. I added the (?n:x:y) alternate syntax because it was easy, but the features I mentioned are not required by the LSP (which is what I care most about).

@savetheclocktower
Copy link
Contributor

My vote would be to move it into its own PR and keep this one to the LSP stuff. That way it'll get merged sooner and people can start using it.

@savetheclocktower
Copy link
Contributor

@Aerijo, what's the status of this one? If the if-else syntax is moved into its own PR, are you happy with the rest of it as it stands?

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 30, 2019

I'm happy with this PR as is. It's quite big, so I've sort of left gaps between commits to see if anything weird breaks, but I haven't exactly stress tested it either. Anything else now, besides review changes & bug fixes can wait for a new PR.

I'm thinking it would be best to get it in Nightly, so we can see how (if at all) disruptive it will be.

@Aerijo Aerijo changed the title [WIP] LSP snippets LSP snippets Dec 4, 2019
@Aerijo
Copy link
Contributor Author

Aerijo commented Dec 4, 2019

@lkashef Would you be able to consider merging this PR? I've got a lot of time right now to iron out any bugs that get discovered in nightly and beta. There are a couple of TODO comments for the resolver functionality (locales and comment delimiters), but I don't consider them blocking.

@Aerijo Aerijo mentioned this pull request Dec 5, 2019
@aviatesk
Copy link

aviatesk commented Dec 7, 2019

@lee-dohm @lkashef bump. I played with this PR for a while and feel that this fairly works well.

@Aerijo
Copy link
Contributor Author

Aerijo commented Dec 8, 2019

@aviatesk Progress is happening; we're aiming to split it into smaller, more manageable PRs. The decaffeination of the remaining files is the first step.

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 31, 2020

This PR was too big. I tried splitting it off into smaller PRs, but it was still much easier to just write a new package from the ground up. Many of the changes in this PR have already been added to the new package, and things like cleaner undo/redo are on my radar. Once (if) it matures, I may revisit making changes to this package, but for now I'm focusing all my snippet efforts on the new package.

https://github.com/Aerijo/snippets-plus

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make snippets LSP compliant Snippet syntax issue when converting from TextMate bundle Variables
4 participants