Last Comment Bug 756423 - Make mention of the keyboard shortcuts in the comment on Scratchpad
: Make mention of the keyboard shortcuts in the comment on Scratchpad
Status: RESOLVED FIXED
[good first bug][mentor=robcee]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on:
Blocks: 771151
  Show dependency treegraph
 
Reported: 2012-05-18 04:58 PDT by Kohei Yoshino [:kohei]
Modified: 2012-08-01 01:17 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (8.51 KB, patch)
2012-07-28 09:23 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (8.52 KB, patch)
2012-07-28 12:44 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (6.05 KB, patch)
2012-07-30 17:39 PDT, Mark Capella [:capella]
rcampbell: review+
Details | Diff | Splinter Review

Description Kohei Yoshino [:kohei] 2012-05-18 04:58:59 PDT
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] 2012-05-18 05:20:25 PDT
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) 2012-05-21 04:07:03 PDT
adding this to the good first bugs train. Thanks for filing!
Comment 3 Abhishek Potnis [:avp] 2012-07-20 07:16:31 PDT
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] 2012-07-28 09:23:43 PDT
Created attachment 646866 [details] [diff] [review]
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] 2012-07-28 10:25:40 PDT
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] 2012-07-28 12:44:17 PDT
Created attachment 646897 [details] [diff] [review]
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) 2012-07-30 10:39:34 PDT
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) 2012-07-30 10:39:58 PDT
Comment on attachment 646897 [details] [diff] [review]
Patch (v2)

cancelling review request based on previous feedback.
Comment 9 Mark Capella [:capella] 2012-07-30 17:39:44 PDT
Created attachment 647372 [details] [diff] [review]
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] 2012-07-31 00:56:44 PDT
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] 2012-07-31 03:52:35 PDT
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) 2012-07-31 11:15:07 PDT
(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) 2012-07-31 11:20:56 PDT
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) 2012-07-31 11:31:39 PDT
https://hg.mozilla.org/integration/fx-team/rev/4f39921f782f
Comment 15 Rob Campbell [:rc] (:robcee) 2012-07-31 15:26:18 PDT
https://hg.mozilla.org/integration/fx-team/rev/86c5e4ecba1e

oops.

Note You need to log in before you can comment on or make changes to this bug.