-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Language switcher: connect to prefs & clean up some code #8444
Changes from 2 commits
cc47043
2f542eb
6e6c618
23a3e91
ba1427c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1037,8 +1037,8 @@ define(function (require, exports, module) { | |
* Do the work to initialize a code hinting session. | ||
* | ||
* @param {Session} session - the active hinting session | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Session can't be null here either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, actually it looks entirely unused... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should make that note in the doc and type it as optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not explicitly indicate it's ok to write code which passes null for that arg, unless we've verified it's correct for it to be ignoring the arg :-) And that seems outside the scope of this PR -- but I'll add a TODO to draw attention to it. |
||
* @param {Document} document - the document the editor has changed to | ||
* @param {Document} previousDocument - the document the editor has changed from | ||
* @param {!Document} document - the document the editor has changed to | ||
* @param {?Document} previousDocument - the document the editor has changed from | ||
*/ | ||
function doEditorChange(session, document, previousDocument) { | ||
var file = document.file, | ||
|
@@ -1138,7 +1138,7 @@ define(function (require, exports, module) { | |
* | ||
* @param {Session} session - the active hinting session | ||
* @param {Document} document - the document of the editor that has changed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document and session are non-nullable as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about session since the caller never uses it, but |
||
* @param {Document} previousDocument - the document of the editor is changing from | ||
* @param {?Document} previousDocument - the document of the editor is changing from | ||
*/ | ||
function handleEditorChange(session, document, previousDocument) { | ||
if (addFilesPromise === null) { | ||
|
@@ -1376,7 +1376,7 @@ define(function (require, exports, module) { | |
* | ||
* @param {Session} session - the active hinting session | ||
* @param {Document} document - the document of the editor that has changed | ||
* @param {Document} previousDocument - the document of the editor is changing from | ||
* @param {?Document} previousDocument - the document of the editor is changing from | ||
*/ | ||
function handleEditorChange(session, document, previousDocument) { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -564,7 +564,7 @@ define(function (require, exports, module) { | |
* information, and reject any pending deferred requests. | ||
* | ||
* @param {Editor} editor - editor context to be initialized. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. editor is required here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
* @param {Editor} previousEditor - the previous editor. | ||
* @param {?Editor} previousEditor - the previous editor. | ||
*/ | ||
function initializeSession(editor, previousEditor) { | ||
session = new Session(editor); | ||
|
@@ -579,7 +579,7 @@ define(function (require, exports, module) { | |
* | ||
* @param {Editor} editor - editor context on which to listen for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also be null in the case that all editors are destroyed (files close all, etc...) it will still reset the cache hint context and do nothing more There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will fix |
||
* changes | ||
* @param {Editor} previousEditor - the previous editor | ||
* @param {?Editor} previousEditor - the previous editor | ||
*/ | ||
function installEditorListeners(editor, previousEditor) { | ||
// always clean up cached scope and hint info | ||
|
@@ -624,18 +624,21 @@ define(function (require, exports, module) { | |
* @param {Editor} previous - the previous editor context | ||
*/ | ||
function handleActiveEditorChange(event, current, previous) { | ||
// Uninstall "languageChanged" event listeners on the previous editor's document | ||
if (previous && previous !== current) { | ||
// Uninstall "languageChanged" event listeners on previous editor's document & put them on current editor's doc | ||
if (previous) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
$(previous.document) | ||
.off(HintUtils.eventName("languageChanged")); | ||
} | ||
if (current && current.document !== DocumentManager.getCurrentDocument()) { | ||
if (current) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing the second check here is what allows us to remove the |
||
$(current.document) | ||
.on(HintUtils.eventName("languageChanged"), function () { | ||
// If current doc's language changed, reset our state by treating it as if the user switched to a | ||
// different document altogether | ||
uninstallEditorListeners(current); | ||
installEditorListeners(current); | ||
}); | ||
} | ||
|
||
uninstallEditorListeners(previous); | ||
installEditorListeners(current, previous); | ||
} | ||
|
@@ -803,13 +806,6 @@ define(function (require, exports, module) { | |
.on(HintUtils.eventName("activeEditorChange"), | ||
handleActiveEditorChange); | ||
|
||
$(DocumentManager) | ||
.on("currentDocumentLanguageChanged", function (e) { | ||
var activeEditor = EditorManager.getActiveEditor(); | ||
uninstallEditorListeners(activeEditor); | ||
installEditorListeners(activeEditor); | ||
}); | ||
|
||
$(ProjectManager).on("beforeProjectClose", function () { | ||
ScopeManager.handleProjectClose(); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Language Switcher pr went through and we didn't update the events in Document.js -- specifically the
LanguageChanged
event (which is a past-tense event name and we normally use current-tense event names)Actually it appears that there are NO events documented for document objects for some reason. is there a reason why we aren't documenting them? Are they documented somewhere else? I couldn't find them documented anywhere.
We also should notify lint extension authors that they should update their extensions to listen to the "languageChanged" event since most of them leave cruft behind after changing the language of a javascript file to "text". We might want to change it to "languageChange" to match the rest of the events if no one is using it yet as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
"languageChanged"
event was added much earlier than the 0.42 language switcher PR -- it dates back to Sprint 21 (c00bfb1), so I'd rather not change it as part of this diff.There are some events documented for Document (see docs on the constructor at the top of Document.js), but not this one. I'll add it.