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

New server configuration key: auto_complete_selector #1408

Merged
merged 14 commits into from
Nov 5, 2020

Conversation

rwols
Copy link
Member

@rwols rwols commented Nov 1, 2020

The auto_complete_selector is used to fine-tune the behavior of the AC popping
up. This selector is added to an entry of the "auto_complete_triggers" of the
view, alongside the trigger characters advertised by the language server.

When both the selector and at least one trigger matches, the AC is shown.

Note that we have to notify all SessionViews of new capability registrations,
so that they can update their trigger chars.

Resolves #933

rwols added 2 commits November 1, 2020 23:19
The auto_complete_selector is used to fine-tune the behavior of the AC popping
up. This selector is added to an entry of the "auto_complete_triggers" of the
view, alongside the trigger characters advertised by the language server.

When both the selector and at least one trigger matches, the AC is shown.

Note that we have to notify all SessionViews of new capability registrations,
so that they can update their trigger chars.
@rchl
Copy link
Member

rchl commented Nov 2, 2020

Trying it out on default preferences.

>>> view.settings().get('auto_complete_triggers')
[{'characters': '<', 'selector': 'text.html, text.xml'}, {'rhs_empty': True, 'selector': 'punctuation.accessor'}, {'characters': '":', 'selector': 'punctuation.separator.mapping.key-value | (string - punctuation.definition.string.end)', 'server': 'LSP-json'}]

Screen Recording 2020-11-02 at 21 09 55

Isn't it supposed to prevent completions popping up within a string?

@rwols
Copy link
Member Author

rwols commented Nov 2, 2020

Isn't it supposed to prevent completions popping up within a string?

That is an escaped character, which I didn't account for in the selector. In order to not trigger AC even for backslash-escaped double-quotes, we should use

punctuation.separator.mapping.key-value | (string - punctuation.definition.string.end - constant.character.escape)

which I will update in the PR for LSP-json.

@rchl
Copy link
Member

rchl commented Nov 2, 2020

What about the colon (also pictured in my gif)?

@rwols
Copy link
Member Author

rwols commented Nov 2, 2020

What about the colon (also pictured in my gif)?

Oh, I didn't notice that one :) that cannot be fixed because the : doesn't receive a separate scope.

@rchl
Copy link
Member

rchl commented Nov 2, 2020

This is not how I've imagined the fix would work. I thought that we can just plain prevent completion from triggering for those two characters (: and ") that LSP-json sets up completions for by for example complementing those with -string scope.

@rwols
Copy link
Member Author

rwols commented Nov 2, 2020

There are a couple options here.

  1. Define a mapping from trigger chars -> selector and match the keys up with the server registrations
  2. Like (1), but per language
  3. Allow disabling trigger characters entirely with a boolean
  4. Same as (3), and allow an auto_complete_selector per language like in (2)

I would personally go for only an extra boolean to disable trigger characters, as the other options get out of hand with respect to configuration options. What do you think?

The interaction between a server's trigger chars, an ST syntax, and ST's default auto_complete_selector seems to work out okay. For instance,

  • pyright uses . and [ as trigger characters. These now won't trigger in undesirable locations.
  • clangd's selector is modified so that it won't trigger for the : of a label.
  • intelephense can probably benefit from a custom selector too as I'd image > is a trigger character for that server
  • I'd imagine solargraph would have . as trigger char, and that is scoped as punctuation.accesor, so using just that selector is sufficient

In almost all of these programming languages a trigger char would not be allowed to be part of a range normally used for identifiers. The exception here is the JSON syntax which may contain trigger chars in strings (where, also somewhat irregular, we do want to allow auto-complete).

@rchl
Copy link
Member

rchl commented Nov 2, 2020

I'll play with it a bit more, maybe it's good enough, but option 1 would allow us to disable colon in scopes like strings so it does sound better. But maybe it's not worth the complexity.

@rchl
Copy link
Member

rchl commented Nov 3, 2020

Could we make it so that auto_complete_selector applies to all trigger characters if it's a string value but can also be an object in which case it will set key=>scope mappings? I feel like we could be limited by just supporting setting scope for all characters and that also makes scopes quite hard to understand and reason about.

Looking at the auto_complete_selector configuration in a LSP package, it's not clear to which trigger characters that scope applies so that makes it hard to tweak and understand the implication of the change.

@rwols
Copy link
Member Author

rwols commented Nov 3, 2020

I went with a boolean to disable the triggers from the server.

  • Some syntaxes are really good
  • Triggers from some servers are bad
  • In most cases you don't even have to touch auto_complete_selector and allow_trigger_characters_from_server as most servers mostly just work fine
  • These two settings have lowest maintenance cost

plugin/core/types.py Outdated Show resolved Hide resolved
plugin/session_view.py Show resolved Hide resolved
sublime-package.json Show resolved Hide resolved
plugin/core/types.py Outdated Show resolved Hide resolved
sublime-package.json Outdated Show resolved Hide resolved
@rwols rwols merged commit 887e559 into st4000-exploration Nov 5, 2020
@rwols rwols deleted the feat/auto-complete-selector branch November 5, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep using trigger characters, or start using selectors?
2 participants