714832 - [Linux] the redo command should also have the ctrl-y shortcut

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Comment 1

[Paul Rouget [:paul]]

Mihai. good first bug?

Comment 2

[Mihai Sucan [:msucan]]

Yes, this is a good first bug.

Comment 3

[Paul Rouget [:paul]]

I was wrong. The right binding is ctrl-shift-z. See: http://developer.gnome.org/hig-book/3.0/input-keyboard.html.en#shortcuts

But ctrl-y is used in a lot different places. I would like to have it too.

Comment 4

[Mihai Sucan [:msucan]]

Paul: I hope it is fine if I lower the priority of the bug (if not, feel free to bump it up again). I also updated the bug summary to reflect what we want now.

Comment 5

[CarpeDiem]

Attachment: Default key bindings in orion editor: Redo , (ctrl-shift-z or ctrl-y).

Comment 6

[Paul Rouget [:paul]]

Thanks for the patch :)

You need to set the feedback flag to "?", and then, you write the name of someone in the next field. In this case "msucan". msucan will then take a look, and switch the flag to + or - depending on his feedback.

Comment 7

[Paul Rouget [:paul]]

(oh, and you forgot to add your name in the Contributor List at the beginning of the file)

Comment 8

[CarpeDiem]

Attachment: Bug 714832 - Redo command should also have the ctrl-y shortcut

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 590420 [details] [diff] [review]
Bug 714832 - Redo command should also have the ctrl-y shortcut

Review of attachment 590420 [details] [diff] [review]:
-----------------------------------------------------------------

This is working really well! Thanks for your patch Shreya!

We want this keyboard shortcut to only be used on Linux and Windows machines, not on Macs. Can you please add the shortcut only if Services.appinfo.OS == "WINNT" or "Linux"? Thanks!

Once you make this minor changes, please look into adding a new test for the Source Editor that checks the keyboard shortcut works. See the following on how to write browser chrome tests:

https://developer.mozilla.org/en/Browser_chrome_tests

It also explains how you can run the existing tests. The Source Editor tests are found in browser/devtools/sourceeditor/test/.

Again, thank you very much for your contribution! Looking forward for the updated patch!

Comment 10

[Paul Rouget [:paul]]

CarpeDiem, do you need some help to address Mihai's comments?

Comment 11

[Thaddee Tyl [:espadrine]]

Attachment: Redo command Linux/Windows-only, with mochitests.

This patch has been tested on mac and linux, not on windows,
although I would assume that it works there, too.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 631556 [details] [diff] [review]
Redo command Linux/Windows-only, with mochitests.

Review of attachment 631556 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, Thaddee! Thanks for your patch!

A couple of comments below. You have r+ with these changes.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +133,5 @@
>  
> +// On Windows and Linux, the Ctrl-Y key is used to perform the redo command.
> +
> +if (Services.appinfo.OS === "WINNT" ||
> +    Services.appinfo.OS === "Linux") {

Nit: please use == to match the coding style of the file you work with.

::: browser/devtools/sourceeditor/test/browser_bug684546_reset_undo.js
@@ +78,5 @@
> +      is(editor.getText(), "",
> +         "CTRL+Y doesn't undo on machines other than Linux and Windows");
> +    }
> +  }, true);
> +  document.body.dispatchEvent(evt);

You can replace all of this code with a single call to EventUtils.synthesizeKey() and then getText() to see if it is what you expect. See the _initialization.js test file to see an example of how synthesizeKey() is used.

Comment 13

[Thaddee Tyl [:espadrine]]

Attachment: CTRL+Y Redo command Linux/Windows only, with mochitest.

I added the test to `browser_sourceeditor_initialization.js` instead.

The only part that differs from last patch is the mochitest,
not the actual code.

Comment 14

[Thaddee Tyl [:espadrine]]

Attachment: CTRL+Y Redo command Linux/Windows only, with mochitest.

- CTRL+Y does *redo*
- removed the import of services
- === → ==
- using VK_Y for conformity

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 632011 [details] [diff] [review]
CTRL+Y Redo command Linux/Windows only, with mochitest.

Review of attachment 632011 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thank you!

Comment 16

[Thaddee Tyl [:espadrine]]

Attachment: [in-fx-team] CTRL+Y Redo command Linux/Windows only, with mochitest.

Still no change to the “real” code.

I re-establish the editor's state that was acquired before my test, after it has been run.

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 633269 [details] [diff] [review]
[in-fx-team] CTRL+Y Redo command Linux/Windows only, with mochitest.

Thanks for the update!

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 633269 [details] [diff] [review]
[in-fx-team] CTRL+Y Redo command Linux/Windows only, with mochitest.

Landed:
https://hg.mozilla.org/integration/fx-team/rev/0c030b4edd8a

Thanks Thaddee!

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0c030b4edd8a