[Developer Toolbar] "pref show prefName" should show the pref name next to the pref value

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools: Graphic Commandline and Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 attachment, 7 obsolete attachments)

STR:
1) Enable the Developer Toolbar
2) In the developer toolbar, type: pref show general.smoothScroll
3) Hit enter

Expected results:
Output shows: general.smoothScroll: true

Actual results:
Output shows: true

This is nice since we have the space and users can forget what the 'true' result relates to.
Component: Developer Tools → Developer Tools: Console
QA Contact: developer.tools → developer.tools.console
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar

Comment 1

5 years ago
Hi, I'm interested in working on this bug for my class project, what are the steps to get started on this?
Hi Jonathan,

The prefShowCmdSpec at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#8953 is where the code is located that runs when a user types "pref show general.smoothScroll". When the user hits enter, the exec function is run.

The args object that is passed in to the exec function will contain all the necessary information to fix this bug.

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#4720 shows what the args.settings object is capable of.

For this bug, you'll want to change the exec function to return the equivalent of:
> return args.setting.name + ': ' + args.setting.value;

I'm not sure if these will need to be localized. I've looked through the gcli.jsm file to see if other command output is localized but I haven't been able to tell one way or the other. If we do decided that this needs to be localization-friendly, then we will need to add a string near http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/gcli.properties#224. The string would be something along the lines of:
> prefShowSettingValue=%1$S: %2$S

Using our localization libraries, the exec function would need to pass in the two values for the pref name and the pref value, along with the localization-string name. This will combine the three to output the desired "prefName: prefValue" string.

We may want to do localization here due to how different locales handle name-value separators (colon for example), or to change the direction of the pair for right-to-left locales such as Hebrew or Arabic (where they may prefer "prefValue :prefName").

Lastly, there may be some unit tests that need updating. Let me know when you've done all the above and I'll look some more to see what unit tests might need new additions or changes.
Assignee: nobody → 2010jonathan
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=jaws][lang=js]

Comment 3

5 years ago
Is there more than 1 developer that can help me with this project? Its a requirement for my project.
Hi Jonathan,

What do you need help with? Feel free to post your questions here and I or someone else can respond.

Comment 5

5 years ago
Well currently I have been trying to build gcli.jsm after editing it by entering c://mozilla-source/mozilla-central/build/pymake/make/py -C browser/devtools/commandline but i get a no makefile error. How can I do incremental builds for this file?
You can incrementally build the parent folder, which contains a Makefile.

c://mozilla-source/mozilla-central/build/pymake/make/py -C browser/devtools/
Please join #introduction on irc.mozilla.org to get quicker feedback and help with getting your build working. I and others are available to help there.

Comment 8

5 years ago
By localization you mean, you want to make the string prefShowSettingValue=%1$S: %2$S usable from the web console command line?

Comment 9

5 years ago
I have finished adding the args.setting.name to make it the output look like this general.smoothScroll: true, but do you still want me to make it localization friendly?
Yeah, this should be localization friendly. You can upload your current work-in-progress and I can help you out further.

Comment 11

5 years ago
Created attachment 712828 [details] [diff] [review]
Added the code to get the correct output
Comment on attachment 712828 [details] [diff] [review]
Added the code to get the correct output

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

::: browser/devtools/commandline/gcli.jsm
@@ +8962,5 @@
>        manual: l10n.lookup('prefShowSettingManual')
>      }
>    ],
>    exec: function Command_prefShow(args, context) {
> +    return args.setting.name + ': ' + args.setting.value;

This is close, but it needs to load the localization string and use these values to format the localization string.

I think you can use imports.stringBundle.formatStringFromName, but you may need to experiment here. See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#592 for one instance where it is used. https://developer.mozilla.org/en-US/docs/XUL/stringbundle#m-getFormattedString shows the documentation for getFormattedString, which is probably what you will need.

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ +221,5 @@
> +
> +# LOCALIZATION NOTE (prefShowManual): A fuller description of the 'pref show'
> +# command. Displayed when the user asks for help on what it does.
> +prefShowManual=Display the value of a given preference  
> +prefShowSettingValue=%1%S: %2$S

This should be:
prefShowSettingValue=%1$S: %2$S
(note the incorrect second percent sign before the colon).

Please add a localization note that describes what %1$S and %2$S get replaced with.

Also, something changed with this file (line endings?) and every line got touched. The file should have UNIX line endings (\n only), so please try to revert that change.
Attachment #712828 - Flags: feedback+

Comment 13

5 years ago
Created attachment 712835 [details] [diff] [review]
Added code and localization to get desired output

First patch did not have localization, so don't mind it.

Comment 14

5 years ago
(In reply to Jonathan Vieyra from comment #13)
> Created attachment 712835 [details] [diff] [review]
> Added code and localization to get desired output
> 
> First patch did not have localization, so don't mind it.

@Jared didn't notice you replied ill change the localization file back to its regular format

Comment 15

5 years ago
Created attachment 715898 [details] [diff] [review]
Reformatted localization file with UNIX line endings and fixed typo
Attachment #712828 - Attachment is obsolete: true
Attachment #712835 - Attachment is obsolete: true
Comment on attachment 715898 [details] [diff] [review]
Reformatted localization file with UNIX line endings and fixed typo

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

Hi Jonathan, it looks like the properties file was left out of this patch. Also, when uploading a patch, please request review from somebody to make sure that it doesn't go unnoticed.
Attachment #715898 - Flags: feedback-

Comment 17

5 years ago
I don't know what happened with the properties file, I changed the format back to UNIX in Notepad++ and I created the patch like I always would. It was probably something I did when saving but if its not what could I do to get the properties file in the patch?
(In reply to Jonathan Vieyra from comment #17)
> I don't know what happened with the properties file, I changed the format
> back to UNIX in Notepad++ and I created the patch like I always would. It
> was probably something I did when saving but if its not what could I do to
> get the properties file in the patch?

You could check to make sure that the changes are present. Maybe they got undone? Maybe they are there and got committed separately? If you join #introduction on irc.mozilla.org you should be able to get help from myself or someone else there.

Comment 19

5 years ago
For some reason stringBundle.formatStringFromName(prefShowSettingValue, [args.setting.name, args.setting.value], 2); does not work anymore so I changed it to imports.stringBundle.getFormattedString(prefShowSettingValue, [args.setting.name, args.setting.value]); and that doesn't work either.  Both give me a Reference error saying either stringBundle or imports is not defined. So has the stringBundle lib changed?

Comment 20

5 years ago
Ok so now I got it down to this, return mozl10n.lookupFormat(prefShowSettingValue, [args.setting.name, args.setting.value]); but it says that prefShowSettingValue is not defined. So is prefShowSettingValue still the key or how can i define prefShowSettingValue?
(In reply to Jonathan Vieyra from comment #20)
> Ok so now I got it down to this, return
> mozl10n.lookupFormat(prefShowSettingValue, [args.setting.name,
> args.setting.value]); but it says that prefShowSettingValue is not defined.
> So is prefShowSettingValue still the key or how can i define
> prefShowSettingValue?

I think the prefShowSettingValue should be in quotes, i.e.

  mozl10n.lookupFormat('prefShowSettingValue', ...);
(In reply to Jonathan Vieyra from comment #20)
> Ok so now I got it down to this, return
> mozl10n.lookupFormat(prefShowSettingValue, [args.setting.name,
> args.setting.value]); but it says that prefShowSettingValue is not defined.
> So is prefShowSettingValue still the key or how can i define
> prefShowSettingValue?

Pressed enter to quickly... 

prefShowSettingValue is defined in gcli.properties as mentioned in 
comments 2 and comment 12.  IOW, you need to add that to the
gcli.properties.

Comment 23

5 years ago
Putting prefShowSettingValue in quotes gives an error, and I dont think im entering to quickly because Ive been typing everything at the same speed.
(In reply to Jonathan Vieyra from comment #23)
> Putting prefShowSettingValue in quotes gives an error, and I dont think im
> entering to quickly because Ive been typing everything at the same speed.

(In reply to Edmund Wong (:ewong) from comment #22)
> (In reply to Jonathan Vieyra from comment #20)
> > Ok so now I got it down to this, return
> > mozl10n.lookupFormat(prefShowSettingValue, [args.setting.name,
> > args.setting.value]); but it says that prefShowSettingValue is not defined.
> > So is prefShowSettingValue still the key or how can i define
> > prefShowSettingValue?
> 
> Pressed enter to quickly... 
> 
> prefShowSettingValue is defined in gcli.properties as mentioned in 
> comments 2 and comment 12.  IOW, you need to add that to the
> gcli.properties.

should read: prefShowSettingValue is supposed to be defined in gcli.properties,
as mentioned in comment 2 and comment 12.  (You need to add that to the
gcli.properties.)

Comment 25

5 years ago
Created attachment 724318 [details]
My gcli.properties

Comment 26

5 years ago
Created attachment 724320 [details]
My gcli.jsm
(In reply to Jonathan Vieyra from comment #26)
> Created attachment 724320 [details]
> My gcli.jsm

You're supposed to create a patch of both gcli.jsm and gcli.properties like
what you did the first time and ask for review. 

Sorry if I mislead you in any way.

Comment 28

5 years ago
Created attachment 724323 [details]
Error Message

Ive attached my gcli.properties and gcli.jsm files and a picture of the error message that I get when I call the exec function.

Comment 29

5 years ago
Sorry, I thought a patch was suppose to be a fix to the bug, and I since I know these files are not a fix to the bug, I didn't put them into a patch.  So anytime I attach something it should be a patch?
(In reply to Jonathan Vieyra from comment #28)
> Created attachment 724323 [details]
> Error Message
> 
> Ive attached my gcli.properties and gcli.jsm files and a picture of the
> error message that I get when I call the exec function.

I just looked at your gcli.properties and gcli.jsm files.  They're truncated
at the top. Are those files actually truncated a bit at the top or
is it a faulty copy&paste?
Created attachment 724463 [details] [diff] [review]
Patch

Jonathan, what do you think of this approach?
Attachment #715898 - Attachment is obsolete: true
Attachment #724318 - Attachment is obsolete: true
Attachment #724320 - Attachment is obsolete: true
Attachment #724323 - Attachment is obsolete: true
Attachment #724463 - Flags: feedback?(2010jonathan)

Comment 32

5 years ago
I like what you added to the localization file, its a lot more clear then what I had, but the return statement in the exec function doesn't give the correct return, it just returns the string 'prefShowSettingValue'. I'll keep playing with it.
(In reply to Jonathan Vieyra from comment #32)
> I like what you added to the localization file, its a lot more clear then
> what I had, but the return statement in the exec function doesn't give the
> correct return, it just returns the string 'prefShowSettingValue'. I'll keep
> playing with it.

I'm not sure what you mean. Did you also rebuild /browser/locales/ ? When I build with this patch, the exec function returns the preference name followed by a colon, then a space, then the pref value.

Comment 34

5 years ago
Ok yeah it works, sorry for the confusion. Is there anything else that you want done?
(In reply to Jonathan Vieyra from comment #34)
> Ok yeah it works, sorry for the confusion. Is there anything else that you
> want done?

No problem. I've pushed the patch to our tryserver to make sure that this change didn't break any tests. Any broken tests will need to be fixed before we can check this in.

https://tbpl.mozilla.org/?tree=Try&rev=93cf8922b21f
Sigh, the try push in comment #35 had the wrong syntax for the test suite. Repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e9e181579513
Created attachment 725089 [details] [diff] [review]
Patch v.1

I had to fix a few tests in /browser/devtools/commandline/test/browser_cmd_prefs.js. Jonathan, does this look good to you?
Attachment #725089 - Flags: feedback?(2010jonathan)
Attachment #724463 - Attachment is obsolete: true
Attachment #724463 - Flags: feedback?(2010jonathan)

Comment 38

5 years ago
Yeah everything looks good. I'm confused about how you ran the test though.
To run the tests, you need to build obj-dir/browser/devtools/commandline/test and then run the following command from the mozilla-central directory:
mach mochitest-browser browser/devtools/commandline/test/browser_cmd_pref.js
Attachment #725089 - Flags: review?(jwalker)
Comment on attachment 725089 [details] [diff] [review]
Patch v.1

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

Thanks!
Attachment #725089 - Flags: review?(jwalker) → review+
Thanks for working on the bug Jonathan. Would you like to take a stab at another bug? You can find some good first bugs at http://www.joshmatthews.net/bugsahoy/

I've pushed the changes to our mozilla-inbound repository:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8789fb12a3
Assignee: 2010jonathan → jAwS
Attachment #725089 - Flags: feedback?(2010jonathan)

Comment 42

5 years ago
Thank you very much Jared, for your help and guidance. Yeah I definitely want to take on another bug! I'll take a look at them, and hopefully I can get started soon.
https://hg.mozilla.org/mozilla-central/rev/8c8789fb12a3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.