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

Commit fa8c824

Browse files
committed
Fix two bugs in filesystem port of DocumentCommandHandlers:
* 'Close Others' with one unsaved file did nothing (after choosing to save) -- this was a merge error in doSave(). * 'Close Others' with multiple unsaved files doesn't close working set items that have never been opened yet (after choosing to save) -- appears to be a bug in the code we copied from PR #5497 (saveFileList() & _doCloseDocumentList() were not on the same page about the list of files meant that was returned from saveFileList()). Added a note in the PR. Also rename 'savedFiles' vars to avoid confusion over whether they include files that weren't dirty. And improve docs.
1 parent 0cb5182 commit fa8c824

File tree

1 file changed

+28
-22
lines changed

1 file changed

+28
-22
lines changed

src/document/DocumentCommandHandlers.js

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ define(function (require, exports, module) {
510510
/**
511511
* Saves a document to its existing path. Does NOT support untitled documents.
512512
* @param {!Document} docToSave
513-
* @return {$.Promise} a promise that is resolved with the FileEntry of docToSave (to mirror
513+
* @return {$.Promise} a promise that is resolved with the File of docToSave (to mirror
514514
* the API of _doSaveAs()). Rejected in case of IO error (after error dialog dismissed).
515515
*/
516516
function doSave(docToSave) {
@@ -531,7 +531,7 @@ define(function (require, exports, module) {
531531
file.write(docToSave.getText(true), function (err) {
532532
if (!err) {
533533
docToSave.notifySaved();
534-
result.resolve();
534+
result.resolve(file);
535535
} else {
536536
handleError(err, file);
537537
}
@@ -725,16 +725,28 @@ define(function (require, exports, module) {
725725
return $.Deferred().reject().promise();
726726
}
727727

728+
/**
729+
* Saves all unsaved documents. Returns a Promise that will be resolved once ALL the save
730+
* operations have been completed. If ANY save operation fails, an error dialog is immediately
731+
* shown but after dismissing we continue saving the other files; after all files have been
732+
* processed, the Promise is rejected if any ONE save operation failed (the error given is the
733+
* first one encountered). If the user cancels any Save As dialog (for untitled files), the
734+
* Promise is immediately rejected.
735+
*
736+
* @param {!Array.<File>} fileList
737+
* @return {!$.Promise} Resolved with {!Array.<File>}, which may differ from 'fileList'
738+
* if any of the files were Unsaved documents. Or rejected with {?FileSystemError}.
739+
*/
728740
function _saveFileList(fileList) {
729741
// Do in serial because doSave shows error UI for each file, and we don't want to stack
730742
// multiple dialogs on top of each other
731743
var userCanceled = false,
732-
savedFiles = [];
744+
filesAfterSave = [];
733745

734746
return Async.doSequentially(
735747
fileList,
736748
function (file) {
737-
// Abort remaining saves if user canceled any Save dialog
749+
// Abort remaining saves if user canceled any Save As dialog
738750
if (userCanceled) {
739751
return (new $.Deferred()).reject().promise();
740752
}
@@ -744,7 +756,7 @@ define(function (require, exports, module) {
744756
var savePromise = handleFileSave({doc: doc});
745757
savePromise
746758
.done(function (newFile) {
747-
savedFiles.push(newFile);
759+
filesAfterSave.push(newFile);
748760
})
749761
.fail(function (error) {
750762
if (error === USER_CANCELED) {
@@ -754,23 +766,16 @@ define(function (require, exports, module) {
754766
return savePromise;
755767
} else {
756768
// working set entry that was never actually opened - ignore
769+
filesAfterSave.push(file);
757770
return (new $.Deferred()).resolve().promise();
758771
}
759772
},
760-
false
773+
false // if any save fails, continue trying to save other files anyway; then reject at end
761774
).then(function () {
762-
return savedFiles;
775+
return filesAfterSave;
763776
});
764777
}
765778

766-
/**
767-
* Saves all unsaved documents. Returns a Promise that will be resolved once ALL the save
768-
* operations have been completed. If ANY save operation fails, an error dialog is immediately
769-
* shown and the other files wait to save until it is dismissed; after all files have been
770-
* processed, the Promise is rejected if any ONE save operation failed.
771-
*
772-
* @return {$.Promise}
773-
*/
774779
function saveAll() {
775780
return _saveFileList(DocumentManager.getWorkingSet());
776781
}
@@ -959,7 +964,7 @@ define(function (require, exports, module) {
959964
}
960965
return promise;
961966
}
962-
967+
963968
function _doCloseDocumentList(list, promptOnly, clearCurrentDoc) {
964969
var result = new $.Deferred(),
965970
unsavedDocs = [];
@@ -1027,13 +1032,14 @@ define(function (require, exports, module) {
10271032
result.reject();
10281033
} else if (id === Dialogs.DIALOG_BTN_OK) {
10291034
// Save all unsaved files, then if that succeeds, close all
1030-
_saveFileList(list).done(function (savedFiles) {
1031-
result.resolve(savedFiles);
1035+
_saveFileList(list).done(function (listAfterSave) {
1036+
// List of files after save may be different, if any were Untitled
1037+
result.resolve(listAfterSave);
10321038
}).fail(function () {
10331039
result.reject();
10341040
});
10351041
} else {
1036-
// "Don't Save" case--we can just go ahead and close all files.
1042+
// "Don't Save" case--we can just go ahead and close all files.
10371043
result.resolve();
10381044
}
10391045
});
@@ -1042,10 +1048,10 @@ define(function (require, exports, module) {
10421048
// If all the unsaved-changes confirmations pan out above, then go ahead & close all editors
10431049
// NOTE: this still happens before any done() handlers added by our caller, because jQ
10441050
// guarantees that handlers run in the order they are added.
1045-
result.done(function (savedFiles) {
1046-
savedFiles = savedFiles || list;
1051+
result.done(function (listAfterSave) {
1052+
listAfterSave = listAfterSave || list;
10471053
if (!promptOnly) {
1048-
DocumentManager.removeListFromWorkingSet(savedFiles, (clearCurrentDoc || true));
1054+
DocumentManager.removeListFromWorkingSet(listAfterSave, (clearCurrentDoc || true));
10491055
}
10501056
});
10511057

0 commit comments

Comments
 (0)