Closed Bug 927605 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [Australis:P4][Australis:M9])

Attachments

(3 files, 4 obsolete files)

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...
Flags: needinfo?
Flags: needinfo?
Flags: needinfo?(paul)
Maybe this should be moved to /addons-sdk/source/lib/sdk/ ?
Flags: needinfo?(paul)
(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?
Flags: needinfo?(dtownsend+bugmail)
The obvious place for this would be /toolkit/modules/. Why should it live in the add-on SDK?
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.
Flags: needinfo?(dtownsend+bugmail)
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.
Attachment #819506 - Flags: review?(paul)
Attachment #819506 - Flags: review?(dao)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #819506 - Flags: review?(paul) → review?(vporof)
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?
Attachment #819506 - Flags: review?(vporof) → review+
Comments addressed, punting toolkit review to Blair.
Attachment #820204 - Flags: review?(bmcbride)
Attachment #819506 - Attachment is obsolete: true
Attachment #819506 - Flags: review?(dao)
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.
Attachment #820204 - Flags: review?(bmcbride) → review-
Alright, done.
Attachment #820211 - Flags: review?(bmcbride)
Attachment #820204 - Attachment is obsolete: true
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.
Attachment #820211 - Flags: review?(bmcbride) → review-
This should have the comments addressed. :-)
Attachment #820240 - Flags: review?(bmcbride)
Attachment #820211 - Attachment is obsolete: true
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.
Attachment #820240 - Flags: review?(bmcbride) → review+
With nits: https://hg.mozilla.org/integration/fx-team/rev/73bebd77c3ed
Whiteboard: [Australis:P4][Australis:M?] → [Australis:P4][Australis:M?][fixed-in-fx-team][leave-open]
Attachment #820240 - Flags: checkin+
Once this merges through m-c to UX, we should also use it.
Attachment #820293 - Flags: review?(mdeboer)
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.
Attachment #820301 - Flags: review?(vporof)
And then we can update this patch, too.
Attachment #820302 - Flags: review?(mdeboer)
Attachment #820293 - Attachment is obsolete: true
Attachment #820293 - Flags: review?(mdeboer)
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.
Attachment #820301 - Flags: review?(vporof) → review+
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!
Attachment #820302 - Flags: review?(mdeboer) → review+
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
Attachment #820301 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/73bebd77c3ed
https://hg.mozilla.org/mozilla-central/rev/5945380e8d1d
Whiteboard: [Australis:P4][Australis:M?][fixed-in-fx-team][leave-open] → [Australis:P4][Australis:M?][leave-open]
https://hg.mozilla.org/projects/ux/rev/0745d47f1cbb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][Australis:M?][leave-open] → [Australis:P4][Australis:M9][fixed-in-ux]
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/0745d47f1cbb
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux] → [Australis:P4][Australis:M9]
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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.