search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

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

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by mrplants Tuesday Nov 12, 2013 at 23:35 GMT Originally opened as https://github.com/adobe/brackets/pull/5969

Submitting fix to bug: https://github.com/adobe/brackets/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.