Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add "New Folder" and "Rename" features #1719

Merged
merged 9 commits into from Oct 3, 2012

Conversation

Projects
None yet
5 participants
Member

gruehle commented Sep 26, 2012

This pull request adds two new features: New Folder, and Rename file/folder.

IMPORTANT: This pull request requires adobe/brackets-shell#120

USAGE

The New Folder command is added to the File menu and the project context menu. When selected, a new folder is created at the same level as the current selection in the project tree. If nothing in the project tree is selected (for example, if a file in the working set is selected), the folder is created at the root of the project folder.

The Rename command is added to the project context menu. It will rename the selected file or folder. Note that the Rename command will only work on project tree items. You cannot rename a file in the working set.

API NOTES

DocumentManager dispatches a new fileNameChange event whenever the name of a file or folder has changed.

DirectoryEntry.getDirectory() is now implemented. This will return a reference to an existing directory or create a new directory if needed.

UNIT TESTS

Unit tests are added for rename(), but not directory creation. We need an API to remove a directory before we can add unit tests that create a directory.

src/project/ProjectManager.js
@@ -979,6 +1020,97 @@ define(function (require, exports, module) {
}
+ /**
+ * Rename the selected item in the project tree
+ */
+ function renameSelectedItem() {
@peterflynn

peterflynn Sep 26, 2012

Member

Drive-by comment: is it worth factoring this so the rename functionality is programmatically accessible? People can't really call the raw fs.rename() directly since it doesn't send out notifications or update any of the UI.

@gruehle

gruehle Sep 28, 2012

Member

Good call. Done.

Member

peterflynn commented Sep 26, 2012

Oh, one other comment actually -- the inline editor UI shows the filename in two places, so it needs to listen for the notification to. (Or maybe its TextRanges do, and then the inline editor UI listens to something they dispatch in turn).

The Find in Files panel could be another one, but I think since we don't keep it up to date in other cases (e.g. the text content changing causing line numbers to shift) we could safely ignore it for now.

Member

gruehle commented Sep 28, 2012

For the inline editor, we actually get lucky. Only the filename is shown. If the file is renamed, it is selected before it is renamed, so the inline editor is closed. If a folder containing the file is renamed, no UI needs to change and the functionality works since the underlying document path is changed.

Find in Files has been fixed.

@ghost ghost assigned redmunds Oct 1, 2012

Contributor

redmunds commented Oct 1, 2012

On Windows, when you use New Folder, the initial folder named "Untitled" is not sorted correctly. It's listed with the files (at the bottom) and not with the folders (at the top). Even after changing the name, it's still sorted with the files.

src/nls/root/strings.js
@@ -140,6 +143,7 @@ define({
// File menu commands
"FILE_MENU" : "File",
"CMD_FILE_NEW" : "New",
+ "CMD_FILE_NEW_FOLDER" : "New Folder",
@redmunds

redmunds Oct 1, 2012

Contributor

Now that we have "New Folder", I think "New" should be changed to "New File".

src/nls/root/strings.js
@@ -148,6 +152,7 @@ define({
"CMD_FILE_SAVE" : "Save",
"CMD_FILE_SAVE_ALL" : "Save All",
"CMD_LIVE_FILE_PREVIEW" : "Live Preview",
+ "CMD_FILE_RENAME" : "Rename\u2026",
@redmunds

redmunds Oct 1, 2012

Contributor

I don't think "Rename..." should have ellipses, since it uses inline editing (i.e. it does not invoke a modal dialog).

@jasonsanjose

jasonsanjose Oct 1, 2012

Member

I thought we used ellipses when we require more input.

@redmunds

redmunds Oct 1, 2012

Contributor

I always thought the distinction was to use ellipses if you're invoking a modal dialog. I don't see ellipses after Rename anywhere else (including Windows File Explorer, DW, VS, Xcode). Also, "New" and "New Folder" require the same additional input as "Rename", and I don't think that we want to add ellipses to those.

@gruehle

gruehle Oct 2, 2012

Member

Yeah, since we don't have ellipses for "New File" or "New Folder", we should probably leave it off "Rename" too. Done.

src/document/DocumentCommandHandlers.js
@@ -95,9 +95,13 @@ define(function (require, exports, module) {
}
}
- function handleCurrentDocumentChange() {
+ function resetDocumentTitle() {
@redmunds

redmunds Oct 1, 2012

Contributor

"reset" sounds like it's being changed back to a previous state. How about updateDocumentTitle() or changeDocumentTitle() ?

@gruehle

gruehle Oct 2, 2012

Member

Good suggestion. Done.

src/document/DocumentManager.js
+ FileUtils.updateFileEntryPath(_openDocuments[newKey].file, oldName, newName);
+ }
+ }
+ }
@redmunds

redmunds Oct 1, 2012

Contributor

In the case of a file (as opposed to a folder), we can exit this loop after we find the first item because we know there will be at most 1. So, it might be worth adding an isFolder parameter to this function.

src/project/ProjectManager.js
- errorCleanup();
+ var successCallback = function (entry) {
+ data.rslt.obj.data("entry", entry);
+ if (isFolder) {
@redmunds

redmunds Oct 1, 2012

Contributor

Updating these classes need to be done earlier so that on Windows the folder is sorted correctly when first created.

@gruehle

gruehle Oct 2, 2012

Member

This is a bit of a chicken-and-egg problem because jsTree sorts after the item is created, but before any callbacks are made.

I added an explicit .sort() call if the new item is a folder. I don't have a Windows system here to verify on, so hopefully this will work :-)

@redmunds

redmunds Oct 2, 2012

Contributor

That doesn't fix it. I think the problem is the second parameter to "_projectTree.jstree("sort", _projectTree);". is supposed to be the parent node (not the whole tree). Note that sorting doesn't even work in the project root.

Note that whatever Rename is doing causes the files/folders to be re-sorted.

@gruehle

gruehle Oct 3, 2012

Member

The problem is the sort plugin listens for the same "jstree.create" event, and does the sorting immediately. We register for the same event, but are second in line since we register after the plugin.

I tried one more thing - call sort, but this time sorting the parent of the newly-created item. If this doesn't work then I'll need to wait until I get access to a Windows machine to fix it.

@redmunds

redmunds Oct 3, 2012

Contributor

Yes, this fixes the sorting. But, the new folder does not get selected. The selection goes to the position where the new folder was before it was re-sorted.

@gruehle

gruehle Oct 3, 2012

Member

OK, let's see if this works (sorry for making you test my changes like this :-) )

The selection is now done after the sorting.

src/project/ProjectManager.js
+ if (!err) {
+ // Update all nodes in the project tree.
+ // All other updating is done by DocumentManager.notifyFileNameChanged() below
+ var nodes = _projectTree.find(".jstree-leaf, .jstree-open, .jstree-closed"),
@redmunds

redmunds Oct 2, 2012

Contributor

We only need to update paths of the sub-tree of the renamed folder. And if it's a file, then only the single node. Always updating the entire tree could have a noticable performance impact.

@gruehle

gruehle Oct 2, 2012

Member

I'm not worried about any potential performance problems:

  • FileUtils.updateFileEntryPath() is a no-op (and exits quickly) if the file entry path doesn't contain oldName
  • We would need to do a basic path check anyway to find the tree items whose path contains oldName anyway, so this isn't adding any extra work.
  • Renaming is a user-driven operation that is not done frequently. Even if this iteration took hundreds of milliseconds (which it shouldn't....), that would not be a problem.
Contributor

redmunds commented Oct 2, 2012

Initial review complete

Member

gruehle commented Oct 2, 2012

Thanks for reviewing! All comments have been addressed and changes pushed.

Contributor

redmunds commented Oct 2, 2012

I notice another bug that the "New File" recognizes when a file named Untitled.js already exists, and then generates a unique name (e.g. Untitled-1.js), but "New Folder" does not generate a unique name.

Contributor

redmunds commented Oct 3, 2012

UTR BUG: I keep hitting the case where "New File" and "New Folder" stop working, but I can't nail down a reproducible recipe. Anyway, something to be on the lookout for.

Member

gruehle commented Oct 3, 2012

I fixed the "New Folder" untitled naming problem. I haven't seen cases where "New File" or "New Folder" break. Are you seeing this on Mac or PC?

Member

njx commented Oct 3, 2012

I've seen the failing "New File" in master, so it's not related to Glenn's changes. There seems to be some case where it throws an exception when trying to scroll the new file into view in the project panel. I'll try to find repro steps and file a bug.

Member

gruehle commented Oct 3, 2012

The selection is now done after sorting. Hopefully this resolves the last issue.

Contributor

redmunds commented Oct 3, 2012

Yes, this fixes the selection issue. This pull request is now ready to merge as soon as the necessary brackets-shell pull request gets merged.

Member

jasonsanjose commented Oct 3, 2012

Merging

jasonsanjose added a commit that referenced this pull request Oct 3, 2012

Merge pull request #1719 from adobe/glenn/new-features
Add "New Folder" and "Rename" features

@jasonsanjose jasonsanjose merged commit 1c3c822 into master Oct 3, 2012

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