927605 - Refactor keyboard-shortcut-to-text-string code into a JSM or otherwise reusable module

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

In bug 887853, I copied some code from devtools. That's sad. We should be refactoring that to make it easier to reuse it. The original code is here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/helpers.js

Paul, you seem to have been the only one to touch that file (unless it got moved or something?) - could we just stick this in a JSM in toolkit and have devtools use that, too, or is there some reason that's a bad idea? It seems like right now it uses the same module system as the add-on SDK (?) and I'm not sure if/how those are compatible, and/or if we could just refactor this bit of code to be usable as either, or...

Comment 1

[Paul Rouget [:paul]]

Maybe this should be moved to /addons-sdk/source/lib/sdk/ ?

Comment 2

[:Gijs (he/him)]

(In reply to Paul Rouget [:paul] from comment #1)
> Maybe this should be moved to /addons-sdk/source/lib/sdk/ ?

Sounds like a plan. Dave, can we just hg move stuff or do we need to go via github?

Comment 3

[Dão Gottwald [:dao]]

The obvious place for this would be /toolkit/modules/. Why should it live in the add-on SDK?

Comment 4

[Dave Townsend [:mossop]]

If you need to use it in a place that you don't have a commonjs module loader right now then it might be easiest to put this into a toolkit jsm instead.

Comment 5

[:Gijs (he/him)]

Attachment: refactor keyboard shortcut code in devtools into a JSM,

Alright, here's a patch to move this to toolkit/modules. I'll take naming suggestions for the JSM, I'm not particularly attached to this one but couldn't come up with anything better. I've tried to keep the devtools impact to a minimum.

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 819506 [details] [diff] [review]
refactor keyboard shortcut code in devtools into a JSM,

If I'm not mistaken, DevtoolsHelpers is only used to hold the prettyKey function.

It becomes a little bit strange to have DevtoolsHelpers.prettyKey just to redirect to prettyKeyboardShortcut.

I would either kill DevtoolsHelpers and only use prettyKeyboardShortcut, or at least use prettyKeyboardShortcut, not prettyShortcut.

Victor will know better.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 819506 [details] [diff] [review]
refactor keyboard shortcut code in devtools into a JSM,

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

Not directly related to this patch, but maybe take a look at bug 859081 as well.
r+ with comments addressed.

::: browser/devtools/debugger/debugger-controller.js
@@ +102,1 @@
>  Object.defineProperty(this, "DevtoolsHelpers", {

DevtoolsHelpers is now a leftover. Please remove it, there's no need for this wrapper and another getter.

::: browser/devtools/scratchpad/scratchpad.js
@@ +75,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "prettyKeyboardShortcut",
> +  "resource://gre/modules/PrettyKeyboardShortcut.jsm");
> +
> +Object.defineProperty(this, "DevtoolsHelpers", {

Ditto, right?

::: toolkit/modules/PrettyKeyboardShortcut.jsm
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["prettyKeyboardShortcut"];

Instead of creating a new file, maybe just move prettyKeyboardShortcut into ViewHelpers.jsm, as a member of the ViewHelpers object?

Comment 8

[:Gijs (he/him)]

Attachment: refactor keyboard shortcut code in devtools into a JSM,

Comments addressed, punting toolkit review to Blair.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 820204 [details] [diff] [review]
refactor keyboard shortcut code in devtools into a JSM,

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

::: toolkit/modules/PrettyKeyboardShortcut.jsm
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["prettyKeyboardShortcut"];

I generally try to avoid JSMs that export functions directly - the default behaviour ends up polluting the global scope when we inevitably add more functions. So how naming this ShortcutUtils.jsm, export a ShortcutUtils object, with a prettifyShortcut() function. Or something like that. I say ShortcutUtils, because there's a few candidate functions in the patch in bug 492557 that could go in here.

Comment 10

[:Gijs (he/him)]

Attachment: refactor keyboard shortcut code in devtools into a JSM,

Alright, done.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 820211 [details] [diff] [review]
refactor keyboard shortcut code in devtools into a JSM,

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

Those extra changes made me take a closer took at the code... sorry.

::: toolkit/modules/ShortcutUtils.jsm
@@ +21,5 @@
> +    *
> +    * @param Node aElemKey
> +    *        The key element to get the modifiers from.
> +    * @param boolean aAllowCloverleaf
> +    *        Pass true to use the cloverleaf symbol instead of a descriptive string.

Nit: Mention that this argument is OSX-only and temporary.

@@ +66,4 @@
>        elemString += PlatformKeys.GetStringFromName("VK_CONTROL") +
>          PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
>      }
> +    if (elemMod.match("meta")) {

Should also be handling "os" (ie, the windows key). See https://developer.mozilla.org/en-US/docs/XUL/Attribute/modifiers

@@ +71,5 @@
>          PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
>      }
> +
> +    return elemString +
> +      (aElemKey.getAttribute("keycode").replace(/^.*VK_/, "") ||

What if I told you... this code had been doing this wrong the whole time?

keys.properties holds the localized strings for all other VK_* keycodes, we need to use that.

Comment 12

[:Gijs (he/him)]

Attachment: refactor keyboard shortcut code in devtools into a JSM,

This should have the comments addressed. :-)

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 820240 [details] [diff] [review]
refactor keyboard shortcut code in devtools into a JSM,

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

::: toolkit/modules/ShortcutUtils.jsm
@@ +58,5 @@
> +        elemString += PlatformKeys.GetStringFromName("VK_ALT") +
> +          PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
> +      }
> +    }
> +    if (elemMod.match("os") && Services.appinfo.OS == "WINNT") {

Don't filter based on OS here - technically it's supported on Linux too. And it just won't ever work on OSX apparently, so don't even bother filtering that out.

Comment 14

[:Gijs (he/him)]

With nits: https://hg.mozilla.org/integration/fx-team/rev/73bebd77c3ed

Comment 15

[:Gijs (he/him)]

Attachment: UX-only: use the ShortcutUtils module in CustomizableUI,

Once this merges through m-c to UX, we should also use it.

Comment 16

[:Gijs (he/him)]

Attachment: invert the cloverleaf flag,

Mike and I were talking about how this parameter is quite unintuitive. So In fact, it's so unintuitive the test that checks the debugger's tooltip passes a different value than the debugger, so if you were to use a cmd key for the debugger shortcut, the test would break. Let's make this more sensible.

Comment 17

[:Gijs (he/him)]

Attachment: UX-only: use the ShortcutUtils module in CustomizableUI,

And then we can update this patch, too.

Comment 18

[Victor Porof [:vporof][:vp]]

Comment on attachment 820301 [details] [diff] [review]
invert the cloverleaf flag,

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

r+ assuming this functions the same way.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 820302 [details] [diff] [review]
UX-only: use the ShortcutUtils module in CustomizableUI,

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

LGTM! Nice improvement, worth the trouble!

Comment 20

[:Gijs (he/him)]

Comment on attachment 820301 [details] [diff] [review]
invert the cloverleaf flag,

(In reply to Victor Porof [:vp] from comment #18)
> Comment on attachment 820301 [details] [diff] [review]
> invert the cloverleaf flag,
> 
> Review of attachment 820301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming this functions the same way.

https://hg.mozilla.org/integration/fx-team/rev/5945380e8d1d

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/73bebd77c3ed
https://hg.mozilla.org/mozilla-central/rev/5945380e8d1d

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/0745d47f1cbb

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/0745d47f1cbb

Comment 24

[Wladimir Palant]

Added dev-doc-needed keyword: ShortcutUtils.jsm module (http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm) has been added here, this functionality is helpful for add-on authors as well.