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

Directory creation error #4244

Merged
merged 10 commits into from
Jul 11, 2013
Merged

Directory creation error #4244

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
53ed7ee
Fixed directory already exists issue
thefirstofthe300 Jun 14, 2013
33e01f3
Merge branch 'duplicate-directory-dialog-fix' into directory-creation…
thefirstofthe300 Jun 15, 2013
6731218
Implemented code review suggestions
thefirstofthe300 Jun 28, 2013
29cd68d
Finished code review fixes
thefirstofthe300 Jun 28, 2013
e17c0a7
Added variables to strings
thefirstofthe300 Jun 28, 2013
de84314
Merge branch 'master' into directory-creation-error
thefirstofthe300 Jul 9, 2013
988e637
Removed Directory_Already_Exists and adopted comments
thefirstofthe300 Jul 9, 2013
0f26eba
Implemented more comments
thefirstofthe300 Jul 10, 2013
b87a928
Fixed typo that broke error_cleanup
thefirstofthe300 Jul 11, 2013
ca269fa
Implemented final comments (I hope)
thefirstofthe300 Jul 11, 2013
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
11 changes: 7 additions & 4 deletions src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ define({
"NO_MODIFICATION_ALLOWED_ERR" : "The target directory cannot be modified.",
"NO_MODIFICATION_ALLOWED_ERR_FILE" : "The permissions do not allow you to make modifications.",
"FILE_EXISTS_ERR" : "The file or directory already exists.",
"FILE" : "file",
"DIRECTORY" : "directory",

// Project error strings
"ERROR_LOADING_PROJECT" : "Error loading project",
Expand All @@ -55,11 +57,12 @@ define({
"ERROR_RENAMING_FILE" : "An error occurred when trying to rename the file <span class='dialog-filename'>{0}</span>. {1}",
"ERROR_DELETING_FILE_TITLE" : "Error deleting file",
"ERROR_DELETING_FILE" : "An error occurred when trying to delete the file <span class='dialog-filename'>{0}</span>. {1}",
"INVALID_FILENAME_TITLE" : "Invalid file name",
"INVALID_FILENAME_TITLE" : "Invalid {0} name",
"INVALID_FILENAME_MESSAGE" : "Filenames cannot contain the following characters: /?*:;{}<>\\| or use any system reserved words.",
"FILE_ALREADY_EXISTS" : "The file <span class='dialog-filename'>{0}</span> already exists.",
"ERROR_CREATING_FILE_TITLE" : "Error creating file",
"ERROR_CREATING_FILE" : "An error occurred when trying to create the file <span class='dialog-filename'>{0}</span>. {1}",
"FILE_ALREADY_EXISTS" : "The {0} <span class='dialog-filename'>{1}</span> already exists.",
"DIRECTORY_ALREADY_EXISTS" : "The directory <span class='dialog-filename'>{0}</span> already exists.",
Copy link
Member

Choose a reason for hiding this comment

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

You probably intended to remove DIRECTORY_ALREADY_EXISTS right?

"ERROR_CREATING_FILE_TITLE" : "Error creating {0}",
"ERROR_CREATING_FILE" : "An error occurred when trying to create the {0} <span class='dialog-filename'>{1}</span>. {2}",

// Application error strings
"ERROR_IN_BROWSER_TITLE" : "Oops! {APP_NAME} doesn't run in browsers yet.",
Expand Down
25 changes: 14 additions & 11 deletions src/project/ProjectManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,11 +1257,14 @@ define(function (require, exports, module) {
(error.name === NativeFileError.TYPE_MISMATCH_ERR)) {
Copy link
Member

Choose a reason for hiding this comment

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

Almost. I went back to the original bug to test and found some more issues not mentioned.

If you create a file with a name foo, the try to create a directory of the same name, you get "The directory foo already exists" instead of "The file foo already exists"...and vice versa.

You can split the if here with an else if branch to handle TYPE_MISMATCH_ERR separate from PATH_EXISTS_ERROR, that should fix it.

Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_ERROR,
Strings.INVALID_FILENAME_TITLE,
StringUtils.format(
Strings.FILE_ALREADY_EXISTS,
StringUtils.breakableUrl(data.rslt.name)
)
!isFolder ?
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to:

StringUtils.format(Strings.INVALID_FILENAME_TITLE, isFolder ? Strings.DIRECTORY : Strings.FILE);
StringUtils.format(Strings.FILE_ALREADY_EXISTS,    isFolder ? Strings.DIRECTORY : Strings.FILE, StringUtils.breakableUrl(data.rslt.name));

Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be better to assign the inline if to a variable at the start of the callback and reuse it in the 4 formats, to reduce the line width.

Copy link
Member

Choose a reason for hiding this comment

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

Might be better? Definitely better. Thanks!

StringUtils.format(Strings.INVALID_FILENAME_TITLE, Strings.FILE) :
StringUtils.format(Strings.INVALID_FILENAME_TITLE, Strings.DIRECTORY),
!isFolder ?
StringUtils.format(Strings.FILE_ALREADY_EXISTS, Strings.FILE,
StringUtils.breakableUrl(data.rslt.name)) :
StringUtils.format(Strings.FILE_ALREADY_EXISTS, Strings.DIRECTORY,
StringUtils.breakableUrl(data.rslt.name))
);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

At this else branch there is another error dialog that always display "File" in the strings as the code fixed. Maybe this could be fixed here too.

var errString = error.name === NativeFileError.NO_MODIFICATION_ALLOWED_ERR ?
Expand All @@ -1270,12 +1273,12 @@ define(function (require, exports, module) {

Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_ERROR,
Strings.ERROR_CREATING_FILE_TITLE,
StringUtils.format(
Strings.ERROR_CREATING_FILE,
StringUtils.breakableUrl(data.rslt.name),
errString
)
isFolder ? StringUtils.format(Strings.ERROR_CREATING_FILE_TITLE, Strings.DIRECTORY) :
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to:

StringUtils.format(Strings.ERROR_CREATING_FILE_TITLE, isFolder ? Strings.DIRECTORY : Strings.FILE),
StringUtils.format(Strings.ERROR_CREATING_FILE,       isFolder ? Strings.DIRECTORY : Strings.FILE, StringUtils.breakableUrl(data.rslt.name), errString)

StringUtils.format(Strings.ERROR_CREATING_FILE_TITLE, Strings.FILE),
isFolder ? StringUtils.format(Strings.ERROR_CREATING_FILE, Strings.DIRECTORY,
StringUtils.breakableUrl(data.rslt.name), errString) :
StringUtils.format(Strings.ERROR_CREATING_FILE, Strings.FILE,
StringUtils.breakableUrl(data.rslt.name), errString)
);
}

Expand Down