Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Prevent re-entrancy into handleFileNewInProject(), fixes issue #738 #794

Merged
merged 3 commits into from

2 participants

@tvoliter

handleFileNewInProject() initiates a two step process where the user is first prompted to name a file and then it is written asynchronously to disk. This process must complete before a new file is generated. This change prevents reentrancy and also closes the rename field when the user chooses "new file" during a rename.

I earlier tried a fancier approach that queue up user requests for new files. This got overly complex plus in the future "new file" is likely to create a new in-memory file rather than one on disk. In which case this code will change.

Fixes issue #738

@peterflynn peterflynn commented on the diff
src/document/DocumentCommandHandlers.js
@@ -257,7 +257,22 @@ define(function (require, exports, module) {
return result.promise();
}
+ /**
+ * Prevent re-entrancy into handleFileNewInProject()
+ *
+ * handleFileNewInProject() first prompts the user to name a file and then asynchronously writes the file when the
+ * filename field loses focus. This boolean prevent additional calls to handleFileNewInProject() when an existing
+ * file creation call is outstanding
+ */
+ var fileNewInProgress = false;
@peterflynn Owner

Nits:

  • prevent -> prevents
  • Add a space between this var and the function below
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/project/ProjectManager.js
@@ -778,6 +778,14 @@ define(function (require, exports, module) {
return result.promise();
}
+ /**
+ * Forces createNewItem() to complete by removing focus from the rename field which causes
+ * the new file to be written to disk
+ */
+ function closeRenameInput() {
@peterflynn Owner

Is there a name that makes it more clear that the outcome is completing the operation rather than canceling it? Maybe something like forceCommitRename() or forceFinishRename()?

Also, we might want to go with something like "FileNaming" instead of "Rename" since it's not really _re_naming an existing file in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterflynn
Owner

Done reviewing. Tested and the behavior seems reasonable enough.

As Ty pointed out, eventually File > New will create a new untitled document (which doesn't get named until you save it), so this is really just a stopgap fix until then.

@tvoliter

Peter, thanks for the review. My changes are pushed.

I opted for the name "forceFinishRename()". Even though the user isn't naming a file on disk, they are renaming the file in terms of the text presented in the tree since a default name was suggested to them and they get to change it. I can image using this same function in the future during a real file rename so I felt "rename" was better than "name" or "naming field"

@peterflynn
Owner

Sounds good enough to me

@peterflynn peterflynn merged commit f92aa7d into from
@peterflynn peterflynn referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 3, 2012
  1. @tvoliter

    prevent reentrancy into handleFileNewInProject(). Add closeRenameInpu…

    tvoliter authored
    …t() to ProjectManager to allow clients to end renaming.
  2. @tvoliter

    fix types. Rename var.

    tvoliter authored
  3. @tvoliter

    Code review fixes

    tvoliter authored
This page is out of date. Refresh to see the latest.
View
20 src/document/DocumentCommandHandlers.js
@@ -257,7 +257,23 @@ define(function (require, exports, module) {
return result.promise();
}
+ /**
+ * Prevents re-entrancy into handleFileNewInProject()
+ *
+ * handleFileNewInProject() first prompts the user to name a file and then asynchronously writes the file when the
+ * filename field loses focus. This boolean prevent additional calls to handleFileNewInProject() when an existing
+ * file creation call is outstanding
+ */
+ var fileNewInProgress = false;
@peterflynn Owner

Nits:

  • prevent -> prevents
  • Add a space between this var and the function below
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
function handleFileNewInProject() {
+
+ if (fileNewInProgress) {
+ ProjectManager.forceFinishRename();
+ return;
+ }
+ fileNewInProgress = true;
+
// Determine the directory to put the new file
// If a file is currently selected, put it next to it.
// If a directory is currently selected, put it in it.
@@ -274,7 +290,9 @@ define(function (require, exports, module) {
// of validating file name, creating the new file and selecting.
var deferred = _getUntitledFileSuggestion(baseDir, "Untitled", ".js");
var createWithSuggestedName = function (suggestedName) {
- ProjectManager.createNewItem(baseDir, suggestedName, false).pipe(deferred.resolve, deferred.reject, deferred.notify);
+ ProjectManager.createNewItem(baseDir, suggestedName, false)
+ .pipe(deferred.resolve, deferred.reject, deferred.notify)
+ .always(function () { fileNewInProgress = false; });
};
deferred.done(createWithSuggestedName);
View
9 src/project/ProjectManager.js
@@ -778,6 +778,14 @@ define(function (require, exports, module) {
return result.promise();
}
+ /**
+ * Forces createNewItem() to complete by removing focus from the rename field which causes
+ * the new file to be written to disk
+ */
+ function forceFinishRename() {
+ $(".jstree-rename-input").blur();
+ }
+
// Define public API
exports.getProjectRoot = getProjectRoot;
exports.isWithinProject = isWithinProject;
@@ -786,6 +794,7 @@ define(function (require, exports, module) {
exports.loadProject = loadProject;
exports.getSelectedItem = getSelectedItem;
exports.createNewItem = createNewItem;
+ exports.forceFinishRename = forceFinishRename;
// Initialize now
(function () {
Something went wrong with that request. Please try again.