756423 - Make mention of the keyboard shortcuts in the comment on Scratchpad

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Kohei Yoshino [:kohei]]

The summary says. Developers prefer to use keyboard shortcuts, so...

/*
 * This is a JavaScript Scratchpad.
 *
 * Enter some JavaScript, then right click, choose from the Execute Menu,
 * or use the keyboard shortcut:
 * 1. Run to evaluate the selected text (Ctrl+R)
 * 2. Inspect to bring up an Object Inspector on the result (Ctrl+I)
 * 3. Display to insert the result in a comment after the selection (Ctrl+L)
 */

The comment should be different on Mac because, as you know, Ctrl is Command on the platform.

(This is a feedback from a Twitter user)

Comment 1

[Paul Rouget [:paul]]

Indeed. Here is an example of how to translate a <key> shortcut to a string:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#289

Comment 2

[Rob Campbell [:rc] (:robcee)]

adding this to the good first bugs train. Thanks for filing!

Comment 3

[Abhishek Potnis [:avp]]

Hi Rob,
I am interested in working on this bug. I am a beginner so could you please guide me on getting started with this bug.

Thanks.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

Still unassigned, so I'll take a stab here.

This patch works locally on my WIN machine. I tried a test of the Mac version (with a hardcode override of the SP_Pretty_Key function). But instead of displaying clover-foo the test displays meta-foo...? Maybe I can't see / prove it without an actual MAC machine displaying it.

I'm also still trying to verify the change to browser/devtools/scratchpad/test/browser_scratchpad_open.js ... I haven't had to run a standalone .js mochitest before, so it might be quicker to ask for help here if I get stuck trying to figure it out.

Comment 5

[Josh Matthews [:jdm]]

TEST_PATH=browser/devtools/scratchpad/test/browser_scratchpad_open.js make -C objdir mochitest-browser-chrome is probably the command you want. The TEST_PATH might need some fiddling.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

Thanks for the tip! This executes better / cleaner. I couldn't check for the initial string to be something particular (as before) since now we create it on the fly. I had to settle for a test that verifies *something* is returned from the getText() string, other than *null*.

Comment 7

[Rob Campbell [:rc] (:robcee)]

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

Hey Mark. Thanks for the patch. :)

-    content/browser/scratchpad.js                 (scratchpad/scratchpad.js)
+*   content/browser/scratchpad.js                 (scratchpad/scratchpad.js)

I think you can do this without preprocessing. I was thinking of a slightly different, less dynamic approach than what you've done.

It is this:

1. Continue to use the intro text from the scratchpad.properties file.
2. Add %1$S, %2$S and %3$S to the intro text in the scratchpad.properties file.

e.g.,

# LOCALIZATION NOTE  (scratchpadIntro): This is a multi-line comment explaining
# how to use the Scratchpad. Note that this should be a valid JavaScript
# comment inside /* and */.
scratchpadIntro=/*\n * This is a JavaScript Scratchpad.\n *\n * Enter some JavaScript, then Right Click or choose from the Execute Menu:\n * 1. Run to evaluate the selected text (%1$S),\n * 2. Inspect to bring up an Object Inspector on the result (%2$S), or,\n * 3. Display to insert the result in a comment after the selection. (%3$S)\n */\n\n

3. Use StringBundle's getFormattedString("scratchpadIntro", [key1name, key2name, key3name]);

where keyNname is a string created using the platformKeys bundle. Rather than using preprocessing, I'd probably look at window.navigator.platform to determine which OS I'm running in.

Let me know if this doesn't make sense or you need any help with any of this.

thanks!

Comment 8

[Rob Campbell [:rc] (:robcee)]

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

cancelling review request based on previous feedback.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v3)

Boy, this a quite a bit tighter with your suggestions and a few more tweaks I added. 

btw .getFormattedString() gave me a hassle, and I wound up using .formatStringFromName() instead.

Comment 10

[Dão Gottwald [:dao]]

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

>-scratchpadIntro=/*\n * ...
>+scratchpadIntro=/*\n * ...

You need to change the string name. A comment explaining the placeholders is needed too.

Comment 11

[Mark Capella [:capella]]

I'm not sure why / how the name of the string var should be changed. It seems the basic function is still the same, just additional information added to it. Maybe something like scratchpadInstrs ?

For the comment re: placeholders something like this maybe ?

 # LOCALIZATION NOTE  (scratchpadIntro): This is a multi-line comment explaining
 # how to use the Scratchpad. Note that this should be a valid JavaScript
 # comment inside /* and */. Placeholders are used to provide textual feedback
 # regarding the platform specific keyboard shortcuts for each numbered item.

Comment 12

[Rob Campbell [:rc] (:robcee)]

(In reply to Mark Capella [:capella] from comment #11)
> I'm not sure why / how the name of the string var should be changed. It
> seems the basic function is still the same, just additional information
> added to it. Maybe something like scratchpadInstrs ?
> 
> For the comment re: placeholders something like this maybe ?
> 
>  # LOCALIZATION NOTE  (scratchpadIntro): This is a multi-line comment
> explaining
>  # how to use the Scratchpad. Note that this should be a valid JavaScript
>  # comment inside /* and */. Placeholders are used to provide textual
> feedback
>  # regarding the platform specific keyboard shortcuts for each numbered item.

it's a "feature" of our localization tools. If we don't change the label when a string is updated, the tools can miss the change and it won't get updated in other locales.

I'll review, this patch but can either update the string name for you or if you're feeling super-motivated you can. We usually just tack a number onto the end.

thanks for that, Dão.

Comment 13

[Rob Campbell [:rc] (:robcee)]

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

looks good, Mark! I'll update the string name and land when I've put this through some testing.

Comment 14

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/4f39921f782f

Comment 15

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/86c5e4ecba1e

oops.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4f39921f782f
https://hg.mozilla.org/mozilla-central/rev/86c5e4ecba1e