impl auto pairs config - #1624
Conversation
39f20cd to
5715e38
Compare
| /// 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)?; |
There was a problem hiding this comment.
In both cases you already have a &Document, but you pass in an id then look up the document again.
There was a problem hiding this comment.
It seems a bit nicer if this is a method on the document: doc.auto_pairs(&editor.config)
There was a problem hiding this comment.
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
Documentwould need to have a copy of the parsedAutoPairs, which is recreated from the config on every newDocumentor - Still keep the parsed
AutoPairsat theEditorlevel and have anRcmember in theDocument
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
I'm not following, why would we be reconstructing the map? It would be the exact same function, but
docwould be the&selfreceiver, and&editor/&editor.configas 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.
| // 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; | ||
| } |
There was a problem hiding this comment.
Any reason for the changes here? This was intended to be a point where we could inject additional hooks in the future.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f8ea609 to
05a2b5e
Compare
|
@archseer do you have any other thoughts about my comments? |
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.
9a93f83 to
6c335f9
Compare
2 participants
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.tomlthat 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.