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

[core-ai-bot]

Issue by mrplants
Tuesday Nov 12, 2013 at 23:35 GMT
Originally opened as adobe/brackets#5969

---

Submitting fix to bug: adobe/brackets#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 included the following code: https://github.com/adobe/brackets/pull/5969/commits

[core-ai-bot]

Comment by mrplants
Tuesday Nov 12, 2013 at 23:58 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Wednesday Nov 13, 2013 at 05:20 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Wednesday Nov 13, 2013 at 05:27 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Wednesday Nov 13, 2013 at 05:29 GMT

---

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.

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 13, 2013 at 14:22 GMT

---

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

[core-ai-bot]

Comment by mrplants
Wednesday Nov 13, 2013 at 14:26 GMT

---

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

[core-ai-bot]

Comment by mrplants
Wednesday Nov 13, 2013 at 15:25 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 13, 2013 at 17:34 GMT

---

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.

[core-ai-bot]

Comment by mrplants
Thursday Nov 14, 2013 at 20:22 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Thursday Nov 14, 2013 at 20:36 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Thursday Nov 14, 2013 at 21:13 GMT

---

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

[core-ai-bot]

Comment by mrplants
Sunday Nov 17, 2013 at 20:58 GMT

---

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

[core-ai-bot]

Comment by redmunds
Monday Nov 18, 2013 at 17:02 GMT

---

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

[core-ai-bot]

Comment by redmunds
Monday Nov 18, 2013 at 17:06 GMT

---

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.

[core-ai-bot]

Comment by redmunds
Monday Nov 18, 2013 at 17:07 GMT

---

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

[core-ai-bot]

Comment by redmunds
Monday Nov 18, 2013 at 17:12 GMT

---

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.

[core-ai-bot]

Comment by peterflynn
Tuesday Dec 03, 2013 at 01:56 GMT

---

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?

[core-ai-bot]

Comment by mrplants
Thursday Dec 05, 2013 at 03:18 GMT

---

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