pulsar-edit / pulsar

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

[grammar-finder] Add new Core Package #909

Open confused-Techie opened 9 months ago

confused-Techie commented 9 months ago

Requires #907

This Draft PR adds a new core package grammar-finder to Pulsar.

The decision to add this package is still under review, and until such time as it's agreed on, this PR will remain in draft status.


Grammar-Finder is a package that takes advantage of the new backend feature that allows searching of packages via the file extension that the grammar within the package supports.

This package comes with two ways to discover these packages:

In implementing this package I tried to put as much room as possible for removing interactions the user doesn't want. Such as being able to use the command palette even if AutoFind is disabled to find a suitable package.

Additionally, this package has a config that allows an array of file extensions that can be added , which it will ignore for any and all checks and never prompt for installable packages.

savetheclocktower commented 9 months ago

A couple things:

  1. I share the feeling expressed elsewhere that viewing a simple list of possible grammars to install might not give the user enough context. If I knew nothing about how you'd implemented this, and I just heard the feature described, I'd envision it linking to a search results page on web.pulsar-edit.org.

    There are certainly pros and cons to that approach, and keeping it all within the editor makes it a more streamlined experience. But suppose I open a tex file and am told that Pulsar can install a grammar for it. Great! Which one do I pick? If I had to pick quickly, probably the one with more downloads — but the top two candidates are pretty close to one another in that respect, and I'd probably want to dig into each one and see which has more features, which was more recently updated, et cetera.

    I don't want to make an ultimatum here because I'm certainly not the intended user of this feature. Just mentioning it in case a lot of other people have the same feeling.

  2. Let's make sure that, when tests are added, they cover these edge cases. I'm not saying I know exactly what the package should do in each of these situations, but let's at least brainstorm about it ahead of time:

    • What happens when the file type actually warrants a plain text grammar? Like a txt file, or a README with no file extension? Those two might be easy to guard against, but I'll bet there are plenty more. At the very least, those two file extensions should be listed in ignoreExtList by default.
    • This won't appear when the user creates a new untitled document, right? I gather that the lack of a file type would preclude the notification?
    • What happens when the user already has a package that can understand the given file type, but that package is disabled? Do we try to detect that scenario?
    • What happens if the user hits Esc or clicks the close button? As in “I’m not choosing any of these options explicitly; I’m just busy and would prefer to think about it later”? (I think we can prevent the notification from being dismissed in these manners, but should we?) Do we take that to mean “ask again literally the next time they open this kind of file”? Or do we try to set a flag not to ask them again… when? For 24 hours? Until the window is reloaded?
confused-Techie commented 9 months ago

@savetheclocktower I appreciate the extremely well thought out review and questions here, and I'll go ahead and address what I can think of right away, but otherwise will likely need to think a bit on a few of these.

For your questions on behaviors I want to quickly draw the distinction between the Command Palette invocation of this package and the AutoFind feature. When this package is triggered via the Command Palette it will always show community packages unless it finds none, or that extension is listed as an ignored extension. But for the rest of my answers I'll say them strictly from the perspective of the AutoFind feature.

When text.plain is the best grammar

This package checks strictly for text.plain.null-grammar. Which as far as I can tell is only used when Pulsar can't find the right grammar.

For example when I open test.txt the grammar scope name chosen is text.plain. So this AutoFind wouldn't do anything, as a proper grammar was selected.

So as long as the actually plain text grammar is being used, then this package won't act on it. Of course there may be a situation where plain text is the best and Pulsar also happens to be falling back to a null grammar, but unless we can come up with a common list of extensions where this happens, that may be somewhat impossible to protect against. And somewhat against the purpose of the AutoFind feature, which is intended to only act when Pulsar can't find a grammar.

But to continue, your example of a file named README is a good one. Since Pulsar falls back to text.plain.null-grammar. Which makes does cause this package to act. But in this case, since there's no community packages that offer support of this extension (Which is detected as README), nothing happens. Since I did really want to make this package as unobtrusive as possible for any automatic features. If the package errors out (in a known way) or otherwise can't find any packages, it just does nothing. Not bothering the user at all.

So the second part of the answer to when text is really the best, I'd like to think that is a self solving problem. If text is truly the best, no community packages will exist to support it, so that this package will end up failing to find any suitable community packages and end up doing nothing. Sure that all comes at the cost of a few CPU cycles wasted, but is nice in theory.

Blank New Document

You are right, in this case no extension can be identified, so it's unable to find any packages at all. Simple staying silent and doing nothing.

If a suitable package is currently disabled

As of now we do absolutely nothing to detect this situation. Since the only sources of information for this package come from:

Nothing beyond that is currently considered, but if we thought it important to do nothing in this situation we could certainly look at implementing it. Since I'm all for making this package as unobtrusive as possible.

Hitting Esc

If esc is hit while viewing the list of packages, only that modal goes away. Leaving the notification up until it's closed or esc is hit again. If the notification is closed we currently do nothing to prevent asking again.

But adding this behavior is honestly a really good idea. I'd personally say we cache extension dismissals until the current window is closed. We could just store that as a local variable of the package, and let it get destroyed naturally. Then of course if the user wants a more non-temporal way to dismiss that extension, they could always add it to the extension ignore list.


But for the concerns of making informed decisions about the best package, that seems to be the most obvious thing lacking so far, and will absolutely take some time to figure out how best to address it. Whether that means making that information available in this view, or passing off the actual request to settings-view remains to be seen.

Again thanks for taking the time to look at this one

confused-Techie commented 9 months ago

Alright, added your suggestions as well as made one small change:

I like what you said about trying not to prompt twice, so I've added a very simple implementation for this.

In a local non-cached variable we track every extension AutoFind prompts for. Then we don't prompt if the extension is on that list.

So that in the event the user opens an unrecognized file then closes it and opens again, they won't get two of the same notification.

Or if they open multiple of the same kind of file they won't get a prompt for every single one.

Hopefully this can cull the lowest hanging fruit to this problem

Daeraxa commented 9 months ago

On the topic of "known plaintext files" how does/should it behave with files that have no extension - common for Linux config files.

There is also one format I can imagine could both be annoying and useful at the same time which would be for .log. The chance of there being a correct grammar for your specific .log file is rather low but there are already three packages out there that do supply for that extension - just not anything I personally would have much use for.

confused-Techie commented 9 months ago

@Daeraxa For files that have no extension, it will end up looking at the name of the file for the extension.

This is because of how NodeJS's path.parse() method behaves, I slightly disagree with it.

Take for example the following:

Here I'd personally say that the extension is nvm and the file has no actual name, but NodeJS disagrees. But even more existing packages think the same way here, for example the package that provides syntax highlighting for GitIgnore files declares the extension as gitignore. So if we don't get an extension from path.parse() we fallback to using the name just in case this is happening.

Which will also mean for files that legitimately have no extension like README the name will be used instead. Which if memory serves will likely be the same way it's handled within Pulsar itself, so hopefully that stays consistent.

As for any specific examples like log that could arguably be a default on the ignored extension list, since usually they are rather specific in terms of a usable grammar, but it's hard to say without taking a deeper look at what currently exists.