Last Comment Bug 701712 - GCLI help should be more, um helpful
: GCLI help should be more, um helpful
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 11
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
: Brian Grinstead [:bgrins]
Mentors:
Depends on:
Blocks: GCLI-SHIP
  Show dependency treegraph
 
Reported: 2011-11-11 07:44 PST by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-12-15 23:54 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (20.85 KB, patch)
2011-11-18 08:01 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review+
Details | Diff | Splinter Review
upload 2 (24.02 KB, patch)
2011-11-22 02:20 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
dao+bmo: review+
paul: feedback+
Details | Diff | Splinter Review
list-style-image bug (95.88 KB, image/png)
2011-11-22 06:46 PST, Paul Rouget [:paul]
no flags Details

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-11 07:44:35 PST
It doesn't even display the 'manual' text or param desc right now.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 08:01:32 PST
Created attachment 575463 [details] [diff] [review]
upload 1

dcamp has already seen this at https://github.com/campd/gcli/pull/1
mihai i wanted to change a description to an hbox and change the class, but you may propose a different solution.
Comment 2 Mihai Sucan [:msucan] 2011-11-18 09:59:02 PST
Comment on attachment 575463 [details] [diff] [review]
upload 1

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

The patch doesn't apply cleanly on top of the fx-team repo code. No bug dependencies have been set either. I can't test it.

Giving r+ because the patch looks fine to me and if you tested it doesn't cause failures, it's all fine. Please address the two comments below.

Thank you!

::: browser/devtools/webconsole/HUDService.jsm
@@ +7125,5 @@
> +
> +    let hud = HUDService.getHudReferenceById(this.hudId);
> +    let timestamp = ConsoleUtils.timestamp();
> +    new Templater().processNode(element, {
> +      iconContainerStyle: "margin-left=" + (hud.groupDepth * GROUP_INDENT) + "px",

s/margin-left=/margin-left:/

::: browser/themes/gnomestripe/devtools/gcli.css
@@ +291,5 @@
> +/* From: $GCLI/lib/gcli/commands/help.css */
> +
> +.gcli-help-name {
> +  text-align: right;
> +}

Is this correct for RTL?
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 09:59:38 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 11:56:40 PST
(In reply to Mihai Sucan [:msucan] from comment #2)
> Comment on attachment 575463 [details] [diff] [review] [diff] [details] [review]
> upload 1
> 
> Review of attachment 575463 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> The patch doesn't apply cleanly on top of the fx-team repo code. No bug
> dependencies have been set either. I can't test it.
> 
> Giving r+ because the patch looks fine to me and if you tested it doesn't
> cause failures, it's all fine. Please address the two comments below.
> 
> Thank you!

I could tell you all the previous bug to apply, but then you'd want to kill me for cruel and unusual punishment. I think there are 8 patches before this!

I always post to try before I land.

> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +7125,5 @@
> > +
> > +    let hud = HUDService.getHudReferenceById(this.hudId);
> > +    let timestamp = ConsoleUtils.timestamp();
> > +    new Templater().processNode(element, {
> > +      iconContainerStyle: "margin-left=" + (hud.groupDepth * GROUP_INDENT) + "px",
> 
> s/margin-left=/margin-left:/

Huh - good spot.

> ::: browser/themes/gnomestripe/devtools/gcli.css
> @@ +291,5 @@
> > +/* From: $GCLI/lib/gcli/commands/help.css */
> > +
> > +.gcli-help-name {
> > +  text-align: right;
> > +}
> 
> Is this correct for RTL?

Probably not. Will check and fix.
Thanks!
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-21 08:34:17 PST
https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-22 02:20:43 PST
Created attachment 576109 [details] [diff] [review]
upload 2

Fix for test failure.
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-22 02:41:56 PST
https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427
Comment 8 Paul Rouget [:paul] 2011-11-22 06:46:28 PST
Created attachment 576144 [details]
list-style-image bug

The code looks good, but the list-style-image is wrong with linux (see screenshot)
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-22 06:50:41 PST
Paul - it happens on all OSes unfortunately. Bug 704182
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-28 02:12:35 PST
Dao - This bug is still looking for your review. Thanks.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-30 15:53:23 PST
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=058542478c55
Comment 12 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 04:17:51 PST
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cab4a629450e
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-08 06:06:37 PST
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481
Comment 14 Tim Taubert [:ttaubert] 2011-12-08 21:22:49 PST
https://hg.mozilla.org/mozilla-central/rev/cab4a629450e
Comment 15 Francesco Lodolo [:flod] 2011-12-10 21:02:16 PST
helpSearchManual=A search string to use in narrowing down the list of commands that are displayed to the user. Any part of the string can match, regular expressions are not supported.

Is it just me or this string is not really clear? First there is a "search string", then a "string" which is, if I understand it correctly, the searched item (command in this case). Maybe "Any part of the command (name) can match" would be clearer (obviously if I'm not misunderstanding this string)?
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-13 05:52:05 PST
(In reply to flod (Francesco Lodolo) from comment #15)
> helpSearchManual=A search string to use in narrowing down the list of
> commands that are displayed to the user. Any part of the string can match,
> regular expressions are not supported.
> 
> Is it just me or this string is not really clear? First there is a "search
> string", then a "string" which is, if I understand it correctly, the
> searched item (command in this case). Maybe "Any part of the command (name)
> can match" would be clearer (obviously if I'm not misunderstanding this
> string)?

Quite right - command name is much clearer. I'll find a way to fix this.
Thanks.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-15 15:14:28 PST
(In reply to flod (Francesco Lodolo) from comment #15)
> helpSearchManual=A search string to use in narrowing down the list of
> commands that are displayed to the user. Any part of the string can match,
> regular expressions are not supported.
> 
> Is it just me or this string is not really clear? First there is a "search
> string", then a "string" which is, if I understand it correctly, the
> searched item (command in this case). Maybe "Any part of the command (name)
> can match" would be clearer (obviously if I'm not misunderstanding this
> string)?

Hey Francesco - I've got a change which literally just swaps the second 'string' for 'command name' as suggestsed. Would you like a key change to go with minor changes like that? I'm not changing any meaning, so perhaps no key change is needed?
Comment 18 Francesco Lodolo [:flod] 2011-12-15 21:41:20 PST
CCing Axel. 

Personally I would go without a key change in this case (I can send a message in dev.l10n to point this out for localizers working on central), but there's a bit of confusion in dev.l10n about this lately ;-)
Comment 19 Axel Hecht [:Pike] 2011-12-15 23:54:22 PST
Yeah, borderline.

Looking through mxr, most folks translated it very much like the original text, which means, it'll be confusing to users.

Thus, I'd favor changing the key.

Also, I'd make it "Any part of a command name can match" with "a" instead of "the", as we're not talking about any specific command here.

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