Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

switch project performance improvements #1197

Merged
merged 5 commits into from
Jul 10, 2012
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 19 additions & 26 deletions src/document/ChangedDocumentTracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,37 +47,24 @@ define(function (require, exports, module) {
this._changedPaths = {};
this._windowFocus = true;
this._addListener = this._addListener.bind(this);
this._removeListener = this._removeListener.bind(this);
this._onChange = this._onChange.bind(this);
this._onWindowFocus = this._onWindowFocus.bind(this);
$(DocumentManager).on("workingSetAdd", function (event, fileEntry) {

$(DocumentManager).on("afterDocumentCreate", function (event, doc) {
// Only track documents in the current project
if (ProjectManager.isWithinProject(fileEntry.fullPath)) {
DocumentManager.getDocumentForPath(fileEntry.fullPath).done(function (doc) {
self._addListener(doc);
});
if (ProjectManager.isWithinProject(doc.file.fullPath)) {
self._addListener(doc);
}
});

$(DocumentManager).on("workingSetRemove", function (event, fileEntry) {
// Only track documents in the current project
if (ProjectManager.isWithinProject(fileEntry.fullPath)) {
DocumentManager.getDocumentForPath(fileEntry.fullPath).done(function (doc) {
$(doc).off("change", self._onChange);
doc.releaseRef();
});

$(DocumentManager).on("beforeDocumentDelete", function (event, doc) {
// Only documents in the current project are tracked
if (ProjectManager.isWithinProject(doc.file.fullPath)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for a document to remain loaded, even after its project has been closed? If so, this check will fail and the listener won't be removed. It should be safe to just unconditionally remove the listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of how that could happen, but I made the change to be safe.

I also fixed a few other JSLint warnings in this file.

self._removeListener(doc);
}
});

// DocumentManager has already initialized the working set
DocumentManager.getWorkingSet().forEach(function (fileEntry) {
// Can't use getOpenDocumentForPath() here since being in the working set
// does not guarantee that the file is opened (e.g. at startup)
DocumentManager.getDocumentForPath(fileEntry.fullPath).done(function (doc) {
self._addListener(doc);
});
});


$(window).focus(this._onWindowFocus);
}

Expand All @@ -87,9 +74,15 @@ define(function (require, exports, module) {
*/
ChangedDocumentTracker.prototype._addListener = function (doc) {
$(doc).on("change", this._onChange);
doc.addRef();
};


/**
* @private
*/
ChangedDocumentTracker.prototype._removeListener = function (doc) {
$(doc).off("change", self._onChange);
};

/**
* @private
* Assumes all files are changed when the window loses and regains focus.
Expand Down
12 changes: 9 additions & 3 deletions src/document/DocumentCommandHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ define(function (require, exports, module) {
})
.fail(function (fileError) {
FileUtils.showFileOpenError(fileError.code, fullPath).done(function () {
// For performance, we do lazy checking of file existence, so it may be in working set
DocumentManager.removeFromWorkingSet(new NativeFileSystem.FileEntry(fullPath));
EditorManager.focusEditor();
result.reject();
});
Expand Down Expand Up @@ -189,9 +191,13 @@ define(function (require, exports, module) {
var i;

if (paths.length > 0) {
for (i = 0; i < paths.length - 1; i++) {
DocumentManager.addToWorkingSet(new NativeFileSystem.FileEntry(paths[i]));
}
// Add all files to the working set without verifying that
// they still exist on disk (for faster opening)
var filesToOpen = [];
paths.forEach(function (file) {
filesToOpen.push(new NativeFileSystem.FileEntry(file));
});
DocumentManager.addListToWorkingSet(filesToOpen);

doOpen(paths[paths.length - 1])
.done(function (doc) {
Expand Down
129 changes: 79 additions & 50 deletions src/document/DocumentManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,12 @@
* - currentDocumentChange -- When the value of getCurrentDocument() changes.
* - workingSetAdd -- When a file is added to the working set (see getWorkingSet()). The 2nd arg
* to the listener is the added FileEntry.
* - workingSetAddList -- When a list of files are added to the working set (e.g. project open, multiple file open).
* The 2nd arg to the listener is the array of added FileEntry objects.
* - workingSetRemove -- When a file is removed from the working set (see getWorkingSet()). The
* 2nd arg to the listener is the removed FileEntry.
* - workingSetRemoveList -- When a list of files is to be removed from the working set (e.g. project close).
* The 2nd arg to the listener is the array of removed FileEntry objects.
*
* These are jQuery events, so to listen for them you do something like this:
* $(DocumentManager).on("eventname", handler);
Expand Down Expand Up @@ -216,13 +220,45 @@ define(function (require, exports, module) {
// Dispatch event
$(exports).triggerHandler("workingSetAdd", file);
}


/**
* Adds the given file list to the end of the working set list.
* Does not change which document is currently open in the editor.
* More efficient than calling addToWorkingSet() (in a loop) for
* a list of files because there's only 1 redraw at the end
* @param {!FileEntryArray} fileList
*/
function addListToWorkingSet(fileList) {
var uniqueFileList = [];

// Process only files not already in working set
fileList.forEach(function (file) {
// If doc is already in working set, don't add it again
if (findInWorkingSet(file.fullPath) === -1) {
uniqueFileList.push(file);

// Add
_workingSet.push(file);

// Add to MRU order: either first or last, depending on whether it's already the current doc or not
if (_currentDocument && _currentDocument.file.fullPath === file.fullPath) {
_workingSetMRUOrder.unshift(file);
} else {
_workingSetMRUOrder.push(file);
}
}
});

// Dispatch event
$(exports).triggerHandler("workingSetAddList", [uniqueFileList]);
}

/**
* Removes the given file from the working set list, if it was in the list. Does not change
* the current editor even if it's for this file.
* @param {!FileEntry} file
*/
function _removeFromWorkingSet(file) {
function removeFromWorkingSet(file) {
// If doc isn't in working set, do nothing
var index = findInWorkingSet(file.fullPath);
if (index === -1) {
Expand All @@ -236,7 +272,21 @@ define(function (require, exports, module) {
// Dispatch event
$(exports).triggerHandler("workingSetRemove", file);
}


/**
* Removes all files from the working set list.
*/
function _removeAllFromWorkingSet() {
var fileList = _workingSet;

// Remove all
_workingSet = [];
_workingSetMRUOrder = [];

// Dispatch event
$(exports).triggerHandler("workingSetRemoveList", [fileList]);
}

/**
* Moves document to the front of the MRU list, IF it's in the working set; no-op otherwise.
* @param {!Document}
Expand Down Expand Up @@ -370,10 +420,10 @@ define(function (require, exports, module) {

// Remove closed doc from working set, if it was in there
// This happens regardless of whether the document being closed was the current one or not
_removeFromWorkingSet(file);
removeFromWorkingSet(file);

// Note: EditorManager will dispose the closed document's now-unneeded editor either in
// response to the editor-swap call above, or the _removeFromWorkingSet() call, depending on
// response to the editor-swap call above, or the removeFromWorkingSet() call, depending on
// circumstances. See notes in EditorManager for more.
}

Expand All @@ -383,9 +433,7 @@ define(function (require, exports, module) {
*/
function closeAll() {
_clearCurrentDocument();

var wsClone = _workingSet.slice(0); // can't delete from the same array we're iterating
wsClone.forEach(_removeFromWorkingSet);
_removeAllFromWorkingSet();
}


Expand Down Expand Up @@ -516,14 +564,16 @@ define(function (require, exports, module) {
if (_openDocuments[this.file.fullPath]) {
throw new Error("Document for this path already in _openDocuments!");
}

_openDocuments[this.file.fullPath] = this;
$(exports).triggerHandler("afterDocumentCreate", this);
}
this._refCount++;
};
/** Remove a ref that was keeping this Document alive */
Document.prototype.releaseRef = function () {
//console.log("---REF--- "+this);

this._refCount--;
if (this._refCount < 0) {
throw new Error("Document ref count has fallen below zero!");
Expand All @@ -533,6 +583,8 @@ define(function (require, exports, module) {
if (!_openDocuments[this.file.fullPath]) {
throw new Error("Document with references was not in _openDocuments!");
}

$(exports).triggerHandler("beforeDocumentDelete", this);
delete _openDocuments[this.file.fullPath];
}
};
Expand Down Expand Up @@ -950,49 +1002,24 @@ define(function (require, exports, module) {
var filesToOpen = [],
activeFile;

// in parallel, check if files exist
// TODO: (issue #298) delay this check until it becomes the current document?
function checkOneFile(value, index) {
var oneFileResult = new $.Deferred();

// check if the file still exists; silently drop from working set if it doesn't
projectRoot.getFile(value.file, {},
function (fileEntry) {
// maintain original sequence
filesToOpen[index] = fileEntry;
// Add all files to the working set without verifying that
// they still exist on disk (for faster project switching)
files.forEach(function (value, index) {
filesToOpen.push(new NativeFileSystem.FileEntry(value.file));
if (value.active) {
activeFile = value.file;
}
});
addListToWorkingSet(filesToOpen);

if (value.active) {
activeFile = fileEntry;
}
oneFileResult.resolve();
},
function (error) {
filesToOpen[index] = null;
oneFileResult.resolve();
});

return oneFileResult.promise();
// Initialize the active editor
if (!activeFile && _workingSet.length > 0) {
activeFile = _workingSet[0];
}

var result = Async.doInParallel(files, checkOneFile, false);

result.done(function () {
// Add all existing files to the working set
filesToOpen.forEach(function (file, index) {
if (file) {
addToWorkingSet(file);
}
});

// Initialize the active editor
if (!activeFile && _workingSet.length > 0) {
activeFile = _workingSet[0];
}

if (activeFile) {
CommandManager.execute(Commands.FILE_OPEN, { fullPath: activeFile.fullPath });
}
});
if (activeFile) {
CommandManager.execute(Commands.FILE_OPEN, { fullPath: activeFile });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add '.fullPath' after active file:

 ... { fullPath: activeFile.fullPath });

The file open command is expecting a path name, not a FileEntry.

Edit: Maybe not...

Here is the problem I'm running into: add a few files to the working set, then select something in the file tree that is not in the working set. If you refresh the app, you will get a runtime error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! The activeFile var is correctly getting set to a full path string on line 1010 when active file is in working set, but was incorrectly getting set to a FileEntry in fall back case on line 1017.

}
}


Expand All @@ -1006,6 +1033,8 @@ define(function (require, exports, module) {
exports.getAllOpenDocuments = getAllOpenDocuments;
exports.setCurrentDocument = setCurrentDocument;
exports.addToWorkingSet = addToWorkingSet;
exports.addListToWorkingSet = addListToWorkingSet;
exports.removeFromWorkingSet = removeFromWorkingSet;
exports.getNextPrevFile = getNextPrevFile;
exports.beginDocumentNavigation = beginDocumentNavigation;
exports.finalizeDocumentNavigation = finalizeDocumentNavigation;
Expand All @@ -1015,7 +1044,7 @@ define(function (require, exports, module) {

// Setup preferences
_prefs = PreferencesManager.getPreferenceStorage(PREFERENCES_CLIENT_ID);
$(exports).bind("currentDocumentChange workingSetAdd workingSetRemove", _savePreferences);
$(exports).bind("currentDocumentChange workingSetAdd workingSetAddList workingSetRemove workingSetRemoveList", _savePreferences);

// Performance measurements
PerfUtils.createPerfMeasurement("DOCUMENT_MANAGER_GET_DOCUMENT_FOR_PATH", "DocumentManager.getDocumentForPath()");
Expand Down
9 changes: 9 additions & 0 deletions src/editor/EditorManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,13 @@ define(function (require, exports, module) {
}
// else, file was listed in working set but never shown in the editor - ignore
}

function _onWorkingSetRemoveList(event, removedFiles) {
removedFiles.forEach(function (removedFile) {
_onWorkingSetRemove(event, removedFile);
});
}

// Note: there are several paths that can lead to an editor getting destroyed
// - file was in working set, but not in current editor; then closed (via working set "X" button)
// --> handled by _onWorkingSetRemove()
Expand Down Expand Up @@ -546,6 +553,8 @@ define(function (require, exports, module) {
// Initialize: register listeners
$(DocumentManager).on("currentDocumentChange", _onCurrentDocumentChange);
$(DocumentManager).on("workingSetRemove", _onWorkingSetRemove);
$(DocumentManager).on("workingSetRemoveList", _onWorkingSetRemoveList);

// Add this as a capture handler so we're guaranteed to run it before the editor does its own
// refresh on resize.
window.addEventListener("resize", _updateEditorSize, true);
Expand Down
35 changes: 32 additions & 3 deletions src/project/WorkingSetView.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,17 @@ define(function (require, exports, module) {
_createNewListItem(file);
_redraw();
}


/**
* @private
*/
function _handleFileListAdded(files) {
files.forEach(function (file) {
_createNewListItem(file);
});
_redraw();
}

/**
* @private
*/
Expand All @@ -291,6 +301,17 @@ define(function (require, exports, module) {
_redraw();
}

function _handleRemoveList(removedFiles) {
removedFiles.forEach(function (file) {
var $listItem = _findListItemFromFile(file);
if ($listItem) {
$listItem.remove();
}
});

_redraw();
}

/**
* @private
* @param {Document} doc
Expand All @@ -313,11 +334,19 @@ define(function (require, exports, module) {
$(DocumentManager).on("workingSetAdd", function (event, addedFile) {
_handleFileAdded(addedFile);
});


$(DocumentManager).on("workingSetAddList", function (event, addedFiles) {
_handleFileListAdded(addedFiles);
});

$(DocumentManager).on("workingSetRemove", function (event, removedFile) {
_handleFileRemoved(removedFile);
});


$(DocumentManager).on("workingSetRemoveList", function (event, removedFiles) {
_handleRemoveList(removedFiles);
});

$(DocumentManager).on("dirtyFlagChange", function (event, doc) {
_handleDirtyFlagChanged(doc);
});
Expand Down