[CLOSED] File Rename should complete when clicking anywhere in project panel

[core-ai-bot]

Issue by lkcampbell
Sunday May 05, 2013 at 16:02 GMT
Originally opened as adobe/brackets#3720

---

OS: Windows 7

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

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.

[core-ai-bot]

Comment by njx
Monday May 13, 2013 at 18:22 GMT

---

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

[core-ai-bot]

Comment by vladnicula
Friday May 17, 2013 at 19:48 GMT

---

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? :-?

[core-ai-bot]

Comment by peterflynn
Sunday May 19, 2013 at 20:10 GMT

---

@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?

[core-ai-bot]

Comment by vladnicula
Monday May 20, 2013 at 03:09 GMT

---

@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.

[core-ai-bot]

Comment by peterflynn
Monday May 20, 2013 at 07:16 GMT

---

@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).

[core-ai-bot]

Comment by vladnicula
Monday May 20, 2013 at 08:20 GMT

---

@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.

[core-ai-bot]

Comment by JeffryBooher
Friday Jun 14, 2013 at 17:55 GMT

---

@vladnicula were you still interesting in fixing this issue?

[core-ai-bot]

Comment by vladnicula
Saturday Jun 15, 2013 at 04:50 GMT

---

@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.

[core-ai-bot]

Comment by JeffryBooher
Friday Jun 21, 2013 at 19:02 GMT

---

@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.

[core-ai-bot]

Comment by JeffryBooher
Thursday Aug 08, 2013 at 19:36 GMT

---

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

[core-ai-bot]

Comment by vladnicula
Tuesday Aug 13, 2013 at 07:09 GMT

---

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

[core-ai-bot]

Comment by lkcampbell
Saturday Aug 24, 2013 at 17:05 GMT

---

@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?

[core-ai-bot]

Comment by JeffryBooher
Sunday Aug 25, 2013 at 03:16 GMT

---

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

[core-ai-bot]

Comment by lkcampbell
Sunday Aug 25, 2013 at 04:59 GMT

---

@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.

[core-ai-bot]

Comment by lkcampbell
Sunday Aug 25, 2013 at 16:05 GMT

---

@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.

[core-ai-bot]

Comment by peterflynn
Monday Aug 26, 2013 at 04:35 GMT

---

@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)).

[core-ai-bot]

Comment by peterflynn
Monday Aug 26, 2013 at 04:37 GMT

---

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.

[core-ai-bot]

Comment by lkcampbell
Monday Aug 26, 2013 at 13:37 GMT

---

@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.

[core-ai-bot]

Comment by lkcampbell
Saturday Aug 31, 2013 at 16:38 GMT

---

@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?

[core-ai-bot]

Comment by JeffryBooher
Friday Sep 06, 2013 at 22:39 GMT

---

FBNC@lkcampbell

[core-ai-bot]

Comment by lkcampbell
Friday Sep 06, 2013 at 22:46 GMT

---

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?

[core-ai-bot]

Comment by redmunds
Friday Sep 06, 2013 at 22:50 GMT

---

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

[core-ai-bot]

Comment by lkcampbell
Friday Sep 06, 2013 at 22:54 GMT

---

Confirmed that it is fixed.