Added ESC, Y, A, N, and S as keyboard shortcuts to the Replace and Find ...

[mrplants]

Submitting fix to bug: #5005. ESC, Y, A, N, and S keyboard shortcuts were added to the Replace and Find function. No deletion of code was necessary. I simply attached a handler to the 'modalBar' object that watched for key events and performed the necessary functions fi the above keyCodes were realized.

[mrplants]

I replaced the == with === in order to pass the Travis CI test.

[peterflynn]

Adding a couple drive-by comments... also @mrplants, we need you to sign the contributor agreement before this can be merged.

[peterflynn]

Should use the KeyEvent constants here

[peterflynn]

Also, looks like this pull request adds an empty file called "Run"... can you remove that from the diff?

[peterflynn]

One last point: I wondered whether the mnemonics Y/N/S/A should be localized since the buttons will have different labels in other locales. But we don't do that for any other shortcuts, and it's already covered by the shortcut localization story -- so IMHO no need to worry about it here.

[marcelgerber]

@peterflynn Maybe we should just pick the first char out of the string, for example in german Ja is Yes, which means using J.

[mrplants]

@SAplayer How do we map the first letter to a KeyEvent value? What if it's Chinese? String parsing would be difficult in that case I assume.

[mrplants]

@peterflynn I made the changes and signed the agreement. Thanks for the help.

@SAplayer:
Maybe instead of (Y), it could be (ENTER), and instead of (N), it could be (RIGHT ARROW / LEFT ARROW). And (S) doesn't really need to be there because (ESCAPE) quits it anyways. (SPACEBAR or something else...) could be the replace-all command.

So the keyboard commands would be:
ENTER - replace the currently selected item
RIGHT_ARROW / DOWN_ARROW - go to the closest match after current
LEFT_ARROW / UP_ARROW - go to the closest match before current
SPACE_BAR - replace-all mode
ESCAPE - end the find & replace mode

This way, it can be language-agnostic. Let me know what you think and I'll implement it.

[marcelgerber]

I realized this wasn't a good idea, too. We had to implement special cases like if two strings began with the same char.
Your idea isn't bad, but we definitely shouldn't use ENTER. Enter is handled on tabbing, so normally it does replace the current match, but after tabbing it skips the current match.

@peterflynn Maybe we could add the shortcut key to the modalbar, so german would be Ja (Y) for example.

[TomMalbran]

Using a switch would make this code block cleaner. And you don't really need the first If, if you are then handling each keyCode in it's own if.

[mrplants]

Are there any other problems with this Pull Request that I should fix?

[TomMalbran]

Yes. Please use JSLint when coding for Brackets. If you code in Brackets just enable it to check the code. Right now there is an error with the switch, which should be switch (e.keyCode) {, and you don't need the last default statement in the switch. You will also avoid most of this Travis failures checking the code with JSLint.

[redmunds]

@mrplants We're currently trying to close down Sprint 34, so we're not merging any pull requests until that is done. I'll take a final look probably next week.

[mrplants]

I found a JSLinter and fixed my code. Please advise for any other changes when you review the pull request again.

[redmunds]

@mrplants This is a nice improvement!

This code doesn't quite follow the existing flow of code. I can see 2 ways (so far) where this causes bugs:

1. Type some text in page to replace, type in replacement text, then keep pressing "Y" until last instance of text has been replace. The modal bar is not dismissed after last replace. Same operation clicking "Y" button dismisses modal bar when done.
2. If I click the "All..." button, the modal bar is dismissed, but if I press "A" key, it is not dismissed.

Look at the code in the modalBar.getRoot().on("click", function (e) { above. Notice how it closes the modal bar on every click. The normal code processing will re-invoke the modal bar when required. Your code should do the same. Also notice the "replace-stop" case that destroys modalBar (that has already been closed).

[redmunds]

The Escape and Stop cases are doing the same thing:

        case KeyEvent.DOM_VK_ESCAPE:
            modalBar.close();
            break;
        case KeyEvent.DOM_VK_S:
            modalBar.close();
            break;

So they can be consolidated:

        case KeyEvent.DOM_VK_ESCAPE:
        case KeyEvent.DOM_VK_S:
            modalBar.close();
            break;

Note that this code will likely change due to my previous comment, but these 2 cases will probably still be the same and can still be consolidated.

[redmunds]

Done with review. Let me know when you push fixes to your branch and I'll review again.

[redmunds]

Note that it's best practice to create a branch for your changes. This way you can update your master branch with brackets repo updates, etc. without affecting your pull request. This also allows you to have multiple pull requests.

[peterflynn]

Closing since this is going to be invalidated by the find/replace UI cleanup user story within the next day or two. I'll see if I can work the keyboard handling into that change. If not, @mrplants would you be willing to set up a new pull request based on that newer code after it lands?

[mrplants]

@peterflynn Definitely. I would love to take responsibility for that. Let me know when I should pull and I'll make the necessary changes to add keyboard handling.

On a side note, I'm a student and I have an assignment to improve a data structure in an open source project. Do you think that Brackets has this? I'm looking for a project with the following restrictions:

The improvement will increase efficiency or speed
It is based on a data structure
It is relatively beginner coding. I'm not an expert at JS quite yet.

Any ideas? I realize that the assignment is kind of useless and out of scope, but if you could find something like this for me, I would really appreciate it.