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

Fix duplicate tree nodes when creating a file externally in a folder that was not loaded #6546

Closed
wants to merge 3 commits into from
Closed

Fix duplicate tree nodes when creating a file externally in a folder that was not loaded #6546

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
7fc6cd7
fix bug where unwatch was not called after file was removed. skip inc…
jasonsanjose Jan 16, 2014
21de65c
add console logging for unhandled watch/unwatch error
jasonsanjose Jan 16, 2014
df0e760
only log file watcher error if error is present
jasonsanjose Jan 16, 2014
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
16 changes: 14 additions & 2 deletions src/filesystem/FileSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ define(function (require, exports, module) {
var Directory = require("filesystem/Directory"),
File = require("filesystem/File"),
FileIndex = require("filesystem/FileIndex"),
FileSystemError = require("filesystem/FileSystemError"),
WatchedRoot = require("filesystem/WatchedRoot");

/**
Expand Down Expand Up @@ -273,8 +274,15 @@ define(function (require, exports, module) {

entry.visit(visitor, function (err) {
if (err) {
requestCb(err);
return;
// Unwatching a file/folder that doesn't exist will
// trigger an expected error
if (!shouldWatch && (err === FileSystemError.NOT_FOUND)) {
visitor(entry);
Copy link
Member

Choose a reason for hiding this comment

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

It seems like there may be a flaw here: simply calling visitor() on the root won't be enough if there are subdirectories, which we'd also want to unwatch.

Copy link
Member

Choose a reason for hiding this comment

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

But I'm also concerned as to why we're getting back an error at all. I'd have expected the stat() call in FileSystemEntry.visit() and the getContents() call in _visitHelper() to both use cached data (anything that's watched on a manual-recursion OS must have been visited at least once already, so there should be something cached). Is someone calling _clearCachedData() on the removed entries? If so, it seems like a big problem because it's then impossible to recurse further (because we can't ask the OS for its children anymore)... leaving all subdirectories still watched.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it'd be best to add a new call to the NodeDomain of the form clearAllWatchesWithPrefix(path). That way we wouldn't need visit() and we wouldn't need any cached record of what the children were.

Copy link
Member Author

Choose a reason for hiding this comment

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

WRT

expected the stat() call in FileSystemEntry.visit()

Yes, at the initial rename of New folder to watch, when attempting to unwatch New folder, we find that it was never stat-ed. I confirmed this during debugging. This seems feasible right? We could have a queue of events come through with the initial creation, then renaming of the new folder and never needed to call stat?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clearing all watched paths with a prefix seems reasonable. I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take that back. Unwatching paths with a prefix seems like a bigger change to file system implementations that we should probably address as an API change separately. Would you be ok to merge this fix without something like unwatchPathsWithPrefix in FileWatcherDomain?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, for the record I figured out why that stat() is always uncached here. Whenever we get a directory change event, we immediately call _clearCachedData() on the directory in question -- which blows away cache for both that directory and all its immediate children. So in the case of a rename, we clear the cache of both the renamed item's parent dir and the renamed item itself.

Anyway, that's still a blocker for visit() since we've now lost any record of what the renamed item's children are -- and if any of the children are directories, we need to know about it. So I think the prefix-driven FileWatcherDomain fix is still the only viable solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I'll start a new branch with only the FileSystem and FileWatcherDomain fixes for #6554.

} else {
// Unexpected error
requestCb(err);
return;
}
}

// Then watch or unwatched all these entries
Expand Down Expand Up @@ -714,6 +722,10 @@ define(function (require, exports, module) {
}

var watchOrUnwatchCallback = function (err) {
if (err) {
console.error("FileSystem error in _handleDirectoryChange after watch/unwatch entries: " + err);
}

if (--counter === 0) {
callback(addedEntries, removedEntries);
}
Expand Down
29 changes: 19 additions & 10 deletions src/project/ProjectManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,8 @@ define(function (require, exports, module) {
attr : { id: "node" + _projectInitialLoad.id++ },
metadata: {
entry : entry,
compareString : _toCompareString(entry.name, entry.isDirectory)
compareString : _toCompareString(entry.name, entry.isDirectory),
is_loaded : false
}
};

Expand Down Expand Up @@ -794,10 +795,10 @@ define(function (require, exports, module) {
* jsTree back asynchronously with the node's immediate children data once the subfolder is done
* being fetched.
*
* @param {jQueryObject} treeNode jQ object for the DOM node being expanded
* @param {jQueryObject} $treeNode jQ object for the DOM node being expanded
* @param {function(Array)} jsTreeCallback jsTree callback to provide children to
*/
function _treeDataProvider(treeNode, jsTreeCallback) {
function _treeDataProvider($treeNode, jsTreeCallback) {
var dirEntry, isProjectRoot = false, deferred = new $.Deferred();

function processEntries(entries) {
Expand All @@ -807,7 +808,7 @@ define(function (require, exports, module) {

if (emptyDirectory) {
if (!isProjectRoot) {
wasNodeOpen = treeNode.hasClass("jstree-open");
wasNodeOpen = $treeNode.hasClass("jstree-open");
} else {
// project root is a special case, add a placeholder
subtreeJSON.push({});
Expand All @@ -821,31 +822,38 @@ define(function (require, exports, module) {
// This is a workaround for issue #149 where jstree would show this node as a leaf.
var classToAdd = (wasNodeOpen) ? "jstree-closed" : "jstree-open";

treeNode.removeClass("jstree-leaf jstree-closed jstree-open")
$treeNode.removeClass("jstree-leaf jstree-closed jstree-open")
.addClass(classToAdd);

// This is a workaround for a part of issue #2085, where the file creation process
// depends on the open_node.jstree event being triggered, which doesn't happen on
// empty folders
if (!wasNodeOpen) {
treeNode.trigger("open_node.jstree");
$treeNode.trigger("open_node.jstree");
}
}

deferred.resolve();
}

if (treeNode === -1) {
if ($treeNode === -1) {
// Special case: root of tree
dirEntry = _projectRoot;
isProjectRoot = true;
} else {
// All other nodes: the Directory is saved as jQ data in the tree (by _convertEntriesToJSON())
dirEntry = treeNode.data("entry");
dirEntry = $treeNode.data("entry");
}

// Fetch dirEntry's contents
dirEntry.getContents(function (err, contents, stats, statsErrs) {
// Flag load attempt so we can do incremental updates later
if (isProjectRoot) {
$projectTreeList.data("is_loaded", true);
} else {
$treeNode.data("is_loaded", true);
}

if (err) {
Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_ERROR,
Expand Down Expand Up @@ -2029,8 +2037,9 @@ define(function (require, exports, module) {

// Open the node before creating the new child
_projectTree.jstree("open_node", $directoryNode);
} else {
// Create all new nodes in a batch
} else if ($directoryNode.data("is_loaded")) {
// The directory was already loaded, so we can incrementally
// create all new nodes in a batch
Copy link
Member

Choose a reason for hiding this comment

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

What happens in the !is_loaded case? Is there a risk there will be cases where we fail to ever redraw the tree, since there's no 'else' here anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Does this fail to fix the bug if the 'watcher' folder was already expanded before pasting in the 'README' file? In that case is_loaded would be true, so we'd do exactly what this code did before...

Copy link
Member

Choose a reason for hiding this comment

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

I'm also confused about why, for this specific repro case at least, fixing FileSystem along did not make the bug go away. Once the duplicate watcher notifications are eliminated, wouldn't we only get here once?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, it seems like Jochen has a fix at https://github.com/adobe/brackets/compare/couzteau;fix-6474 that's addressing something similar. Not sure how you want to reconcile those two.

Copy link
Member Author

Choose a reason for hiding this comment

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

!is_loaded implies _treeDataProvder (and therefore Directory.getContents()) was never called to populate the directory prior to this change event. So, the first drawing of any node must be a call to jstree open_node. There should be no case where we fail to ever redraw the tree because at the very least _treeDataProvider will be called via jstree.

I made the mistake of associating this jstree issue with the FileSystem unwatch issue. When debugging this originally, I saw that the change events coming though ProjectManager._fileSystemChange included those phantom directories that should have been unwatched when the folder was renamed. Since the old-named node (e.g. the default "New folder") was removed by a prior event, the event handler would fail to find an existing tree node and bail, making no jstree modifications. I mistook this as a cause of the eventual duplicate README.md entries. That's why fixing the FileSystem did not make the bug go away.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've asked @couzteau to test this branch for a fix for his issue #6474

_createNode($directoryNode, null, addedJSON, true, true);
doRedraw = true;
}
Expand Down