search

atom / snippets

Atom snippets package
MIT License
206 stars 102 forks source link

LSP snippets #288

Closed Aerijo closed 3 years ago

Aerijo commented 5 years ago

NOTE: Ready for review

Requirements

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

Changed

Open Questions

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 https://github.com/atom/apm/issues/213. That conversion uses alternate replacement syntax (?1:=:=) that now has support added here too.

Aerijo commented 5 years ago

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 commented 5 years ago

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 https://github.com/atom/atom/issues/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.

Aerijo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

(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 commented 5 years ago

More broadly, I agree with your take — it sounds like there should be a companion PR that implements the enhancements you’d need on the autocomplete-plus side, and for now this PR can just implement the fallback behavior for choices and leave the rest for future enhancement.

savetheclocktower commented 5 years ago

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.

So the grammar suggests that a choice can contain arbitrary text; , and } can be escaped when needed.

People get creative with placeholders inside snippets — arbitrarily long strings of HTML and whatnot. I don’t use choices in my snippets, but it wouldn’t surprise me to learn that some people put some weird text inside of a choice block, including spaces and non-word characters. If the grammar permits it, but autocomplete-plus gets flummoxed by it, then that’s a problem. If we punt and say “autocompletion of choices works, but only for snippets where every choice is a single word without any other characters,” then we’re drawing a distinction that doesn’t exist in the grammar.

It sounds like you’re saying we can just do the sorting and filtering on our end rather than give it to autocomplete-plus. In an ideal world, the work you’d do to sort and filter choices could be contributed to autocomplete-plus directly and the filterSuggestions option could get more powerful for everyone. It’s good to have the option of manually “driving” an autocompleter menu, but the more we do that, the more we couple it to the quirks of autocompleter-plus and the greater the chance that our manual “mode” will behave differently from a normal autocompleter menu in ways that are tiny and confusing to the user.

I say “in an ideal world” because I could think of a dozen reasons why you might not want to do this. It might be too much work, or it might be that fine-grained control of the menu is ultimately unavoidable so we might as well embrace it now.

Aerijo commented 5 years ago

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.

AFAIK VS Code treats snippets and autocomplete as the same thing. Without the popup, I can't expand the snippet unless I force it in the command palette. Even then, it will use a popup for the choices.

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

Sounds good. As Atom treats snippets and the autocomplete popup separately, we should just treat the first option as a regular tabstop placeholder if there is no autocomplete available. I've considered having a kind of "predicted text" to the right of the cursor as you type, but I feel that woud lead to confusion, edge cases, and other annoying stuff.

but maybe there are those who think it should be a stronger contract.

Those people can write it themselves :). In all seriousness, I don't think it's our job to restrict what they type. Offering the popup menu is all a choice should do IMO. It's equally possible the user wants common choices displayed, but the field could still take anything.

(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).

Yeah, VS Code makes the first tab always confirm the option. No need to complicate it.

In an ideal world, the work you’d do to sort and filter choices could be contributed to autocomplete-plus directly and the filterSuggestions option could get more powerful for everyone

I think I looked into this once, but it caused some other stuff to go weird. I do have an autocomplete package where I "manually sort", and the note I left there says it's because of prefix issues. I needed to do that because I wanted \ in the prefix.

I agree that fixing autocomplete-plus to support non word characters would be the ideal solution.

Aerijo commented 5 years ago

I'm thinking we could get this rolled out earlier if we hold off on the choice handling. We would still parse choices, but only present the first choice when expanding (like in the gif but without the popup).

While not ideal, it would still be a strict improvement over the current implementation, and allow the other features to be taken advantage of.

Aerijo commented 5 years ago

For the autocomplete service itself, a possible design:

  1. Only ever a single "popup" per TextEditor. Having multiple makes little sense, as the user can only interact with one at a time anyway (and autocomplete-plus seems to be designed around only having one)
  2. Packages register as providers for the popup service
  3. A registered package can request popup control in the TextEditor at any time (overriding and hiding the default if needed), with the expectation that the driving package restrict itself to activating at sensible times (e.g., the choice presentation for a snippet). When another package is already in control, the request should fail (if multiple packages are trying to use it at the same time, they're not doing it right or are incompatible).
  4. The showing / hiding of the popup is controlled solely by the package
  5. The position of the popup can either be package controlled, or fixed to a cursor / whatever autocomplete-plus normally does
  6. The contents are provided by the package, with the normal filter option (priority is irrelevant here)
  7. Return of control is determined by the package. This would be on a per TextEditor basis, so there is no risk of locking the popup when the snippet is not the focus (and would help lessen the burden in general on packages).

This design is really made with the choice popup in mind as the sole user of this service, and is not meant to handle complicated juggling of multiple simultaneous requests. As I said, if that does happen then either (1) a package has control when they don't absolutely need it, or (2) the packages do similar things and are therefore incompatible (at least with this feature).

I've no idea how hard this would be to implement, so I still say we hold off on this feature entirely for the initial LSP snippets PR.

Aerijo commented 5 years ago

current behavior is to apply the transform immediately and upon every change to the input text.

Just noting VS Code allows transformations on the primary tabstop, but TextMate doesn't. It may be more flexible to allow it, but I don't think it's necessary or particularly useful. Worth noting as a future improvement though (we could selectively disable live transform when primary is a transform).

Aerijo commented 5 years ago

Just got to

then I think this will be ready for a proper review.

savetheclocktower commented 5 years ago

old TextMate \u syntax (which appears to be deprecated in TextMate 2)

Huh, didn’t realize. Is there a documentation page for the TM2 behavior that you’re pulling from? I don’t remember having seen the /upcase//downcase syntax outside of the VSCode spec you linked in the PR description.

savetheclocktower commented 5 years ago

Just noting VS Code allows transformations on the primary tabstop, but TextMate doesn't. It may be more flexible to allow it, but I don't think it's necessary or particularly useful. Worth noting as a future improvement though (we could selectively disable live transform when primary is a transform).

Yeah, to allow it would be a major refactor at this point, and it strikes me as such a niche feature that I’d be surprised if anyone asked for it. When I added support for transformations, I made it a hard requirement that every tab stop have at least one non-transforming insertion point so that the cursor had somewhere to go.

savetheclocktower commented 5 years ago
  1. Only ever a single "popup" per TextEditor. Having multiple makes little sense, as the user can only interact with one at a time anyway (and autocomplete-plus seems to be designed around only having one)

This is fun because the snippets package supports multiple cursors. You can place three cursors, invoke a snippet via tab trigger, then fill it out in three places at once.

Now, since identical keyboard input at multiple cursors should result in identical output, I think one could get away with showing one choice popup at a time, since the choices would be identical across all instances of snippet expansion. But we’d need explicit code to enforce one at a time.

I usually have autocomplete-snippets disabled, but I enabled that package to test something out. I created a document, set the syntax to HTML, then created three cursors on three consecutive lines. I typed ar and got a single popup of snippet suggestions that floated above the first cursor:

screen shot 2019-02-10 at 12 26 38 pm

Choosing any option with the keyboard and hitting Return did the right thing:

screen shot 2019-02-10 at 12 27 57 pm

So autocomplete-plus definitely has code in it that accounts for multiple cursors. Man, this is complicated, and you’re right to punt on it for version 1. I bet this will require a thoughtful discussion in the autocomplete-plus repo with people who know it much better than I do.

Aerijo commented 5 years ago

Is there a documentation page for the TM2 behavior that you’re pulling from?

The 1.x docs reference \u directly on the snippets page.

The 2.x docs don't explicitly refer to them in the snippets section, but they are listed in the format string grammar.

So it looks like they're still supported, but are trying to encourage the /upcase versions.

So autocomplete-plus definitely has code in it that accounts for multiple cursors.

Skimming the event handling in autocomplete-plus, it looks like they always go with the last cursor. I'd be surprised if it could show two popups, given the trouble I had with just changing it in quick succession. When confirmed, they then iterate all cursors and apply to the ones that match the prefix.

Aerijo commented 5 years ago

Whelp, undo / redo is a massive rabbit hole. The only upside (?) is that the existing logic is also broken; here's my take

  1. Undo should be forced to break on each tabstop. So even if the user types all fields really quickly, undo would only undo one at a time. Currently either ignores tab stops, or undoes one character at a time

  2. Undo should otherwise act like normal (which is broken in itself; time interval is not a useful undo separator). This appears to be fine for "plain" snippets, but ones with transforms will only undo one character at a time.

  3. When undoing inside a tabstop we undo-ed to, we can either wipe the whole thing or undo as per normal, whichever feels better / works easier.

  4. I've no idea what this wrapper is for and what it solves. Ideally all history would be managed by the default buffer history, and we just provide the checkpoints / transactions.

  5. What is real :thinking: ? I can't seem to get it to respect checkpoints as the forced stops mentioned in (1), and every time I think I know what they're doing it does something unexpected. A common glitch is to have the very first snippet when starting the editor take two undoes to remove, but later ones work fine. I need to sleep.

Again, lots / all of this applies to existing snippets, so I don't consider it a blocker unless it introduces even more buggy behaviour (when I spot a bug, I try it out in safe mode too).

before I forget, here's another existing undo bug

  1. Make snippet with multiple plain tabstops
  2. Expand and type in the first stop
  3. Tab to the second stop
  4. Undo
  5. Cursor has jumped to first stop; press tab
  6. Cursor has jumped to third stop because it still thinks it's on the second Should be fixed in this PR

good night

Aerijo commented 5 years ago

Still does this weird thing where the first snippet will require two undoes before reverting to the prefix text, and the prefix may require two undoes to remove. I think the double expansion -> prefix is related to how undo only removes a character at a time when a snippet has transformations. Still looking into the double prefix -> remove.

Aerijo commented 5 years ago

Mostly happy with the undo behaviour now, at least as far as not breaking anything new.

Some behaviour change with the $0 stop has now been introduced: tabbing to a $0 stop now exits the snippet immediately. Tabbing to the last $n stop when implicit end stops is disabled will still treat text being typed as part of the snippet. I've removed the use of goToEndOfLastTabStop; it seems redundant now that end stops are part of the list, and there can be multiple of them. (this change also broke the existing specs)

Really impressed by Sublime Text's handling of snippet tab stops; they will be remembered in the undo history (though undo seems to be the way to cycle back too).

Working on if / else handling in format strings now. From the VS Code implementation, it looks like these values are plain strings, so it should not be too much effort besides parsing.

savetheclocktower commented 5 years ago

4. I've no idea what this wrapper is for and what it solves. Ideally all history would be managed by the default buffer history, and we just provide the checkpoints / transactions.

The problem with transformations is that we have to observe the text editor and change the contents of the buffer whenever the user types things. Amazingly, the snippets package didn’t have to do this at all until transformations were supported. Mirrored tab stops were supported by placing multiple cursors. Once a snippet was expanded, the package didn’t have to do any work until it was time to move to the next tab stop.

Once it became necessary to observe the editor and do work while the user was typing, I had to introduce that wrapper to distinguish between (a) buffer changes that occurred because the user typed something (or pasted, or whatever) and (b) buffer changes that occurred because the user is navigating backward or forward on the history stack.

Imagine I’m inside a tab stop whose content is transformed later in the snippet. When I start typing, the transformation happens. Both the direct change I introduced through typing and the indirect change made by the transformation are captured in the same checkpoint in the undo history. If I then undo, the snippets package needs to know that it should not try to recompute the transformation; it just needs to allow the buffer to revert to the last checkpoint.

When observing editor changes, you are not informed whether the change has come from an undo/redo or from user input. So the only way I found to tell them apart was to use that wrapper to detect when an undo/redo has been applied so that we can set a flag as a signal to ignore the next buffer change. You probably noticed @nathansobo’s suggestion in that PR: a better long-term fix would be to give the change event the information we need. I looked into it a while back and got lost. My vague recollection: inside the text editor code, the knowledge that an undo/redo has happened is pretty far afield from the code that fires the change event, and it wasn’t immediately obvious to me how to bring them closer together.

Aerijo commented 5 years ago

It looks like the text buffer code can be modified to accept a param object to the emitDidChangeTextEvent method, and make the undo / redo call pass in the option isUndo / isRedo. The tricky part would be properly handling the nested call depth stuff, but it might work to just go with the params passed by whatever triggers the actual emit event (so the shallowest call). But that's a separate issue.

savetheclocktower commented 5 years ago

So let me know when this is feature-complete and ready for testing and I’ll use it for a few days and see if it breaks anything.

I have the commit bit on this repo, but am otherwise not associated with Atom or its sponsor organizations. Hence I would love it if someone else weighed in once we can assert the exact set of changes that will break backward compatibility. So far I think they’re all changes that we can live with (especially given the massive upside of feature parity with VSCode’s snippets) but I wouldn’t mind having that discussion if someone felt otherwise.

Aerijo commented 5 years ago

I consider it to be feature complete; any changes now would just be bug fixes, refactoring, or fine tuning (still need to test the undo behaviour with more demanding and complex snippets & test the resolver service). I'll also try to write a template for resolver provider.

Regarding merge permissions, it's on the list for review. I don't know how likely it is to actually get reviewed, but an initial review from you should help that along.

If worst comes to worst, I'll just publish under the name snippets-plus 🙂 (but this should definitely be part of the official package).

Aerijo commented 5 years ago

Tested the resolver service, found and fixed a typo. The following can now be used as a template to try it out

// init.js
atom.packages.serviceHub.provide("snippetsResolver", "0.0.0", {
  variableResolvers: {
    "AUTHOR": () => { return "Me" },
    "EMAIL": () => { return "example@example.com" },
    "PROJECT_VERSION": ({editor}) => { 
      /* get project version from context */ 
      return "12.3" 
    } 
  },
  transformResolvers: {
    "asciify": ({input}) => {
      return input.split("").filter(c => /[\x00-\x7F]/.test(c)).join("")
    }
  }
})

E.g., try this snippet with ->☃☃<- not ascii in the clipboard

# snippets.cson
'*':
  'details':
    'prefix': 'details'
    'body': '''
      Author: $AUTHOR
      Email:  $EMAIL
      Version: $PROJECT_VERSION
      E.g.: ${CLIPBOARD/.*/${0:/asciify}/g}
    '''

Transforms are given less context than variable resolvers; this is because I think transforms should act more like pure functions, and apply themselves consistently to the same input.

asciify is offered by TextMate. It's also supposed to replace some Unicode characters with Ascii approximations, like ø to o. I think this file contains a full list of things to map (this is a potential future improvement, or note for anyone who wants to make a resolver provider).

There's also potential for all the PascalCase, camelCase, snake_case, etc., and literally whatever else someone can program.

savetheclocktower commented 5 years ago

Still planning to test this out; just haven’t had the time. Feel free to ping me again after a week or so.

calebmeyer commented 5 years ago

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

For ruby this works as I would expect. It's somewhat difficult to trigger a snippet when your cursor is inside a word (with your test snippet, I had to type it at the beginning of the word in question), but it does what I would consider to be right: |hello foo|hello press enter TM_CURRENT_WORD: foohello

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.

This one definitely does not work for ruby. The line comment in ruby is # and the block comment (not frequently used) is =begin and =end. This gives me // and / /. I consider it really minor though, since any snippet scoped to ruby can just use the literals for those.

Aerijo commented 5 years ago

@calebmeyer

It's somewhat difficult to trigger a snippet when your cursor is inside a word

Currently, it will use the last word part of the prefix if typing at the end, which is desired.

There is a bug, in that using the autocomplete-plus popup to insert it will fill in the whole prefix instead of the text you actually typed. E.g., the prefix may be foobar, but autocomplete-plus will allow you to trigger with foo; the desired value of TM_CURRENT_WORD here is foo, but it will insert foobar. I thinks that's related to how autocomplete-plus expands snippets though (by inserting the entire prefix and calling snippets:expand), so isn't a bug with this PR (I'll have to double check).

Aerijo commented 5 years ago

- TODO: Fix shift-tab deindent regression (this PR makes it deindent the line as well as go back a tab stop).

Aerijo commented 5 years ago

@savetheclocktower I'll be rewriting the specs tomorrow. I believe I've fixed all regressions I've come across, and hopefully any other bugs will appear when writing the specs.

savetheclocktower commented 5 years ago

Awesome. Don’t worry — I haven’t forgotten about this PR.

Aerijo commented 5 years ago

@savetheclocktower I'm having some trouple speccing the undo/redo behaviour. When I test manually, the tested cases work as expected, but the programatic tests are undoing the transformations one step ahead of the originals.

I also think I understand the SnippetHistoryProvider proxy better now, and am wondering if the existing wrap is equivalent to this

function wrap (manager, callbacks) {
  return new Proxy(manager, {
    get (target, name) {
      if (name in callbacks) {
        callbacks[name]()
      }
      return target[name]
    }
  })
}
Aerijo commented 5 years ago

Discovered an interesting behaviour when speccing.

'break':
    prefix: 'b1'
    body: 'hello $1 foo $2'

try that with two at once like so

b1|
b1|

When the final tabstop is reached, pressing tab again will destroy the extra cursor and leave the original (whichever you placed first). I consider this to be a bug; I would expect the editor to insert a tab at each cursor instead. When the end is $0 instead of $2 it does work as expected, but I believe this is more coincidental than by design.

I came across this bug because part of this PR is refining the $0 behaviour, so it popped up when multiple are expanded at once, and the first was destroying the end marks of the others. So I ended up with the following when both should have arrived at $0.

hello  foo | 
hello | foo

I don't know how much work this will take to get right. Hopefully not much.

Aerijo commented 5 years ago

That took longer than expected. I've kept the spirit of the tests, but made a few changes. Most obvious is using an empty editor for all tests that don't need surrounding context. This means we don't load in sample.js each time, which almost halves the test time. I also made the test snippets valid in all scopes so it doesn't need to be a JS file (just realised this means we don't need to load language-javascript, cutting the time even further). E.g., my times went from 8.5 seconds on average to 3.5

What's left is to solve the bug noted above, and get the undo/redo specs working. Then I'll add a new specs file specifically for how various inputs are transformed into snippets, and then it might actually be ready.

Aerijo commented 5 years ago

OK, there can only be so many more regressions left to find. I believe most behaviour is now specced, and I've played with a snippet with transforms and such for a while now without (new) errors.

I also tried to separate the 'destruction' behaviour of individual expansions and the set of active ones. Previously, an individual expansion could destroy the other expansions, meaning when one got to the end tabstop with multiple active it would invalidate the others and prevent them also going to the end. Now when moving tabstops, it can destroy itself and signal to the expansion manager that everything should be destroyed after it's done processing them.

Also added the disableTabDedentInSnippet config option (I don't know how, but I just seem to have a way with names...). When going to previous tabstop on an indented snippet, if it was already on the first stop it will dedent the line. Setting this option will prevent that.

savetheclocktower commented 5 years ago

Finally playing around with this PR.

Discovered an interesting behaviour when speccing.

I have news to report here. It makes me uneasy.

I tried to reproduce this behavior and instead found a different behavior: when the final tab stop is reached — moving from $2 to the implicit $0 — whatever I typed for 2 gets selected. I got this result no matter how many cursors I had on the screen. After a few minutes, I realized what was happening (I think) — the snippet is getting rewritten to hello $1 foo $2$0, meaning that two markers were created ($2 and $0) that had the exact same range and settings. So typing into $2 caused $0's marker to grow.

In this particular case, where $0 is adjacent to a real tab stop, I think this might be a regression because of refactoring you've done that has the effect of treating $0 less like a weird special tab stop and more like an actual tab stop, but that's just a guess.

Distressingly, I realized that I never merged #281 — it's designed to prevent situations like the one I described above. I used VSCode's implementation to solve it — the idea is that the only markers that are allowed to grow in response to input are markers associated with the active tab stop (or any tab stop that references the active tab stop). So $2’s marker would have the setting exclusive: false when it's active, but $0 next to it would have exclusive: true, so it would get pushed rightward rather than expanding to accept the adjacent input.

My intent (as an outsider committer) was to leave #281 open for a week or so to get feedback, then land it if there was none. Then I got distracted by a leaf or a pigeon or something. Of course, this PR has gone and decaffeinated the source files, so it's not exactly something that can be fixed with a cherry-pick.

I ended up doing a half-assed port of my code to your PR to see if it would fix the problem, and it more or less does. The only problem now is that hitting Tab after typing into $2 does nothing — but that's arguably the right behavior. (I think that the implicitEndTabstop setting should only add a tab stop at the end when the snippet doesn't already end with a tab stop, but that's a separate issue.) I'm going to clean up this working copy and put it on a branch somewhere so you can benefit from that work.

But, of course, that only fixes the problem that I experienced. I never was able to reproduce the bug as you describe it. Is it no longer valid? Do I have to disable implicitEndTabstop in order to reproduce it?

I'll continue reviewing this when I get some time — this speed bump took up about 90 minutes. Serves me right for forgetting to merge my own PRs.

Aerijo commented 5 years ago

I tried to reproduce this behavior and instead found a different behavior: when the final tab stop is reached — moving from $2 to the implicit $0 — whatever I typed for 2 gets selected

Can confirm. Wasn't intended. I believe I fixed the original behaviour I described though, which is why it couldn't be reproduced.

the snippet is getting rewritten to hello $1 foo $2$0

Yeah. It should be possible to omit the implicit end if the snippet body ends with a tabstop.

savetheclocktower commented 5 years ago

Yeah. It should be possible to omit the implicit end if the snippet body ends with a tabstop.

If so, then incorporating #281 would fix this issue altogether. My plan is to clean up the changes I made, include some comments in the code whenever I made some judgment calls that may or may not be correct, and then push them to a branch where you can do a git cherry-pick --no-commit and then review those commented places to decide if I did the right thing. That'd probably take less time for you than implementing my changes from scratch using my PR as reference.

savetheclocktower commented 5 years ago

OK, here's the commit. Ported over the two new tests as well and they pass fine. Feel free to add another test for the b1 scenario if you like.

Turns out that I didn't leave any comments for your perusal; the specs were my guide in the two places where I was unsure if I was doing the right thing. Please do scan the changes and let me know if I haven't explained them properly in the code, but once you're satisfied, cherry-pick onto this PR at your leisure.

Aerijo commented 5 years ago

Oh, thought that was weird. They weren't removed tests, they were just added after this PR was started.

savetheclocktower commented 5 years ago

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 commented 5 years ago

@savetheclocktower I can add a setting for that.

savetheclocktower commented 5 years ago

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 commented 5 years ago

*I’ll write some documentation instead

savetheclocktower commented 5 years ago

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

Aerijo commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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.