730898 - Reuse editMenuKeys in scratchpad.xul

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P4)

Description

[Mihai Sucan [:msucan]]

In scratchpad.xul we redefine a number of Edit-related keybindings that we could reuse from the edit menu overlay. See bug 684445 comment 20.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

The description for this bug/change request is a little vague, but the attached may serve as a start point. Please let me know if this is along the lines of what you had in mind ...

Summarizing ...

I added context menu keys, and incorporated them into their context menuitems in editmenuoverlay.xul

I then used those items for cut/copy/paste/selectall in the Edit Menu dropdown, and for cut/copy/paste/selectall/delete in the Context Menu pop-up in scratchpad.xul

Finally, I removed (now) unused items for cut/copy/paste/selectall in scratchpad.dtd

--mark

Comment 2

[Mihai Sucan [:msucan]]

Mark: that's great! Thanks for taking the patch!

Yep, the description is a bit vague, indeed - my bad!

Patch looks great, but please limit the changes to scratchpad.xul and the dtd only - you do not need to change the edit menu overlay.

What we intend to fix with this bug is to only make better use of the existing editMenuKeys from the editMenuOverlay.

Thanks again!

Comment 3

[Mark Capella [:capella]]

I got into the editMenuOverlay, as the context items for cut/copy/paste/delete wont display access keys on the right side, and kinda just expanded on the idea.

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Mark Capella [:capella] from comment #3)
> I got into the editMenuOverlay, as the context items for
> cut/copy/paste/delete wont display access keys on the right side, and kinda
> just expanded on the idea.

Yeah. You can fix that in scratchpad.xul. Change the context menu items: add key="key_foo".

Comment 5

[Mark Capella [:capella]]

I must be missing something ... I tried (for example) and get runtime errors ...

     <menuitem id="cMenu_cut" key="key_cut"/>
----------------------------------^

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

I spoke too soon and found the problem ...

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 602703 [details] [diff] [review]
Patch (v2)

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

Thanks for your patch Mark! This is great work.

Please do the same for the find keys. We should use the editMenuOverlay keys for those cases as well.

For key_undo and key_redo we need to copy them from editMenyOverlay.xul to source-editor-overlay.xul. Rename the ids to se-key-undo/se-key-redo and associate them to se-cmd-undo/se-cmd-redo. Update the scratchpad.xul file accordingly.

(this patch is really about cleaning up code!)

Comment 8

[Mihai Sucan [:msucan]]

Mark: for subsequent patch updates please ask for feedback or review from me - set the patch flags accordingly.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v3)

Here's the next incremental ... in addition to the previous changes, it does the "find" key changes you asked for ...

Note that I had to modify editMenuOverlay.xul to get the correct key mapping for "findagain" ...

Also note that the "key_findAgain2" and "key_findPrevious2" keys could be removed there as they are unused, and we could then go further and remove the "findAgainCmd.key2" entity from editMenuOverlay.dtd, which they are the sole consumers of ...

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 603328 [details] [diff] [review]
Patch (v3)

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

Thanks for your updated patch Mark! This is great!


(In reply to Mark Capella [:capella] from comment #9)
> Created attachment 603328 [details] [diff] [review]
> Patch (v3)
> 
> Here's the next incremental ... in addition to the previous changes, it does
> the "find" key changes you asked for ...

Awesome!


> Note that I had to modify editMenuOverlay.xul to get the correct key mapping
> for "findagain" ...

That's not needed, because key_findAgain2 has VK_F3 for Macs. Please do not change editMenuOverlay.xul.


> Also note that the "key_findAgain2" and "key_findPrevious2" keys could be
> removed there as they are unused, and we could then go further and remove
> the "findAgainCmd.key2" entity from editMenuOverlay.dtd, which they are the
> sole consumers of ...

They are not associated to other commands, buttons or menu items, but their existence is relevant. Those keyboard shortcuts work. I'm glad you did not remove them in your patch.


Please also address my request in comment 7 related to key_undo/key_redo, then we can land the patch. Thanks a lot!

Comment 11

[Mark Capella [:capella]]

Ok, I can remove the code from editMenuOverlay.xul ... but on my Win machine that causes "Find Again" to display CTRL-G and map to the CTRL-G key ...

It might be time for me to do a clobber build, as I just also noticed that my versions REDO key displays CTRL-Y but maps to the CTRL-SHIFT-Z key ...

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Mark Capella [:capella] from comment #11)
> Ok, I can remove the code from editMenuOverlay.xul ... but on my Win machine
> that causes "Find Again" to display CTRL-G and map to the CTRL-G key ...

Yeah, well it should display the same as in the Edit menu of Firefox. We are now making Scratchpad consistent with the rest of Firefox.

On Linux I get Ctrl-G as well. This is not broken - F3 also works.

> It might be time for me to do a clobber build, as I just also noticed that
> my versions REDO key displays CTRL-Y but maps to the CTRL-SHIFT-Z key ...

Yeah, no worries. Looking forward for the updated patch. Don't forget about comment 7 - the key_undo/key_redo changes! ;)

Comment 13

[Mark Capella [:capella]]

Attachment: [in-fx-team] Patch (v4)

Ok ... crosses fingers ... this seems to work :-)

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 603494 [details] [diff] [review]
[in-fx-team] Patch (v4)

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

This is looking good Mark! Great work!

Thanks a lot for your patch! I am currently working on bug 731394 and I will land these two patches together when I am also ready.

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 603494 [details] [diff] [review]
[in-fx-team] Patch (v4)

Landed:
https://hg.mozilla.org/integration/fx-team/rev/82531ecd89f1

Thanks Mark for your contribution!

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/82531ecd89f1