File Rename should complete when clicking anywhere in project panel

[lkcampbell]

OS: Windows 7

Build: sprint 24 development build 0.24.0-0 (master a9e2771)

Repro Steps:

1. Open a project that has a small amount of files. The goal is to have plenty of empty space at the bottom of the project tree.
2. Rename one of the files and, while the edit box is still open, click anywhere on the empty space at the bottom of the project tree.

Observed Result:

Nothing happens. The rename edit box is still around the file name with the cursor blinking.

Expected Result:

The same result that occurs when I click on the working set, the project tree, or even the main editor window: the renaming completes and the rename file box goes away.

[njx]

Reviewed. Marking medium priority and starter bug--seems like it should be straightforward to fix.

[vladnicula]

There are a few ways to fix this. I solved it by calling ProjectManager.forceFinishRename(); when clicking on #sidebar in SidebarView.js htmlReady callback. Another option is to look for the sidebar parent in ProjectManager.js.

I'd like to make a pull request with this. Which approach should I take? :-?

[peterflynn]

@vladnicula I'm a little unclear how the two approaches you're describing differ. Is the difference in which DOM node you listen for clicks on, or just where in the codebase the listener is created?

[vladnicula]

@peterflynn it is the second case. in order to make it work the code needs to bind to #sidebar. The sidebar dom node is stored in SidebarView.js, however from a functionality perspective, this is a behavior that is best added in ProjectManager.js since the rest of the code related to the project tree is there. ProjectManager.js has a dom referece to a child node of the sidebar and uses that for all its binding. So, my question is this : is it ok to add the listener in sidebarview, or in projectmanager? in the second case projectmanager should have a reference to the sidebar node.

[peterflynn]

@vladnicula Any particular reason to care about only clicks in the sidebar? If I click in a blank part of the right-hand toolbar, for example, shouldn't that terminate the rename too? In which case, a capture listener on the overall document might be cleanest. (With logic to ignore clicks within the rename field itself, of course).

[vladnicula]

@peterflynn That's a good point!

So instad of #sidebar it should document or maybe just body - not sure if it will make any difference-. Last question: Where would you add the document.body.addEventListener with ProjectManager.forceFinishRename(); ? I see a lot of files that bind to window.document.body.

[JeffryBooher]

@vladnicula were you still interesting in fixing this issue?

[vladnicula]

@JeffryBooher Yes I am. I know what needs to be added in order to make it work. I'm just not certain where is the best place for it - in which file.

[JeffryBooher]

@vladnicula Hard to tell without knowing how you plan to implement it. I'd say add it to ProjectManager.js and if it looks like something that doesn't belong then it should get pointed out in the code review along with suggestions on where to put it. I think the main thing is to get it working and up for a pull request then let the community guide you on how to make it perfect.

[JeffryBooher]

@vladnicula Do you have time to finish this? We have another issue which we'd like you to add to this.

[vladnicula]

@JeffryBooher I started working on another project. Thanks for the opportunity. Best of luck! 👍

[lkcampbell]

@vladnicula, to clarify, are you no longer working on this bug? If that is the case, I can take it over.

@JeffryBooher, I can look at the other bug as well. Are you referring to issue #4693 or a different issue?

[JeffryBooher]

@lkcampbell yes, issue #4693 should be fixed as well. Thanks for taking this one!

[lkcampbell]

@JeffryBooher, I have the fix done. Will post in a bit.

Per your suggestions on the previous pull request, I put listeners on the main view click and the menu popup. Collapsing and expanding the panel appears to automatically finish the rename so I don't think the listener on the sidebar panelCollapsed is necessary. I did, however, have to put a listener on the sidebar panelResizeStart because clicking on the sidebar divider does not force the rename to finish.

I tested a listener on the contextmenu per your suggested fix on issue #4693. It worked...sort of. The context menu appears and the rename is forced to finish, but then the file receives focus and the context menu suddenly disappears. In other words, it doesn't look very good. I'd like to play around with it a bit more to see if I can get it to behave better. Since issue #4693 is low priority and this issue is medium priority, I'm going to get this fix submitted now and look at issue #4693 separately later on.

[lkcampbell]

@JeffryBooher, we might not want to force a rename when opening a menu. There are a few commands in the menu that are very useful within the context of renaming a file. The most obvious are Copy and Paste.

An unexpected side effect I noticed today, using the shortcut key for any of the menu items activates the listener as well. So I cannot, for example, rename a file to highlight its name, then hit Ctrl-C to copy the file name. The rename is forced to end just before the text is copied.

[peterflynn]

@lkcampbell The context menu popping closed sounds like a case of #1910, so maybe not that important to fix yet.

Note that the context menu doesn't actually have Copy/Paste items -- it's not possible to invoke Copy/Paste from HTML UI -- so there's less need to worry about being unable to access menu items. We definitely don't want to block keyboard shortcuts though. It sounds like there might be a brackets-shell bug in when APP_BEFORE_MENUPOPUP is triggered. But I think actually that's the wrong event to use here anyway, since it's for native menus. For HTML context menu popups, the "beforeContextMenuOpen" event that ContextMenu dispatches should be more correct (I think you only need to listen to the one: Menus.getContextMenu(ContextMenuIds.PROJECT_MENU)).

[peterflynn]

Oh, one other thought: it's probably useful to be able to resize the sidebar during a rename, in case you need more room to see the text field. I'm guessing Resizer does something on mousedown that causes you to never get a click event on it, so... I'm wondering if maybe listening for mousedown globally instead of click is safer.

[lkcampbell]

@peterflynn, you bring up a lot of good points.

Since a click to the main view is such a broad event, I am going to keep things as simple as possible while still addressing this bug.

The only event I am keeping at this point is the main view mouse click.

The context menu solution you suggested still is affected by issue #1910.

The shortcut being blocked by Commands.APP_BEFORE_MENUPOPUP needs to filed but I need to get together a repro and don't have time presently to do that. Maybe I can get that done in a couple of days.

Submitting an updated pull request now.

[lkcampbell]

@peterflynn, I'm having a bit of trouble trying to figure out how to set up a repro for this "shortcut being blocked by Commands.APP_BEFORE_MENUPOPUP" issue. I can't really provide a gist. Maybe I could create an extension but that seems like overkill. The only way I have to demonstrate the problem right now is to change the core Brackets code.

Any ideas on this or should I just describe the problem in general and forget about providing code demonstrating the problem?

[JeffryBooher]

FBNC @lkcampbell

[lkcampbell]

Is it okay for me to confirm and close an issue when I created the fix? Or do you guys want someone else to confirm and close it?

[redmunds]

@lkcampbell I think this is simple enough for you to close. For complex issues, it's best to get someone else to confirm.

[lkcampbell]

Confirmed that it is fixed.