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

working set files are not guaranteed to be openable, so check for failure #1564

Merged
merged 1 commit into from
Sep 6, 2012
Merged

working set files are not guaranteed to be openable, so check for failure #1564

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions src/document/DocumentManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,15 +409,22 @@ define(function (require, exports, module) {

// Switch editor to next document (or blank it out)
if (nextFile) {
CommandManager.execute(Commands.FILE_OPEN, { fullPath: nextFile.fullPath });
CommandManager.execute(Commands.FILE_OPEN, { fullPath: nextFile.fullPath })
.done(function () {
// (Now we're guaranteed that the current document is not the one we're closing)
console.assert(!(_currentDocument && _currentDocument.file.fullPath === file.fullPath));
})
.fail(function () {
// File chosen to be switched to could not be opened, and the original file
// is still in editor. Close it again so code will try to open the next file,
// or empty the editor if there are no other files.
closeFullEditor(file);
Copy link
Member

Choose a reason for hiding this comment

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

What if we change DocumentCommandHandlers.doOpen() to call DocumentManager.notifyFileDeleted() instead of calling removeFromWorkingSet() as it does today? Then this fail() block would be unneeded (since notifyFileDeleted() calls closeFullEditor()).

That would also get us halfway to fixing #1451 in the bargain...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That change doesn't work if I remove the fail() block code, but it does work if I leave the fail() block code in.

I'm not sure if I agree with #1451, so I'll add my comment there.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see -- my suggestion isn't sufficient because closeFullEditor() is relying on FILE_OPEN (of the no-longer-existing file) to switch away from the closed file's editor. FILE_OPEN notifying that the next file was deleted doesn't substitute for that, since notifyFileDeleted() only touches the editor if the deleted file was open in the editor. It has no idea about the pending close operation that wants to get rid of some other editor.

I still believe #1451 is a change we should make, but it indeed doesn't help with anything here. Sorry for the confusion.

});
} else {
_clearCurrentDocument();
}
}

// (Now we're guaranteed that the current document is not the one we're closing)
console.assert(!(_currentDocument && _currentDocument.file.fullPath === file.fullPath));

// 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);
Expand Down