Implementation of Navigate Recent Projects #4546

TomMalbran · 2013-07-23T08:49:17Z

As specified in the this Trello card, CTRL+ALT+R opens the recent project dropdown, up and down moves to the next and previous item and enter triggers a click event`on the currently selected item, which goes to the selected project or opens a new folder (if that item is selected).

JeffryBooher · 2013-07-24T01:48:41Z

src/extensions/default/RecentProjects/main.js

@@ -130,6 +142,76 @@ define(function (require, exports, module) {
    }

    /**
+     * Hide the delete button.
+     */
+    function hideDeleteButton() {


minor nit: but you should probably call this removeDeleteButton and showDeleteButton would become addDeleteButton.

Sure. These were the original names. I only moved this functions since I needed to use them and have them declared before.

JeffryBooher · 2013-07-24T02:00:54Z

done with first pass on the review

JeffryBooher · 2013-07-26T23:04:43Z

src/extensions/default/RecentProjects/main.js

+
+    // Register command handlers
+    CommandManager.register(Strings.CMD_TOGGLE_RECENT_PROJECTS, TOGGLE_DROPDOWN, handleKeyEvent);
+    KeyBindingManager.addBinding(TOGGLE_DROPDOWN, KeyboardPrefs.recentProjects);


@adrocknaphobia I don't think CTRL+ALT+R/CMD+ALT+R should toggle the recent projects menu.
The acceptance criteria doesn't specify the behavior but I would expect that CTRL+ALT+R/CMD+ALT+R would invoke the most Project MRU menu but only ESC, ENTER, clicking on another UI element or the Brackets Application Frame is deactivated (another top-level application window gains focus) would be the only things that dismiss it. This should be true if it were invoked from the keyboard or mouse.

adrocknaphobia · 2013-07-29T22:55:48Z

@TomMalbran Thanks for taking this on! Once the menu is open (Ctrl+Alt+R), the menu should be closed when hitting ESC, ENTER or when losing focus (e.g. click outside the menu). Typically menus don't toggle (at least not on Windows), but as long as ESC/ENTER works, I'm OK if Ctrl+Alt+R toggles the menu close as well.

TomMalbran · 2013-07-29T23:05:44Z

@adrocknaphobia Sure, using Esc to to close the menu sounds good. Enter already closes it, since it works as clicking on any menu item. There wasn't any mention of it in the trello card, but I figure that there should be a way to close it using keyboard events. Using Alt when a native menu is open on Windows closes it, and alt is the way to open them, so maybe we could also leave Ctrl+Alt+R to close the menu.

TomMalbran · 2013-07-29T23:24:07Z

@adrocknaphobia One other thing. There is still no way to remove projects from the list using the keyboard (can only be done clicking on the "x" button). Should we implement it using the BackSpace/Delete keys?

JeffryBooher · 2013-08-05T22:15:38Z

@adrocknaphobia that doesn't feel right to me. Can you point to another tool that does it like that? Visual Studio you can only close windows from the Windows... Dialog at the bottom of the Window menu. Then it's a dialog that you can use to manage your windows. I'm afraid that delete would be confused with deleting the file from disk or from your project. On the Mac, Cmd+W is the typical shortcut to close the currently open document which I believe is implemented and Alt+F4 on Windows is as well. If not, then I think we should just let them close only the currently open document with those shortcuts. That's my $.02

TomMalbran · 2013-08-13T03:33:09Z

@JeffryBooher I guess you are right. Using delete to remove the files might not be the best way. Anyway I think we should make it possible to remove the projects from the list using the keyboard only.

My other idea would be to make sure that tabbing works. When the menu is open tabbing should stay within the panel and should reach the "x" links. I was thinking of making the list items tab-able, so that you can tab to a list item, which will select it and show the delete icon and then tab to the delete icon. The next tab will select the following item in the list, or the first if we reached the end.

JeffryBooher · 2013-08-25T03:20:40Z

@TomMalbran That needs a UX involvement so I wouldn't get to far down the road on that but it sounds about right. Delete I think is outside the scope of this issue so I will add it to the product backlog if it isn't already. I need to test the changes and finish the review for this pull request.

JeffryBooher · 2013-08-25T03:30:26Z

@TomMalbran I tried this from an open document (using the keyboard to invoke the menu) and keys other than the ones that are expressly handled by the menu are going to the editor which isn't good. I then managed to get into a state with multiple popups showing because code hints popped up.

This doesn't happen when using the mouse to invoke the menu because focus is shifted to the drop down so I think you just need to set the document's active element to the popup.

…nt-projects

TomMalbran · 2013-08-25T06:24:58Z

@JeffryBooher I was able to fix the blur issue, was harder than expected since I couldn't focus almost any element, but bluing CodeMirror works. I added an extra bit to refocus CodeMirror if you close the menu using Escape, so you can open the menu, close it and keep coding. I also removed the toggle on Ctrl+Alt+R. Now it only opens the menu, and Esc closes it.

JeffryBooher · 2013-08-29T04:41:19Z

Nominating for Sprint 31

JeffryBooher · 2013-09-03T20:54:28Z

@TomMalbran almost there... I think the navigation keys (Left/Right/Home/End/PgUp/PgDn) are still going to code mirror. I tried vairous other keys and it seems that blocking those keys is all that remains.

peterflynn · 2013-09-03T22:16:25Z

Couple quick thoughts -- apologies if any of this already came up...

I think to fix the focus issue it's simplest to just explicitly focus the popup (I think you can render any DOM node focusable by just adding a tabIndex, even a negative one), rather than leaving the focus on CodeMirror and trying to prevent any key events from reaching it. I think this also means you could just use a regular key listener instead of the global key-hook mechanism.
In handleKeyEvent(), we should probably always focus the popup -- not just if an editor had focus. If focus was in a search field or something, we still don't want any key input to go into that text field...
Does anything bad happen if cleanupDropdown() just always re-focuses the editor? That would clean up the code because you wouldn't need all the stuff that plumbs the key event over from PopUpManager (which I worry just invites misuse by future pieces of code).

TomMalbran · 2013-09-03T23:42:45Z

@peterflynn Thanks.

I forgot about tabindex, which made the list focusable and I was able to do it.
Is there any advantage of using a regular key listener instead of the global key-hook mechanism? and which one is better to use?
It looks like everything works fine by always refocusing the editor.

@JeffryBooher Those navigation keys where used on the body, but now with the focus fix, those keys shouldn't run on the sidebar.

JeffryBooher · 2013-09-04T16:41:03Z

src/extensions/default/RecentProjects/main.js

-
-    var $dropdownToggle,
-        $dropdown;
+    var KeyboardPrefs = JSON.parse(require("text!keyboard.json"));


This isn't used anywhere -- can we remove it?

Is actually used in line 423 to assign it to the keybinding.

JeffryBooher · 2013-09-04T16:44:08Z

@TomMalbran the edit keys are fixed there is one comment on a recent change (unused var) that needs to be fixed.

njx · 2013-09-04T17:28:26Z

src/extensions/default/RecentProjects/main.js

+
+            showDropdown();
+            $dropdown.focus();
+			$dropdownItem = $dropdown.find("a").first();


Drive-by: looks like an indentation error here

Indent looks wrong on this line

We need to upgrade JSLint so that it starts shouting me about this stuff... hehe

Doesn't JSLint already complain about bad indentation normally? It sure does for me...

It doesn't do for me. I would have seen this error if it did complained.

peterflynn · 2013-09-04T20:05:42Z

@TomMalbran About the global key hook... I think it's intended for use in cases where you're trying to override someone else's global capture handler. Since that's not needed here, and the focus location is better now, I think it's simplest to use a standard DOM listener (jQuery, really).

The existing code you have up here, using the global key hook, seems like it will still work though. So if you'd rather get this merged now and do the cleanup later, that seems ok too.

TomMalbran · 2013-09-06T02:43:08Z

Fixed the indentation and keydown biding issues. I think is ready to merge now :)

peterflynn · 2013-09-06T18:20:52Z

@TomMalbran Looks good to me! Anything more needed @JeffryBooher?

Btw, just tested it out and I love it! It's soo nice being able to jump back to the previous project with just a quick "Ctrl+Alt+R, Enter" combo...

JeffryBooher · 2013-09-06T22:21:10Z

Looks good @peterflynn ... Merging! Thanks @TomMalbran!

Implementation of Navigate Recent Projects

Implementation of the navigation in the Recent Projects dropdown menu

cacacdc

ghost assigned JeffryBooher Jul 23, 2013

JeffryBooher reviewed Jul 24, 2013
View reviewed changes

Rename of a couple of functions

740d19d

JeffryBooher reviewed Jul 26, 2013
View reviewed changes

TomMalbran added 2 commits August 25, 2013 01:46

Merge remote-tracking branch 'upstream/master' into tom/navigate-rece…

20074ea

…nt-projects

Blur/Focus the editor when the menu opens

39d9427

Focus updates

9f73503

lkcampbell mentioned this pull request Sep 4, 2013

Remove shortcut keys lkcampbell/brackets-ruler#26

Closed

JeffryBooher reviewed Sep 4, 2013
View reviewed changes

njx reviewed Sep 4, 2013
View reviewed changes

Indentation and keydown fix

b35668b

JeffryBooher added a commit that referenced this pull request Sep 6, 2013

Merge pull request #4546 from TomMalbran/tom/navigate-recent-projects

cb7ea89

Implementation of Navigate Recent Projects

JeffryBooher merged commit cb7ea89 into adobe:master Sep 6, 2013

TomMalbran deleted the tom/navigate-recent-projects branch September 6, 2013 22:30

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Implementation of Navigate Recent Projects brackets-archive/bracketsIssues#4208

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Navigate Recent Projects #4546

Implementation of Navigate Recent Projects #4546

TomMalbran commented Jul 23, 2013

JeffryBooher Jul 24, 2013

TomMalbran Jul 24, 2013

JeffryBooher commented Jul 24, 2013

JeffryBooher Jul 26, 2013

adrocknaphobia commented Jul 29, 2013

TomMalbran commented Jul 29, 2013

TomMalbran commented Jul 29, 2013

JeffryBooher commented Aug 5, 2013

TomMalbran commented Aug 13, 2013

JeffryBooher commented Aug 25, 2013

JeffryBooher commented Aug 25, 2013

TomMalbran commented Aug 25, 2013

JeffryBooher commented Aug 29, 2013

JeffryBooher commented Sep 3, 2013

peterflynn commented Sep 3, 2013

TomMalbran commented Sep 3, 2013

JeffryBooher Sep 4, 2013

TomMalbran Sep 4, 2013

JeffryBooher commented Sep 4, 2013

njx Sep 4, 2013

peterflynn Sep 4, 2013

peterflynn Sep 4, 2013

TomMalbran Sep 4, 2013

peterflynn Sep 4, 2013

TomMalbran Sep 5, 2013

peterflynn commented Sep 4, 2013

TomMalbran commented Sep 6, 2013

peterflynn commented Sep 6, 2013

JeffryBooher commented Sep 6, 2013

Implementation of Navigate Recent Projects #4546

Implementation of Navigate Recent Projects #4546

Conversation

TomMalbran commented Jul 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffryBooher commented Jul 24, 2013

Choose a reason for hiding this comment

adrocknaphobia commented Jul 29, 2013

TomMalbran commented Jul 29, 2013

TomMalbran commented Jul 29, 2013

JeffryBooher commented Aug 5, 2013

TomMalbran commented Aug 13, 2013

JeffryBooher commented Aug 25, 2013

JeffryBooher commented Aug 25, 2013

TomMalbran commented Aug 25, 2013

JeffryBooher commented Aug 29, 2013

JeffryBooher commented Sep 3, 2013

peterflynn commented Sep 3, 2013

TomMalbran commented Sep 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffryBooher commented Sep 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Sep 4, 2013

TomMalbran commented Sep 6, 2013

peterflynn commented Sep 6, 2013

JeffryBooher commented Sep 6, 2013