Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll 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 from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25bfe6b
Added Dialog prompt for single file import
sdalmeida Apr 10, 2017
fca9257
Added warning if imported file overwrites local files.
sdalmeida May 12, 2017
0e9075d
Fixed Cancel on tar archives
sdalmeida May 12, 2017
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
4 changes: 4 additions & 0 deletions locales/en-US/editor.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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?
Copy link

Choose a reason for hiding this comment

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

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


# Image Viewer
IMAGE_DIMENSIONS={0} (width) &times; {1} (height) pixels
Expand Down
119 changes: 103 additions & 16 deletions src/filesystem/impls/filer/ArchiveUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

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) {
Expand Down Expand Up @@ -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') {
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

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

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"));
}
});
}
});
});
}
}
Expand Down Expand Up @@ -227,11 +288,33 @@ define(function (require, exports, module) {
}

fs.mkdirp(basedir, function(err) {
if(err && err.code !== "EEXIST") {
Copy link

Choose a reason for hiding this comment

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

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"));
}
});
});
});
}

Expand All @@ -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);
}

Expand Down
52 changes: 50 additions & 2 deletions src/filesystem/impls/filer/lib/WebKitFileImport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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;
}

Copy link

Choose a reason for hiding this comment

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

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);
Expand All @@ -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!");
Copy link

Choose a reason for hiding this comment

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

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
Copy link

Choose a reason for hiding this comment

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

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;
}
Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/utils/DragAndDrop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -182,6 +181,7 @@ define(function (require, exports, module) {
return result.promise();
}, false)
.fail(function () {
console.log("fail");
Copy link

Choose a reason for hiding this comment

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

Revert all the changes in this file.

function errorToString(err) {
if (err === ERR_MULTIPLE_ITEMS_WITH_DIR) {
return Strings.ERROR_MIXED_DRAGDROP;
Expand Down
2 changes: 2 additions & 0 deletions src/widgets/Dialogs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;
Expand Down