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

Add runtime language configuration (#1794) #1866

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

zen3ger
Copy link
Contributor

@zen3ger zen3ger commented Mar 25, 2022

  • Add set-language typable command to change the language of current buffer.
  • Add completer for available language options.

Closes #1794

* Add set-language typable command to change the language of current buffer.
* Add completer for available language options.
@@ -537,6 +537,10 @@ impl Loader {
None
}

pub fn language_configs(&self) -> impl Iterator<Item = &Arc<LanguageConfiguration>> {
Copy link
Member

@kirawi kirawi Mar 25, 2022

Choose a reason for hiding this comment

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

I think creating a function like language_config_for_scope would be more homogenous with the code.

Edit: Ah, you're using fuzzy finding. Never mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. 🙂 Though, now that you mentioned it, I could've added a language_config_for_id functions which could've been useful in the actual command implementation...


let doc = doc_mut!(cx.editor);
let loader = Some(cx.editor.syn_loader.clone());
let config = cx.editor.syn_loader.language_configs().find_map(|config| {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than a custom implementation, set_language2 already takes a scope name and looks up the appropriate language. See the discussion on #1787

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a set_language3 that takes the name of a lang? doing :lang scope.rust isn't intuitive IMO.

Copy link
Member

Choose a reason for hiding this comment

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

set_language2(format!("scope.{}", language),...)

Copy link
Member

@kirawi kirawi Mar 25, 2022

Choose a reason for hiding this comment

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

If that's the approach taken, then the fuzzy finding for the languages should be changed to look through the scopes rather than the language names. There are a few langs where the names and scope endings aren't 1-to-1 (e.g. git-diff v. scope.diff)

Copy link
Contributor Author

@zen3ger zen3ger Mar 26, 2022

Choose a reason for hiding this comment

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

I didn't use scope because the prefixes don't seem to follow the source.<lang> format all the time and the language id doesn't always match with suffix either.
Here's a list of those that don't match with language id:

  • protobuf source.proto
  • c-sharp source.csharp
  • javascript source.js
  • typescript source.ts
  • html text.html.basic
  • latex source.tex
  • ocaml-interface source.ocaml.interface
  • racket source.rkt
  • comment scope.comment
  • llvm-mir source.llvm_mir
  • llvm-mir-yaml source.yaml
  • markdown source.md
  • git-commit git.commitmsg
  • git-diff source.diff
  • git-rebase source.gitrebase
  • git-config source.gitconfig
  • solidity source.sol

Edit: I agree that adding set_language3 that takes the language id would be nicer, also adding syntax::Loader::language_config_for_language_id() could be added to not poke into other structs too much.

None
}
});
doc.set_language(config, loader);
Copy link
Member

Choose a reason for hiding this comment

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

Similar to #1787 you also need to call refresh_language_server to change language servers. The function also needs to be fixed: it the server changes we need to send a text document close to the old one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text document close seems to be already handled in Editor::launch_language_server(), more specifically from L464:

        if let Some(language_server) = language_server {
            // only spawn a new lang server if the servers aren't the same
            if Some(language_server.id()) != doc.language_server().map(|server| server.id()) {
                if let Some(language_server) = doc.language_server() {
                    tokio::spawn(language_server.text_document_did_close(doc.identifier()));
                }

                let language_id = doc.language_id().map(ToOwned::to_owned).unwrap_or_default();

                // TODO: this now races with on_init code if the init happens too quickly
                tokio::spawn(language_server.text_document_did_open(
                    doc.url().unwrap(),
                    doc.version(),
                    doc.text(),
                    language_id,
                ));

                doc.set_language_server(Some(language_server));
            }
        }

The only fixing that's needed for Editor::refresh_language_server() is to check if user already set a language or not, otherwise the detection will overwrite the user set value.

     pub fn refresh_language_server(&mut self, doc_id: DocumentId) -> Option<()> {
         let doc = self.documents.get_mut(&doc_id)?;
-        doc.detect_language(self.syn_loader.clone());
+        // try detection only if language is not already set for this document.
+        if doc.language.is_none() {
+            doc.detect_language(self.syn_loader.clone());
+        }
         Self::launch_language_server(&mut self.language_servers, doc)
     }

I'm not sure if this check should be part of Document::detect_language() or not.

* Add language id based config lookup on `syntax::Loader`.
* Add `Document::set_language3` to set programming language based on language
  id.
* Update `Editor::refresh_language_server` to try language detection only if
  language is not already set.
@zen3ger
Copy link
Contributor Author

zen3ger commented Mar 29, 2022

I've played around with using set_language2 and the scope names, but as kirawi pointed out using the whole scope name is not user friendly, and looking at the list I've collected where the scope names don't follow the source.<lang> format, I think using the language_id is the least hackish way of doing this.

In the last commit I've added a set_language3 which does the look-up based on the id, and some other smaller helpers to keep things more aligned with what's already in the code base... If set_language2 is still the way to go, then I'm happy to rework this if someone can "enlighten" me how to go about the oddball scopes. 🙂

@zen3ger zen3ger requested a review from archseer March 29, 2022 21:52
helix-view/src/document.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
* Move document language detection to where the scratch buffer is saved.
* Rename Document::set_language3 to Document::set_language_by_language_id.
@zen3ger zen3ger requested a review from archseer March 31, 2022 17:24
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Just one minor comment

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
@zen3ger zen3ger requested a review from archseer April 4, 2022 17:30
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Nice work!

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.

set a buffer's language at runtime
3 participants