Fix duplicate tree nodes when creating a file externally in a folder that was not loaded #6546
Conversation
jasonsanjose
commented
Jan 16, 2014
jasonsanjose
commented
- fix bug where unwatch was not called after file/folder was removed/renamed
- skip incremental update to jstree if directory was not loaded
…remental update to jstree if directory was not loaded.
| @@ -714,6 +722,7 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| var watchOrUnwatchCallback = function (err) { | |||
| console.error("FileSystem error in _handleDirectoryChange after watch/unwatch entries: " + err); | |||
There was a problem hiding this comment.
Having this logging here will be helpful the next time we see bad file watcher paths
There was a problem hiding this comment.
This should only log if err is truthy
|
For #6537 |
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
!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.
|
@jasonsanjose Done reviewing |
|
UGH. I think I just found a worse bug when debugging this one. If, external to brackets, I copy in a folder with a subfolder, then again externally try to rename the folder that I pasted, windows prevents me from doing so with this warning:
Note that this only happens for externally added folders that include a subfolder. Pasting in a folder with no subfolders doesn't no reproduce the issue. I assume that the file watcher for the subfolder is what is holding a file lock. I file separately since it's not quite related to this bug. That may be a blocker. Filed #6551 |
|
@peterflynn fix pushed. See above for issues not addressed. |
|
Closing in favor of #6553. |
