Make mention of the keyboard shortcuts in the comment on Scratchpad

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Scratchpad
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kohei, Assigned: capella)

Tracking

unspecified
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=robcee])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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
adding this to the good first bugs train. Thanks for filing!
Whiteboard: [good-first-bugs][mentor=robcee]

Updated

5 years ago
Whiteboard: [good-first-bugs][mentor=robcee] → [good first bug][mentor=robcee]

Updated

5 years ago
Blocks: 771151
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.
(Assignee)

Comment 4

5 years ago
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.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #646866 - Flags: review?(rcampbell)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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*.
Attachment #646866 - Attachment is obsolete: true
Attachment #646866 - Flags: review?(rcampbell)
Attachment #646897 - Flags: review?(rcampbell)
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 on attachment 646897 [details] [diff] [review]
Patch (v2)

cancelling review request based on previous feedback.
Attachment #646897 - Flags: review?(rcampbell)
(Assignee)

Comment 9

5 years ago
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.
Attachment #646897 - Attachment is obsolete: true
Attachment #647372 - Flags: review?(rcampbell)
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.
(Assignee)

Comment 11

5 years ago
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.
(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 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.
Attachment #647372 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/integration/fx-team/rev/4f39921f782f
Whiteboard: [good first bug][mentor=robcee] → [good first bug][mentor=robcee][fixed-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/86c5e4ecba1e

oops.
https://hg.mozilla.org/mozilla-central/rev/4f39921f782f
https://hg.mozilla.org/mozilla-central/rev/86c5e4ecba1e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=robcee][fixed-in-fx-team] → [good first bug][mentor=robcee]
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.