Avoid searching binary files; tweaks to language mappings; APIs to unregister file extension mapping #7290
Conversation
* Recognize a wider variety of HTML templates embedded in script tags * Remove generic "clike" language declaration that no files map to - seems unneeded, and clutters the dropdown in PR #6409 * Add missing commenting syntax to Scala language * Automatically filter out a variety of common binary file types from Find in Files (it already checks isBinary) - note that this does not affect Mac, where all binary file IO fails automatically. Helps reduce bugs like #6091. Leaves the ProjectManager.isBinary() API in place for now, since it also excludes files from Quick Open and other getAllFiles() clients that don't yet check isBinary via LanguageManager. * Fix incorrect code in Find in Files that only narrowly avoids creating bugs where filtered-out files are incorrectly added to search results.
| "mode": "clike", | ||
| "blockComment": ["/*", "*/"], | ||
| "lineComment": ["//", "#"] | ||
| }, |
There was a problem hiding this comment.
@TomMalbran It looks like this was originally added by you in e0b47b2... do you remember why it seemed needed? None of the languages that use "clike" under the hood seem affected by its removal.
There was a problem hiding this comment.
It is required to add block comment and line comment in PHP and other c like languages, since when you use get language for mode it returns clike and not php when in php mode.
There was a problem hiding this comment.
Hmm, it seems wrong for LanguageManager to return that result. I guess we need a workaround similar to this one: https://github.com/adobe/brackets/blob/master/src/language/LanguageManager.js#L792 (and eventually, we should generalize that so extensions could do similar things for other languages without needing core changes)
(#6873) - with unit tests. - Remove inaccurate docs on Language add extension/name APIs - Fix PHP mixed-mode handling so token-level detection (getLanguageForMode()) works - similar to our handling of "htmlmixed" mode. Allows us to remove "clike" dummy language without breaking PHP Toggle Comment functionality
|
@TomMalbran PR is updated - I went ahead and implemented #6873 so extensions can take over an already-registered file extension, and I tweaked LanguageManager to keep PHP working without needing the dummy "clike" language. |
peterflynn
changed the title
|
@peterflynn Awesome. Would it make sense to automatically unregister an extension/filename when registering a new language? I am thinking that if you want to register a new language with an extension already used by another language it is for a good reason. If not, it might be nice to be able to pass and array of extensions/filenames to the functions. |
|
Hey @TomMalbran We were going to branch for our next release early next week, could you have a look at this by then? Thanks! |
|
@pthiess Sure. Code looks good, besides the 2 questions I added. I will test it later. |
|
@TomMalbran re auto-unregistering: there's something nice about an extension having to explicitly acknowledge that it knows it's overwriting a core setting -- this puts the default emphasis on not breaking core functionality, if a conflict arises by accident or after an extension is written. There's a precedent in how key binding registration works (same as this PR: by default, it fails, but there's an API to explicitly unregister the conflict first). I think I missed where your 2nd question was posted. What was the other thing you wanted me to address? |
|
@peterflynn Ok, makes sense. My other question/suggestion was to change the unregister APIs to recieve an array of strings or just an array. If you want to unregister multiple extensions at once, that change would make it better. |
|
@TomMalbran Ah, got it... I was just paralleling the existing |
|
Sure, we could change both. And is easy to make it take a string or an array. We already provide that with |
…accept an array of multiple items in addition to a single item as supported before. With unit tests.
|
@TomMalbran Changes pushed -- let me know if good to go now (this fixes several reported crashes, so we really want it for Sprint 38). |
peterflynn
changed the title
| * @param {!string} extension A file extension used by this language | ||
| * @return {boolean} Whether adding the file extension was successful or not | ||
| * Adds one or more file extensions to this language. | ||
| * @param {!string|Array.<string>>} extension A file extension (or array thereof) used by this language |
|
@peterflynn Thanks for the API updates, it works great. I was able to easily update my fonts viewer extensions with just a couple of extra lines, which is nice. Just one final thing. Do you think that we should add even more extensions (since is easy to remove them) like Code looks great and test are passing, so feel free to merge it after what you decide to do with the extra extensions. |
webfonts, MS Office files, SQLite db files
|
@TomMalbran Update pushed: fixes the typo nit and adds a bunch more binary file extensions (including all the ones you suggested). I've left audio separate for now so we don't have to worry about any potential backwards-compatibility issues. |
|
Awesome. Merging |
Avoid searching binary files; tweaks to language mappings; APIs to unregister file extension mapping
ProjectManager.isBinary()API in place for now, since it also excludes files from Quick Open and other getAllFiles() clients that don't yet check isBinary via LanguageManager the way Find in Files does. When we get to the real user story we should fully eliminate it, though.