Closed Bug 701712 Opened 13 years ago Closed 12 years ago

GCLI help should be more, um helpful

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

Attachments

(2 files, 1 obsolete file)

It doesn't even display the 'manual' text or param desc right now.
Attached patch upload 1 (obsolete) — Splinter Review
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.
Attachment #575463 - Flags: review?(mihai.sucan)
Attachment #575463 - Flags: review?(dcamp)
Attachment #575463 - Flags: feedback?(paul)
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?
Attachment #575463 - Flags: review?(mihai.sucan) → review+
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
(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!
Attached patch upload 2Splinter Review
Fix for test failure.
Attachment #575463 - Attachment is obsolete: true
Attachment #575463 - Flags: review?(dcamp)
Attachment #575463 - Flags: feedback?(paul)
Attachment #576109 - Flags: review?(dcamp)
Attachment #576109 - Flags: feedback?(paul)
Attached image list-style-image bug
The code looks good, but the list-style-image is wrong with linux (see screenshot)
Paul - it happens on all OSes unfortunately. Bug 704182
Whiteboard: [land-in-fx-team]
Attachment #576109 - Flags: feedback?(paul) → feedback+
Whiteboard: [land-in-fx-team]
Attachment #576109 - Flags: review?(dao)
Dao - This bug is still looking for your review. Thanks.
Attachment #576109 - Flags: review?(dcamp) → review+
Attachment #576109 - Flags: review?(dao) → review+
https://tbpl.mozilla.org/?tree=Fx-Team&rev=cab4a629450e
Whiteboard: [landed-in-fxteam]
Whiteboard: [landed-in-fxteam] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cab4a629450e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
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)?
(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.
(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?
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 ;-)
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.