Closed Bug 760951 Opened 12 years ago Closed 12 years ago

Add a filter box to the error console

Categories

(Toolkit Graveyard :: Error Console, defect)

defect
Not set
normal

Tracking

(firefox16 disabled, firefox17 disabled)

RESOLVED FIXED
mozilla16
Tracking Status
firefox16 --- disabled
firefox17 --- disabled

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
Finding specific messages in the error console can be very cumbersome. So I took a peek into how the web console does filtering and figured it wouldn't be too hard to implement. 

I never really looked at XUL before and the rest of the code also is confusing. I have not written a test for this, because I have no idea how this is done, going to look into that.

Dolkse I asked you for feedback, because you reviewed some other bug in this component that I found.
Attachment #629575 - Flags: feedback?(dolske)
Attached patch v2 (obsolete) — Splinter Review
Attachment #629575 - Attachment is obsolete: true
Attachment #629575 - Flags: feedback?(dolske)
Comment on attachment 629577 [details] [diff] [review]
v2

Sorry for bugspam, just noticed a small bug while looking at this in the review mode.
Attachment #629577 - Flags: feedback?(dolske)
Attachment #629577 - Attachment is patch: true
Comment on attachment 629577 [details] [diff] [review]
v2

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

::: toolkit/components/console/content/console.xul
@@ +78,5 @@
>        <toolbarbutton id="Console:clear" oncommand="clearConsole();"
>                       label="&clear.label;" accesskey="&clear.accesskey;"/>
> +      <toolbarseparator/>
> +      <textbox placeholder="Filter" type="search" id="Filter" oncommand="changeFilter()" 
> +              oninput="changeFilter()"/>

Can we move this textbox and placeholder down to the ToolbarEval toolbar? The height of the ToolbarEval toolbar is more inline with the height of the textbox so it should fit in better visually there.

::: toolkit/components/console/content/consoleBindings.xml
@@ +296,5 @@
> +          if (this.stringMatchesFilters(aRow.getAttribute("msg"), this.mFilter)) {
> +            aRow.classList.remove("filtered-by-string")
> +          } else {
> +            aRow.classList.add("filtered-by-string")
> +          }

These four lines are duplicated with applyFilter(). Can you remove this duplication?

@@ +349,5 @@
> +            return true;
> +          }
> +
> +          let searchStr = aString.toLowerCase();
> +          let filterStrings = aFilter.toLowerCase().split(/\s+/);

stringMatchesFilter is called for each row in the Error Console. It would be nice if we didn't have to toLowerCase() the filter each time this function is called. Can you move toLowerCase() on the filter to the callers?
Attachment #629577 - Flags: feedback?(dolske) → feedback+
Comment on attachment 629577 [details] [diff] [review]
v2

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

::: toolkit/components/console/content/console.xul
@@ +77,5 @@
>        <toolbarseparator/>
>        <toolbarbutton id="Console:clear" oncommand="clearConsole();"
>                       label="&clear.label;" accesskey="&clear.accesskey;"/>
> +      <toolbarseparator/>
> +      <textbox placeholder="Filter" type="search" id="Filter" oncommand="changeFilter()" 

As mentioned on IRC by evilpie, the 'Filter' user-facing text will have to be moved to a DTD file for l10n.
Attached patch v3 (obsolete) — Splinter Review
So fixed the nits, I don't actually believe that keeping mFilter around as lower case is worthwhile, this string is usually going to be short. 

>Can we move this textbox and placeholder down to the ToolbarEval toolbar? The height of the 
>ToolbarEval toolbar is more inline with the height of the textbox so it should fit in better 
>visually there.
I showed this to Jared on IRC and he agreed that, it doesn't look too bad, so he wants to test this on Windows to see if it would work there, too.
Attachment #629577 - Attachment is obsolete: true
Attachment #630176 - Flags: review?(jaws)
Attachment #630176 - Attachment is obsolete: true
Attachment #630176 - Flags: review?(jaws)
Attachment #630180 - Flags: review?(jaws)
Comment on attachment 630180 [details] [diff] [review]
v3 now with 100% less debugging code

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

This seems fine to me but we should move it down to ToolbarEval since the textbox is too tall on Windows. See http://screencast.com/t/80POtbg5Z9 for a screenshot of the patch on Windows.

Just so you know though, bug 602006 is going to remove the error console from the Firefox developer tools menu, thus making this change less visible.
Attachment #630180 - Flags: review?(jaws) → feedback+
Attached patch v4Splinter Review
After a few words on IRC jaws and me, came up with that. The box is now aligned to right of the window, but at the middle of the toolbar so it doesn't look to big on windows.
Attachment #630180 - Attachment is obsolete: true
Attachment #630325 - Flags: review?(jaws)
Comment on attachment 630325 [details] [diff] [review]
v4

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

I noticed a weird bug where the textarea would shrink the length of the placeholder text when given focus. It's probably a bug in xul:textbox.

r+ with the following change. Thanks!

::: toolkit/components/console/content/console.xul
@@ +77,5 @@
>        <toolbarseparator/>
>        <toolbarbutton id="Console:clear" oncommand="clearConsole();"
>                       label="&clear.label;" accesskey="&clear.accesskey;"/>
> +
> +      <vbox valign="middle" align="right" flex="1">

align="end"
Attachment #630325 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/5f99fe9d2182

(Merged by Ed Morley)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 763821
Depends on: 764346
Bah, the extra textbox in the tab order messes with my muscle memory...

(In reply to Jared Wein from comment #9)
> (From update of attachment 630325 [details] [diff] [review])
> > +      <vbox valign="middle" align="right" flex="1">
> align="end"
valign="middle" is also deprecated, you might have wanted pack="center".
Comment on attachment 630325 [details] [diff] [review]
v4

>+        <textbox placeholder="&filter.label;" accesskey="&filter.accesskey;" type="search" id="Filter" 
>+                 oncommand="changeFilter()" oninput="changeFilter()"/>
Also, don't use oninput on a <textbox type="search">. If you really don't like the default 500ms timeout you should change that instead.
This broke styling and landed without tests. I would very much like to see unittests for features in the error console.
Based on the slow progress on fixing 763821, I think we should back this out from Firefox 16.
Or make better progress on bug 763821 :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please do the new work in a new bug - this one's done :)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(In reply to neil@parkwaycc.co.uk from comment #13)
> Comment on attachment 630325 [details] [diff] [review]
> v4
> 
> >+        <textbox placeholder="&filter.label;" accesskey="&filter.accesskey;" type="search" id="Filter" 
> >+                 oncommand="changeFilter()" oninput="changeFilter()"/>
> Also, don't use oninput on a <textbox type="search">. If you really don't
> like the default 500ms timeout you should change that instead.

Filed bug 799084.
Depends on: 799084
[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug, not ready to ship
User impact if declined: Error Console devtool will look wonky
Testing completed (on m-c, etc.): locally, and it was also backed out of 16-aurora
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none
Attachment #669340 - Flags: approval-mozilla-beta?
(In reply to Jared Wein [:jaws] from comment #20)
> String or UUID changes made by this patch: none

Correction, two strings are *removed* by this backout, but no string changes or additions.
Attachment #669340 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out of firefox17-beta since bug 763821 didn't get fixed until firefox18:
https://hg.mozilla.org/releases/mozilla-beta/rev/33fc1373a99b
Depends on: 801266
No a direct theme change, but a change in the UI:
[url=https://bugzilla.mozilla.org/show_bug.cgi?id=760951]Bug 760951[/url] - Add a filter box to the error console
[url=https://bugzilla.mozilla.org/show_bug.cgi?id=763821]Bug 763821[/url] - Error console looks wonky on OS X
[url=https://bugzilla.mozilla.org/show_bug.cgi?id=799081]Bug 799081[/url] - Bar at the bottom of the error console should be a <statusbar>
(In reply to Alfred Kayser from comment #23)
Oops, wrong box... Apologies!
Depends on: 855641
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.