The default bug view has changed. See this FAQ.

Add a filter box to the error console

RESOLVED FIXED in mozilla16

Status

Toolkit Graveyard
Error Console
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla16
Dependency tree / graph

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #629575 - Flags: feedback?(dolske)
(Assignee)

Comment 1

5 years ago
Created attachment 629577 [details] [diff] [review]
v2
(Assignee)

Updated

5 years ago
Attachment #629575 - Attachment is obsolete: true
Attachment #629575 - Flags: feedback?(dolske)
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
Attachment #629577 - Attachment is obsolete: true
Attachment #630176 - Flags: review?(jaws)
(Assignee)

Comment 6

5 years ago
Created attachment 630180 [details] [diff] [review]
v3 now with 100% less debugging code
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+
(Assignee)

Comment 8

5 years ago
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.
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+
(Assignee)

Comment 10

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f99fe9d2182
https://hg.mozilla.org/mozilla-central/rev/5f99fe9d2182

(Merged by Ed Morley)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
Depends on: 768642
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 :)
Backed out on Aurora for Firefox 16 because of bug 763821:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6da3c55e7c
status-firefox16: --- → disabled
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please do the new work in a new bug - this one's done :)
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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
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
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.

Updated

5 years ago
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

Updated

5 years ago
Depends on: 801266

Updated

5 years ago
status-firefox17: --- → disabled

Comment 23

4 years ago
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

4 years ago
(In reply to Alfred Kayser from comment #23)
Oops, wrong box... Apologies!

Updated

4 years ago
Depends on: 855641
Duplicate of this bug: 83019
Duplicate of this bug: 260046
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.