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

impl auto pairs config #1624

Merged
merged 13 commits into from
Feb 25, 2022
Merged

Conversation

dead10ck
Copy link
Member

@dead10ck dead10ck commented Feb 7, 2022

Implements configuration for which pairs of tokens get auto completed.

In order to help with this, the logic for when not to auto complete has been generalized from a specific hardcoded list of characters to simply testing if the next/prev char is alphanumeric.

It is possible to configure a global list of pairs as well as at the language level. The language config will take precedence over the global config. This is specifically intended to address and close #992 and #1347; it adds a pair mapping to the Rust section of the default languages.toml that excludes single quotes.

I had some ideas for more sophisticated configs for pairings, such as lexical rules, tree-sitter node lineage, etc, but I think these can be separate PRs. In particular, I'd really like to add a rule for checking a regex pattern on the line before and/or after the cursor to determine if a closer should be paired; e.g. pair <> but only if the < is directly adjacent to an alphabetic char to avoid pairing in "less than" expressions.

@dead10ck dead10ck mentioned this pull request Feb 7, 2022
helix-core/src/auto_pairs.rs Outdated Show resolved Hide resolved
helix-core/src/auto_pairs.rs Outdated Show resolved Hide resolved
helix-core/src/auto_pairs.rs Outdated Show resolved Hide resolved
helix-core/src/auto_pairs.rs Outdated Show resolved Hide resolved
@sudormrfbin sudormrfbin linked an issue Feb 7, 2022 that may be closed by this pull request
@dead10ck dead10ck force-pushed the auto_pairs_config_simple branch 2 times, most recently from 39f20cd to 5715e38 Compare February 7, 2022 16:38
/// otherwise, falls back to the global auto pairs config.
pub fn get_document_auto_pairs(&self, id: DocumentId) -> Option<&AutoPairs> {
let global_config = (&self.auto_pairs).as_ref();
let doc = self.document(id)?;
Copy link
Member

Choose a reason for hiding this comment

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

In both cases you already have a &Document, but you pass in an id then look up the document again.

Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit nicer if this is a method on the document: doc.auto_pairs(&editor.config)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I see what you mean, but I don't think we'd want to pass in the editor config, because then we'd be reconstructing a HashMap on every keystroke. And if we put the parsed AutoPairs object in the Document instead, then we'd have a choice between:

  • Each Document would need to have a copy of the parsed AutoPairs, which is recreated from the config on every new Document or
  • Still keep the parsed AutoPairs at the Editor level and have an Rc member in the Document

Neither would be an awful choice, but it seems like a lot just to avoid an array index operation to look up the same Document.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not following, why would we be reconstructing the map? It would be the exact same function, but doc would be the &self receiver, and &editor/&editor.config as the passed in global_config.

Even without the change to the API, the function signature could be pub fn get_document_auto_pairs(&'_ self, doc: &Document) -> Option<&'_ AutoPairs>

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'm not following, why would we be reconstructing the map? It would be the exact same function, but doc would be the &self receiver, and &editor/&editor.config as the passed in global_config.

Oh, in your earlier comment, you suggested passing in editor.config, but this is just the representation of the config, not the parsed auto pairs. If all we have is the config, then we'd have to get the parsed representation of it somehow, so we'd have to either parse it again or keep a reference to the parsed object, like I mentioned above.

However, I hadn't considered using the whole Editor. I changed it to a method on Document with this approach.

helix-view/src/editor.rs Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
helix-core/src/syntax.rs Outdated Show resolved Hide resolved
helix-core/src/syntax.rs Outdated Show resolved Hide resolved
Comment on lines -4512 to -4063
// run through insert hooks, stopping on the first one that returns Some(t)
for hook in hooks {
if let Some(transaction) = hook(text, selection, c) {
doc.apply(&transaction, view.id);
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the changes here? This was intended to be a point where we could inject additional hooks in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I also just realized it's not possible to opt out of auto pairs anymore: before setting auto_pairs = false would disable this entirely, but now language specific settings are always applied even if the global config is none.

Copy link
Member Author

@dead10ck dead10ck Feb 8, 2022

Choose a reason for hiding this comment

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

Any reason for the changes here? This was intended to be a point where we could inject additional hooks in the future.

Yeah, because now the auto pairs hook and the insert char hook have different interfaces. I say this with all due respect, but I'm actually not so sure it would have been a good starting point for a chain of hooks anyway. I don't think we can expect every hook imaginable to take a Rope, Selection, and char, nor would we necessarily always want to stop after the first hook is applied successfully. I think if we want a hook system (and that would be a good idea, especially for plugins), then we probably need a very general purpose hook system for any kind of text edit, not just a special hook for typing characters in insert mode.

Edit: But with this said, do you have any suggestions for the handling this incompatibility in a more preferable way?

I also just realized it's not possible to opt out of auto pairs anymore: before setting auto_pairs = false would disable this entirely, but now language specific settings are always applied even if the global config is none.

Hmm, yeah, I was actually thinking about this as a feature—having it be off by default, but only on in certain languages—but thinking about it a bit more, if we are going to have specific language defaults, like one without single quotes for Rust, then it becomes very annoying to turn it off if you don't want it at all. I'll change the logic to special case having the global config value = false to take precedence even over language config.

@dead10ck
Copy link
Member Author

@archseer do you have any other thoughts about my comments?

helix-view/src/editor.rs Outdated Show resolved Hide resolved
Implements configuration for which pairs of tokens get auto completed.

In order to help with this, the logic for when *not* to auto complete
has been generalized from a specific hardcoded list of characters to
simply testing if the next/prev char is alphanumeric.

It is possible to configure a global list of pairs as well as at the
language level. The language config will take precedence over the
global config.
@archseer archseer merged commit a494f47 into helix-editor:master Feb 25, 2022
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.

Disable auto-pairing for ' in Rust Enable config for auto-pairs
2 participants