core-ai-bot
commented
3 years ago Open core-ai-bot opened 3 years ago
core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago 
Points to consider: maybe we want to make this configurable by the user.
core-ai-bot
commented
3 years ago 
Yes, we definitely do -- see https://trello.com/card/101-remember-user-syntax-mode-per-file-extension-2/4f90a6d98f77505d7940ce88/338. I guess this falls into the same bucket as menus and keyboard shortcuts -- things that are programmatically, not declaratively, specified by extensions but that we want users to be able to override in their preferences. We don't have a great answer for that yet in general.
That's also true of the current language API though, where one extension may create a language A bound to file extension .foo but the user prefs may want to override that and bind it to some other language B.
core-ai-bot
commented
3 years ago 
Reviewed. Made this a medium priority considering that we would want this for a 1.0.
core-ai-bot
commented
3 years ago
If extensions promise to only call the API during startup :-) then it might work to just make Language._addFileExtension() public. If we wanted something that could be done at any time then we'd need to wire up some notifications that Documents can listen to, etc.
core-ai-bot
commented
3 years ago
We could print an error if the method is called after APP_READY has been fired.
core-ai-bot
commented
3 years ago
Just for the fun of it, I added a language after a 1s delay and called _triggerLanguageAdded() in _addFileExtension, which results in doc._updateLanguage() being called, which then finds the language for the previously unregistered extension. This suggests that adding a "languageModified" event in addition to "languageAdded" might do the trick.
However, I was a bit shocked to see that doc._updateLanguage() is called for all open documents each time a language is added. DocumentManager is apparently only loaded after the languages have been added, so luckily that only affects languages defined by extensions at the moment, by default only LESS.@peterflynn: I suppose we use that approach because DocumentManager always sticks around whereas Documents come and go and we want to prevent memory leaks?
core-ai-bot
commented
3 years ago
For performance reasons we could buffer all such changes (languages added or changed) and when nothing has changed for a certain amount of time, fire one "updated" event on LanguageManager that should be used by all code that somehow in the past has made a decision based on a language definition or association.
core-ai-bot
commented
3 years ago
I'm still fond of somehow expressing these things more directly. Some brainstorming:
$.track(LanguageManager.getLanguageForExtension, [extension], _onLanguageChanged);
LanguageManager.trackLanguageForExtension(extension, _onLanguageChanged)
LanguageManager.getLanguageForExtension.startTracking(extension, _onLanguageChanged)
LanguageManager.getLanguageForExtension.stopTracking(extension, _onLanguageChanged)
$(LanguageManager.getLanguageForExtension).on(extension, _onLanguageChanged)
$(LanguageManager.getLanguageForExtension).off(extension, _onLanguageChanged)I suppose all of these still have the problem of creating memory leaks if the documents implement these themselves. I suppose we could create callbacks in the DocumentManager instead that have a reference to _openDocuments and the file path, though this would break/need updating on renames. We could instead give every document a unique ID that never changes and use that as the index. We would probably still want to keep a quick way to retrieve documents by their path...
core-ai-bot
commented
3 years ago
Re@DennisKehrig's comment:
I was a bit shocked to see that doc._updateLanguage() is called for all open documents each time a language is added.
I'm not sure how else it would work: any given Document might have a file extension that matches the new language, so we have to check all of them. The check no-ops if it finds that Document.language is unchanged.
Definitely agree it'd be nice to get all the core languages loaded before lots of Documents are created, though. Currently, that seems guaranteed because brackets.js doesn't call ProjectManager.openProject() until after LanguageManager.ready resolves.
If some extension adds a ton of languages all at once then in theory it might be inefficient, but I suspect the hit would be totally unnoticeable. At the most extreme we're talking maybe ~10 languages added back to back times ~40 open Documents... 400 iterations of a for loop that does some simple string processing is probably negligible. V8 is fast :-)
core-ai-bot
commented
3 years ago
Once a document has a language other than "unknown", merely adding a language is not interesting to it anymore, that would never change the language for the file. But we let the document come to the same conclusion over and over again, namely that yes, indeed, it still wants the language it had all along for the file extension it had all along.
Also it's more than simple string processing, it's lots of function calls and hash lookups, too. And sure, V8 is fast, but are we not worried about performance already? Especially startup performance? And while an individual project doesn't contain too many languages, usually, the user might still have a lot of extensions installed that provide languages - after all we're planning on removing support for most languages from the core and put them into extensions. Than the incidental optimization by loading ProjectManager after DocumentManager wouldn't help anymore.
But right now it's not a problem, I agree. And we can fix it, should it become one, without breaking the API.
core-ai-bot
commented
3 years ago 
@DennisKehrig If brackets extensions are allowed to extend existing languages as we were discussing in Issue #3044 then an open document may need to be notified that it's current mode should be updated would it not?
In regards to performance I think it would be interesting to setup some kind of bench test to see what combinations in the number of languages and open docs would cause a noticeable difference in performance. It would be cool to know what the current implementations can handle before performance begins to suffer in that area.
core-ai-bot
commented
3 years ago
@MarkMurphy True, and we already have a languageChanged event that is fired when doc._updateLanguage() comes to a different conclusion than before. We'd just need to make sure that DocumentManager notifies the documents of a potential change regarding languages (by calling _updateLanguage()).
core-ai-bot
commented
3 years ago
@peterflynn I'm not sure about the second part in the description just now. You can totally use any CodeMirror mode you want for a new language. That is independent from the file extension.
core-ai-bot
commented
3 years ago
@peterflynn The performance considerations are indeed negligible since at boot time, most documents are not actually "open" yet, and adding/modifying an extension at a later time is a rare event for which it would be okay to take long.
core-ai-bot
commented
3 years ago
I added a pull request for this, but it all seems super simple, so I'm worried I may have overlooked an important case in which we need to react to languageModified.
core-ai-bot
commented
3 years ago
For one, I didn't update the language name in the status bar if the language changes. That was already an issue for language changes on rename, I suppose.
core-ai-bot
commented
3 years ago
@peterflynn Assigned to you to close it if you're satisfied
core-ai-bot
commented
3 years ago
@DennisKehrig If you havn't already you should also do this for Language.addFileName as well. I believe I made a reference to this same issue in the the LanguageManager when I added the _addFileName method.
core-ai-bot
commented
3 years ago
@MarkMurphy Yes, both methods call this._wasModified();, which causes the documents to update their language.
core-ai-bot
commented
3 years ago
Currently, Language._addFileExtension() is private (and it may not work well if not called early enough). We should make it a public fully supported API.
This enables: