New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #712

Closed
wants to merge 3 commits into
base: master
from
Jump to file or symbol
Failed to load files and symbols.
+160 鈭19
Diff settings

Always

Just for now

@@ -21,6 +21,7 @@ DIRECTORY_NAME=Directory Name
# File Open/Save Error strings
FILE_EXISTS_HEADER=The file already exists.
# The {0} here will be replaced by an actual error message
OPEN_DIALOG_ERROR=An error occurred when showing the open file dialog. (error {0})
# {0} will be replaced with a filename and {1} will be replaced by an error message
@@ -94,6 +95,8 @@ SAVE_AND_OVERWRITE=Overwrite
DELETE=Delete
BUTTON_YES=Yes
BUTTON_NO=No
USE_IMPORTED=Use Imported
KEEP_EXISTING=Keep Existing
# Quick Edit
@@ -136,6 +139,7 @@ DND_SUCCESS_UNTAR_TITLE=Untar Completed Successfully
DND_SUCCESS_UNZIP=Successfully unzipped <b>{0}</b>.
# {0} will be replaced by a tar filename
DND_SUCCESS_UNTAR=Successfully untarred <b>{0}</b>.
DND_FILE_REPLACE=A file named \"{0}\" already exists in this location. Do you want to use the imported file or keep the existing?

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Have all these strings been added to Thimble's repo yet?

@humphd

humphd May 31, 2017

Member

Have all these strings been added to Thimble's repo yet?

# Image Viewer
IMAGE_DIMENSIONS={0} (width) &times; {1} (height) pixels
@@ -16,6 +16,15 @@ define(function (require, exports, module) {
var Buffer = Filer.Buffer;
var Path = Filer.Path;
var fs = Filer.fs();
var Dialogs = require("widgets/Dialogs");
var DefaultDialogs = require("widgets/DefaultDialogs");
var Strings = require("strings");
var StringUtils = require("utils/StringUtils");
// These are const variables

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Get rid of this comment.

@humphd

humphd May 31, 2017

Member

Get rid of this comment.

var CANCEL_OPERATION = -1,
OVERWRITE_OPERATION = 1,
KEEP_EXISTING_OPERATION = 2;
// Mac and Windows clutter zip files with extra files/folders we don't need
function skipFile(filename) {
@@ -57,6 +66,49 @@ define(function (require, exports, module) {
});
}
function showOverwriteWarning(path, callback) {
var filepath = stripRoot(path);
Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_INFO,
Strings.FILE_EXISTS_HEADER,
StringUtils.format(Strings.DND_FILE_REPLACE, filepath),
[
{
className : Dialogs.DIALOG_BTN_CLASS_NORMAL,
id : Dialogs.DIALOG_BTN_CANCEL,
text : Strings.CANCEL
},
{
className : Dialogs.DIALOG_BTN_CLASS_NORMAL,
id : Dialogs.DIALOG_BTN_IMPORT,
text : Strings.USE_IMPORTED
},
{
className : Dialogs.DIALOG_BTN_CLASS_PRIMARY,
id : Dialogs.DIALOG_BTN_OK,
text : Strings.KEEP_EXISTING
}
]
).getPromise().then(function (id) {
var result;
if (id === Dialogs.DIALOG_BTN_IMPORT) {
result = OVERWRITE_OPERATION;
} else if (id === Dialogs.DIALOG_BTN_OK) {
result = KEEP_EXISTING_OPERATION;
} else if (id === Dialogs.DIALOG_BTN_CANCEL) {
result = CANCEL_OPERATION;
}
callback(null, result);
}, callback);
}
function stripRoot(path) {
var root = StartupState.project("root");
var rootRegex = new RegExp("^" + root + "\/?");
return path.replace(rootRegex, "");
}
// zipfile can be a path (string) to a zipfile, or raw binary data.
function unzip(zipfile, options, callback) {
if(typeof options === 'function') {
@@ -100,21 +152,30 @@ define(function (require, exports, module) {
} else {
// XXX: some zip files don't seem to be structured such that dirs
// get created before files. Create base dir if not there yet.
fs.stat(basedir, function(err, stats) {
if(err) {
if(err.code !== "ENOENT") {
return callback(err);
}
fs.mkdirp(basedir, function(err) {
if(err) {
return callback(err);
}
fs.writeFile(path.absPath, path.data, callback);
});
} else {
fs.writeFile(path.absPath, path.data, callback);
fs.mkdirp(basedir, function (err) {
if (err) {
return callback(err);
}
fs.stat(path.absPath, function(err, stats) {
if(err && err.code !== "ENOENT") {
return callback(err);

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Too much indent here.

@humphd

humphd May 31, 2017

Member

Too much indent here.

}
if (stats.type === "FILE") {
showOverwriteWarning(path.absPath, function (err, result) {
if (err) {
return callback(err);
}
if (result === OVERWRITE_OPERATION) {
fs.writeFile(path.absPath, path.data, callback);
} else if (result === KEEP_EXISTING_OPERATION) {
callback();
} else if (result === CANCEL_OPERATION) {
callback(new Error("Operation Cancelled"));
}
});
}
});
});
}
}
@@ -227,11 +288,33 @@ define(function (require, exports, module) {
}
fs.mkdirp(basedir, function(err) {
if(err && err.code !== "EEXIST") {

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Why this change?

@humphd

humphd May 31, 2017

Member

Why this change?

if(err) {
return callback(err);
}
fs.writeFile(path, new Buffer(data), {encoding: null}, callback);
fs.stat(path, function (err, stats) {
if (err && err.code !== "ENOENT") {
return callback(err);
}
if (stats.type !== "FILE") {
return callback();
}
showOverwriteWarning(path, function (err, result) {
if (err) {
return callback(err);
}
if (result === OVERWRITE_OPERATION) {
fs.writeFile(path, new Buffer(data), {encoding: null}, callback);
} else if (result === KEEP_EXISTING_OPERATION) {
return callback();
} else if (result === CANCEL_OPERATION) {
callback(new Error("Operation Cancelled"));
}
});
});
});
}
@@ -244,6 +327,10 @@ define(function (require, exports, module) {
function writeCallback(err) {
if(err) {
if (err.message === "Operation Cancelled") {
finish(err);
return;
}
console.error("[Bramble untar] couldn't extract file", err);
}
@@ -38,6 +38,7 @@ define(function (require, exports, module) {
StringUtils = require("utils/StringUtils"),
Filer = require("filesystem/impls/filer/BracketsFiler"),
Path = Filer.Path,
fs = Filer.fs(),
Content = require("filesystem/impls/filer/lib/content"),
LanguageManager = require("language/LanguageManager"),
StartupState = require("bramble/StartupState"),
@@ -134,13 +135,13 @@ define(function (require, exports, module) {
});
}
function handleRegularFile(deferred, file, filename, buffer, encoding) {
function saveFile(deferred, file, filename, buffer, encoding) {
file.write(buffer, {encoding: encoding}, function(err) {
if (err) {
onError(deferred, filename, err);
return;
}

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Get rid of this change.

@humphd

humphd May 31, 2017

Member

Get rid of this change.

// See if this file is worth trying to open in the editor or not
if(shouldOpenFile(filename, encoding)) {
pathList.push(filename);
@@ -150,11 +151,55 @@ define(function (require, exports, module) {
});
}
function handleRegularFile(deferred, file, filename, buffer, encoding) {
fs.exists(filename, function(doesExist) {
if (doesExist) {
console.log("File: ", filename, " already exists!");

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Remove this.

@humphd

humphd May 31, 2017

Member

Remove this.

// File exists. Prompt user for action
Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_INFO,
Strings.FILE_EXISTS_HEADER,
StringUtils.format(Strings.DND_FILE_REPLACE, FileUtils.getBaseName(filename)),
[
{
className : Dialogs.DIALOG_BTN_CLASS_NORMAL,
id : Dialogs.DIALOG_BTN_CANCEL,
text : Strings.CANCEL
},
{
className : Dialogs.DIALOG_BTN_CLASS_NORMAL,
id : Dialogs.DIALOG_BTN_IMPORT,
text : Strings.USE_IMPORTED
},
{
className : Dialogs.DIALOG_BTN_CLASS_PRIMARY,
id : Dialogs.DIALOG_BTN_OK,
text : Strings.KEEP_EXISTING
}
]
)
.done(function(id) {
if (id === Dialogs.DIALOG_BTN_IMPORT) {
// Override file per user's request
saveFile(deferred, file, filename, buffer, encoding);
}
});
} else {
// File doesn't exist. Save without prompt

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Do this case first, and then return, and then you don't need to indent everything above so much:

if (!doesExist) {
    // File doesn't exist. Save without prompt
    saveFile(deferred, file, filename, buffer, encoding);
    return;
}

// rest of code here for other case.
@humphd

humphd May 31, 2017

Member

Do this case first, and then return, and then you don't need to indent everything above so much:

if (!doesExist) {
    // File doesn't exist. Save without prompt
    saveFile(deferred, file, filename, buffer, encoding);
    return;
}

// rest of code here for other case.
saveFile(deferred, file, filename, buffer, encoding);
}
});
}
function handleZipFile(deferred, file, filename, buffer, encoding) {
var basename = Path.basename(filename);
ArchiveUtils.unzip(buffer, { root: parentPath }, function(err) {
if (err) {
if (err.message === "Operation Cancelled") {
return deferred.resolve();
}
onError(deferred, filename, new Error(Strings.DND_ERROR_UNZIP));
return;
}
@@ -168,6 +213,9 @@ define(function (require, exports, module) {
ArchiveUtils.untar(buffer, { root: parentPath }, function(err) {
if (err) {
if (err.message === "Operation Cancelled") {
return deferred.resolve();
}
onError(deferred, filename, new Error(Strings.DND_ERROR_UNTAR));
return;
}
View
@@ -140,7 +140,6 @@ define(function (require, exports, module) {
return Async.doInParallel(paths, function (path, idx) {
var result = new $.Deferred();
// Only open files.
FileSystem.resolve(path, function (err, item) {
if (!err && item.isFile) {
@@ -182,6 +181,7 @@ define(function (require, exports, module) {
return result.promise();
}, false)
.fail(function () {
console.log("fail");

This comment has been minimized.

@humphd

humphd May 31, 2017

Member

Revert all the changes in this file.

@humphd

humphd May 31, 2017

Member

Revert all the changes in this file.

function errorToString(err) {
if (err === ERR_MULTIPLE_ITEMS_WITH_DIR) {
return Strings.ERROR_MIXED_DRAGDROP;
View
@@ -43,6 +43,7 @@ define(function (require, exports, module) {
DIALOG_BTN_OK = "ok",
DIALOG_BTN_DONTSAVE = "dontsave",
DIALOG_BTN_SAVE_AS = "save_as",
DIALOG_BTN_IMPORT = "import",
DIALOG_CANCELED = "_canceled",
DIALOG_BTN_DOWNLOAD = "download";
@@ -443,6 +444,7 @@ define(function (require, exports, module) {
window.addEventListener("resize", setDialogMaxSize);
exports.DIALOG_BTN_CANCEL = DIALOG_BTN_CANCEL;
exports.DIALOG_BTN_IMPORT = DIALOG_BTN_IMPORT;
exports.DIALOG_BTN_OK = DIALOG_BTN_OK;
exports.DIALOG_BTN_DONTSAVE = DIALOG_BTN_DONTSAVE;
exports.DIALOG_BTN_SAVE_AS = DIALOG_BTN_SAVE_AS;
ProTip! Use n and p to navigate between commits in a pull request.