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)
Toolkit Graveyard
Error Console
Tracking
(firefox16 disabled, firefox17 disabled)
RESOLVED
FIXED
mozilla16
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(2 files, 4 obsolete files)
9.28 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
9.30 KB,
patch
|
akeybl
:
approval-mozilla-beta+
|
Details | Diff | 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)
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #629575 -
Attachment is obsolete: true
Attachment #629575 -
Flags: feedback?(dolske)
Assignee | ||
Comment 2•12 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•12 years ago
|
Attachment #629577 -
Attachment is patch: true
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #630176 -
Attachment is obsolete: true
Attachment #630176 -
Flags: review?(jaws)
Attachment #630180 -
Flags: review?(jaws)
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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 9•12 years ago
|
||
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•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f99fe9d2182
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5f99fe9d2182 (Merged by Ed Morley)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
This broke styling and landed without tests. I would very much like to see unittests for features in the error console.
Comment 15•12 years ago
|
||
Based on the slow progress on fixing 763821, I think we should back this out from Firefox 16.
Comment 16•12 years ago
|
||
Or make better progress on bug 763821 :)
Comment 17•12 years ago
|
||
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•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•12 years ago
|
||
Please do the new work in a new bug - this one's done :)
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 19•12 years ago
|
||
(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
Comment 20•12 years ago
|
||
[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?
Comment 21•12 years ago
|
||
(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•12 years ago
|
Attachment #669340 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•12 years ago
|
||
Backed out of firefox17-beta since bug 763821 didn't get fixed until firefox18: https://hg.mozilla.org/releases/mozilla-beta/rev/33fc1373a99b
Updated•12 years ago
|
status-firefox17:
--- → disabled
Comment 23•12 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•12 years ago
|
||
(In reply to Alfred Kayser from comment #23) Oops, wrong box... Apologies!
Updated•8 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•