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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: paul, Assigned: espadrine)

Tracking

Trunk
Firefox 16
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

6 years ago
Summary: [orion] On Linux (gtk), the control shortcut should be ctrl-y, not ctrl-shift-z. → [orion] On Linux (gtk), the redo shortcut should be ctrl-y, not ctrl-shift-z.

Updated

6 years ago
Whiteboard: [sourceeditor][orion]
(Reporter)

Comment 1

5 years ago
Mihai. good first bug?
Priority: -- → P2
Yes, this is a good first bug.
Whiteboard: [sourceeditor][orion] → [sourceeditor][orion][good first bug][mentor=msucan][lang=js]
(Reporter)

Comment 3

5 years ago
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.
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.
OS: All → Linux
Priority: P2 → P3
Hardware: x86 → All
Summary: [orion] On Linux (gtk), the redo shortcut should be ctrl-y, not ctrl-shift-z. → [Linux] the redo command should also have the ctrl-y shortcut

Comment 5

5 years ago
Created attachment 590414 [details] [diff] [review]
Default key bindings in orion editor: Redo , (ctrl-shift-z or ctrl-y).

Updated

5 years ago
Attachment #590414 - Flags: feedback+

Updated

5 years ago
Attachment #590414 - Flags: feedback+
(Reporter)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

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

Updated

5 years ago
Assignee: nobody → kawaii.imouto

Updated

5 years ago
Attachment #590414 - Flags: feedback?(mihai.sucan)

Comment 8

5 years ago
Created attachment 590420 [details] [diff] [review]
Bug 714832 - Redo command should also have the ctrl-y shortcut
Attachment #590414 - Attachment is obsolete: true
Attachment #590414 - Flags: feedback?(mihai.sucan)

Updated

5 years ago
Attachment #590420 - Attachment is patch: true
Attachment #590420 - Flags: feedback?(mihai.sucan)
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!
Attachment #590420 - Flags: feedback?(mihai.sucan) → feedback+
(Reporter)

Comment 10

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

Updated

5 years ago
Assignee: kawaii.imouto → mihai.sucan
Status: NEW → ASSIGNED
Depends on: 759351
Whiteboard: [sourceeditor][orion][good first bug][mentor=msucan][lang=js] → [sourceeditor]
(Assignee)

Comment 11

5 years ago
Created attachment 631556 [details] [diff] [review]
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.
Assignee: mihai.sucan → thaddee.tyl
Attachment #590420 - Attachment is obsolete: true
Attachment #631556 - Flags: review?(mihai.sucan)
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.
Attachment #631556 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 632008 [details] [diff] [review]
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.
Attachment #631556 - Attachment is obsolete: true
Attachment #632008 - Flags: review?(mihai.sucan)
(Assignee)

Comment 14

5 years ago
Created attachment 632011 [details] [diff] [review]
CTRL+Y Redo command Linux/Windows only, with mochitest.

- CTRL+Y does *redo*
- removed the import of services
- === → ==
- using VK_Y for conformity
Attachment #632008 - Attachment is obsolete: true
Attachment #632008 - Flags: review?(mihai.sucan)
Attachment #632011 - Flags: review?(mihai.sucan)
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!
Attachment #632011 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 633269 [details] [diff] [review]
[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.
Attachment #632011 - Attachment is obsolete: true
Attachment #633269 - Flags: review?(mihai.sucan)
Comment on attachment 633269 [details] [diff] [review]
[in-fx-team] CTRL+Y Redo command Linux/Windows only, with mochitest.

Thanks for the update!
Attachment #633269 - Flags: review?(mihai.sucan) → review+
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!
Attachment #633269 - Attachment description: CTRL+Y Redo command Linux/Windows only, with mochitest. → [in-fx-team] CTRL+Y Redo command Linux/Windows only, with mochitest.

Updated

5 years ago
No longer depends on: 759351
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0c030b4edd8a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Whiteboard: [sourceeditor][fixed-in-fx-team] → [sourceeditor]
You need to log in before you can comment on or make changes to this bug.