pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.33k stars 140 forks source link

[core] Emit an Event whenever a Grammar is AutoAssigned #907

Open confused-Techie opened 9 months ago

confused-Techie commented 9 months ago

This PR adds a new event that's emitted whenever the GrammarRegistry auto assigns a grammar to a file.

The event will contain the payload of:

{
  grammar
  buffer
}

This change is required for the feature I'm attempting to add to core that would allow auto-finding of packages to support unsupported file extensions.

But this event can be used to emit always, so that if there is some other potential use for this information other community packages can take advantage.

confused-Techie commented 9 months ago

@DeeDeeG Thanks for the approval! I'm happy to wait for the first user of it, which I'm assuming you know is in #909

mauricioszabo commented 6 months ago

Some questions on this:

First, is that I'm unsure when this event is going to be emitted. Suppose I open a Clojure file - will this event be emitted? And what if I open a .ini file, will it be emitted with something like "unknown format"?

Second is - usually, we treat event emitters as implementation details. Care to add an API atom.grammars.onDidAutoAssignGrammar and add documentation to it explaining the first question I had?

confused-Techie commented 6 months ago

@mauricioszabo This event would only ever be triggered when a buffer is auto-assigned a Grammar of "Plain Text".

So the answer to Clojure would be probably not, as Pulsar has built in support. But if you open an .ini file and have nothing that provides syntax highlighting for it, then yes it'd be emitted.

Although documentation is a very good idea to add here, and I'll happily look into adding a method that can be called as well here.

mauricioszabo commented 6 months ago

Oh, I see. Is it possible to be triggered with some different grammar? Do we even auto-assign some grammar if we have a plug-in for it?

confused-Techie commented 6 months ago

@mauricioszabo To my knowledge and testing this would only ever be triggered when being auto assigned to "Plain Text" as a fallback.

Meaning that if "Plain Text" is the right grammar, such as opening a .txt document, this won't be triggered. Then if a grammar is found for your language this would never be triggered. So yeah to my knowledge it only triggers in the event of us not being able to find any appropriate grammar at all for your currently open buffer.

DeeDeeG commented 6 months ago

If anything, a tweak to the PR title might clear up the use-case for this.

EDIT: The actual event name and code comment might be clarified as well.

I think "fallback" has about the right ring to it, or "default." Something to imply this is a placeholder decision when the right choice wasn't as clear or explicit as it sometimes is.

EDIT 2:

// Extended: Remove any language mode override that has been set for the // given {TextBuffer}. This will assign to the buffer the best language // mode available.

Isn't this, like, resetting if the user had manually picked a specific language before we call this? So if it's a .rb file and the user had manually chosen JavaScript as the language... maybe this resets it to Ruby?

And if there was nothing manually set, I would still expect the event to fire regardless of if the language for the TextBuffer was changed by calling this function? Maybe good to include what language was in it before in the event output, IDK, it depends on the use-case really.

As such I think the name and code comment in the PR is fine but would want to make sure this does what you need it to do @confused-Techie ?

(If it does exactly what you describe, then I think that means the code comment and the naming of the function, existing from before this PR, are a bit misleading.)

confused-Techie commented 6 months ago

@DeeDeeG In my testing this function does exactly as I described. But we can wait on this one for further testing to ensure that if we would like, since those tests were done a while ago and things are a bit foggy. But also the purpose of this PR is to interact with #909 so in my mind doesn't really have much urgency to be merged until I finish the feedback on that PR, which I've been lacking on doing so