Last Comment Bug 760951 - Add a filter box to the error console
: Add a filter box to the error console
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Error Console (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
: 83019 260046 (view as bug list)
Depends on: 768642 855641 763821 764346 799084 801266
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-03 02:25 PDT by Tom Schuster [:evilpie]
Modified: 2013-04-19 13:40 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
disabled


Attachments
v1 (8.02 KB, patch)
2012-06-03 02:25 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v2 (8.02 KB, patch)
2012-06-03 02:30 PDT, Tom Schuster [:evilpie]
jaws: feedback+
Details | Diff | Review
v3 (9.73 KB, patch)
2012-06-05 08:31 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review
v3 now with 100% less debugging code (9.23 KB, patch)
2012-06-05 08:39 PDT, Tom Schuster [:evilpie]
jaws: feedback+
Details | Diff | Review
v4 (9.28 KB, patch)
2012-06-05 14:18 PDT, Tom Schuster [:evilpie]
jaws: review+
Details | Diff | Review
backout of patch for beta 17 (9.30 KB, patch)
2012-10-08 17:06 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Tom Schuster [:evilpie] 2012-06-03 02:25:28 PDT
Created attachment 629575 [details] [diff] [review]
v1

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.
Comment 1 Tom Schuster [:evilpie] 2012-06-03 02:30:38 PDT
Created attachment 629577 [details] [diff] [review]
v2
Comment 2 Tom Schuster [:evilpie] 2012-06-03 02:32:43 PDT
Comment on attachment 629577 [details] [diff] [review]
v2

Sorry for bugspam, just noticed a small bug while looking at this in the review mode.
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-06-04 10:39:29 PDT
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?
Comment 4 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-06-04 10:54:53 PDT
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.
Comment 5 Tom Schuster [:evilpie] 2012-06-05 08:31:07 PDT
Created attachment 630176 [details] [diff] [review]
v3

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.
Comment 6 Tom Schuster [:evilpie] 2012-06-05 08:39:09 PDT
Created attachment 630180 [details] [diff] [review]
v3 now with 100% less debugging code
Comment 7 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-06-05 13:00:18 PDT
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.
Comment 8 Tom Schuster [:evilpie] 2012-06-05 14:18:23 PDT
Created attachment 630325 [details] [diff] [review]
v4

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.
Comment 9 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-06-05 15:57:45 PDT
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"
Comment 11 Graeme McCutcheon [:graememcc] 2012-06-08 04:21:51 PDT
https://hg.mozilla.org/mozilla-central/rev/5f99fe9d2182

(Merged by Ed Morley)
Comment 12 neil@parkwaycc.co.uk 2012-06-20 02:37:52 PDT
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 13 neil@parkwaycc.co.uk 2012-06-20 02:49:37 PDT
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.
Comment 14 Rob Campbell [:rc] (:robcee) 2012-06-26 11:31:41 PDT
This broke styling and landed without tests. I would very much like to see unittests for features in the error console.
Comment 15 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-07 12:19:04 PDT
Based on the slow progress on fixing 763821, I think we should back this out from Firefox 16.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-07 12:31:25 PDT
Or make better progress on bug 763821 :)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-21 14:56:16 PDT
Backed out on Aurora for Firefox 16 because of bug 763821:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6da3c55e7c
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-11 04:15:03 PDT
Please do the new work in a new bug - this one's done :)
Comment 19 Dão Gottwald [:dao] 2012-10-08 05:03:42 PDT
(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.
Comment 20 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-08 17:06:29 PDT
Created attachment 669340 [details] [diff] [review]
backout of patch for beta 17

[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
Comment 21 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-08 17:07:18 PDT
(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.
Comment 22 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-08 17:32:25 PDT
Backed out of firefox17-beta since bug 763821 didn't get fixed until firefox18:
https://hg.mozilla.org/releases/mozilla-beta/rev/33fc1373a99b
Comment 23 Alfred Kayser 2012-11-16 02:59:51 PST
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>
Comment 24 Alfred Kayser 2012-11-16 03:00:41 PST
(In reply to Alfred Kayser from comment #23)
Oops, wrong box... Apologies!
Comment 25 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-19 12:28:20 PDT
*** Bug 83019 has been marked as a duplicate of this bug. ***
Comment 26 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-19 13:40:09 PDT
*** Bug 260046 has been marked as a duplicate of this bug. ***

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