GCLI help should be more, um helpful

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools: Console
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

It doesn't even display the 'manual' text or param desc right now.
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.
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!
https://tbpl.mozilla.org/?tree=Try&rev=28134b5df88a
Created attachment 576109 [details] [diff] [review]
upload 2

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)
https://tbpl.mozilla.org/?tree=Try&rev=587bd6d86427

Comment 8

6 years ago
Created attachment 576144 [details]
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]

Updated

6 years ago
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.
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=058542478c55

Updated

6 years ago
Attachment #576109 - Flags: review?(dcamp) → review+

Updated

6 years ago
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://tbpl.mozilla.org/?tree=Fx-Team&rev=cf3ed4316481
https://hg.mozilla.org/mozilla-central/rev/cab4a629450e
Status: NEW → RESOLVED
Last Resolved: 6 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.
You need to log in before you can comment on or make changes to this bug.