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

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Jonathan Vieyra]

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jonathan Vieyra]

Is there more than 1 developer that can help me with this project? Its a requirement for my project.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Hi Jonathan,

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

Comment 5

[Jonathan Vieyra]

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?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

You can incrementally build the parent folder, which contains a Makefile.

c://mozilla-source/mozilla-central/build/pymake/make/py -C browser/devtools/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jonathan Vieyra]

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

Comment 9

[Jonathan Vieyra]

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?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, this should be localization friendly. You can upload your current work-in-progress and I can help you out further.

Comment 11

[Jonathan Vieyra]

Attachment: Added the code to get the correct output

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Jonathan Vieyra]

Attachment: Added code and localization to get desired output

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

Comment 14

[Jonathan Vieyra]

(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

[Jonathan Vieyra]

Attachment: Reformatted localization file with UNIX line endings and fixed typo

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 17

[Jonathan Vieyra]

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?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(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

[Jonathan Vieyra]

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

[Jonathan Vieyra]

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?

Comment 21

[Edmund Wong (:ewong)]

(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', ...);

Comment 22

[Edmund Wong (:ewong)]

(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

[Jonathan Vieyra]

Putting prefShowSettingValue in quotes gives an error, and I dont think im entering to quickly because Ive been typing everything at the same speed.

Comment 24

[Edmund Wong (:ewong)]

(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

[Jonathan Vieyra]

Attachment: My gcli.properties

Comment 26

[Jonathan Vieyra]

Attachment: My gcli.jsm

Comment 27

[Edmund Wong (:ewong)]

(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

[Jonathan Vieyra]

Attachment: 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

[Jonathan Vieyra]

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?

Comment 30

[Edmund Wong (:ewong)]

(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?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Jonathan, what do you think of this approach?

Comment 32

[Jonathan Vieyra]

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.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(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

[Jonathan Vieyra]

Ok yeah it works, sorry for the confusion. Is there anything else that you want done?

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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?

Comment 38

[Jonathan Vieyra]

Yeah everything looks good. I'm confused about how you ran the test though.

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 40

[Joe Walker [:jwalker]]

Comment on attachment 725089 [details] [diff] [review]
Patch v.1

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

Thanks!

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 42

[Jonathan Vieyra]

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.

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8c8789fb12a3