Last Comment Bug 717264 - Move Quick Filter Bar toggle button from tab toolbar to mail toolbar
: Move Quick Filter Bar toggle button from tab toolbar to mail toolbar
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on: 730072
Blocks: tb-tabsontop
  Show dependency treegraph
 
Reported: 2012-01-11 09:08 PST by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-02-23 12:44 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Patch v1 (7.34 KB, patch)
2012-02-16 11:25 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (13.24 KB, patch)
2012-02-16 11:33 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v3 (15.31 KB, patch)
2012-02-16 12:14 PST, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-01-11 09:08:41 PST
The current placement of the QFB toggle button doesn't make a whole lot of sense.  It exists outside of the tab that it belongs to, and is disabled unless that tab is selected.

Blake and I are toying with the idea of removing the Quick Filter Bar button from the tab toolbar, and just having it exist as an optional button in the Customize Toolbar dialog.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-01-11 09:10:47 PST
CC'ing some folks who might be interested.  I'm including davida, since it was a discussion with him that precipitated this idea.
Comment 2 Jim Porter (:squib) 2012-01-11 10:14:48 PST
I think we should keep it, since the button is actually useful on other tabs (potentially). Both Lightning calendar and task tabs have UI that works basically like the QFB, and it just needs to be hooked up to take advantage of the toggle button (the task tab already responds to Ctrl+Shift+K; not sure on the calendar tab).
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-01-11 10:19:08 PST
I don't know if I understand your comment…  Are you suggesting that we have the QFB toggle button do different things per-tab?
Comment 4 Jim Porter (:squib) 2012-01-11 11:00:19 PST
Correct. I think the QFB toggle should be a toggle for anything in a tab that works as a filter you can show/hide. Both kinds of Lightning tabs have something like this. The address book tab might also have something like this, though maybe we just want an always-present search box for that.

I know the Lightning devs have expressed interest in doing this before, but I think they ran into an issue with it (which might be fixed by now).
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-01-11 11:01:34 PST
Philipp:

Did you have some input on this?

-Mike
Comment 6 Philipp Kewisch [:Fallen] 2012-01-11 15:42:21 PST
Given we have a search feature both in calendar and task tab, I wouldn't object to keeping it. I understand that other tab types may not profit from this though.

If you plan to remove it from the tab toolbar anyway, I'd appreciate if you could add an optional toolbar button to Lighting (both tabs) too.

squib is right though, I think Ctrl+Shift+K is not hooked up on the calendar tab. We should fix that, maybe as part of a different bug.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 09:40:09 PST
Jb wants this for 11.
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 11:25:56 PST
Created attachment 597914 [details] [diff] [review]
Patch v1

Here's my first run at this.

I'm wary of landing stuff on beta, but a swath of local testing seems to indicate that this patch does what it's supposed to do.  I've written a simple test case for custom toolbars as well.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 11:29:01 PST
Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=ff1ee8bb1de8
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 11:33:17 PST
Created attachment 597919 [details] [diff] [review]
Patch v2

Whoops - forgot to include my tests with that last patch.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 11:39:50 PST
Try builds for Patch v2 coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=692962308836
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-02-16 12:14:26 PST
Created attachment 597947 [details] [diff] [review]
Patch v3

So I was a bit overconfident when I pushed to try so soon - I just ran all Mozmill tests locally, and I had a failure in the old migration tests for TB 2 to TB 3.

This version of the patch fixes it.  The rest of the failures I got locally were random oranges we sometimes happen to get.

I'm going to hold off requesting review until I see the test results that try spits out.  If it's just the same migration test failure as I got here, then I'll go ahead and r?.  Until then, I'd rather not stuff squib's inbox with more bugmail than he needs.  :D

-Mike
Comment 13 Thomas D. (currently busy elsewhere; needinfo?me) 2012-02-16 14:28:17 PST
After this patch, can users customize the mail toolbar to remove the quick filter button altogether? Chances are that many users (like me :) will never use the button because it's such a frequent action that it's really worth remembering the keyboard shortcut...
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-02-17 06:23:21 PST
Thomas,

Yes! The Quick Filter toggle will be like any of the other toolbar buttons - you can move it to other toolbars, or get rid of it altogether.

-Mike
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-02-21 09:47:47 PST
Comment on attachment 597947 [details] [diff] [review]
Patch v3

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

Aside from the question below, I'm happy with this.  r=me, and I think ui-r=me, just in case you need it…

::: mail/base/modules/mailMigrator.js
@@ -197,0 +197,12 @@
> > +      // In UI version 3, we move the QFB button from the tabbar toolbar to
> > +      // to the mail toolbar.
> > +      if (currentUIVersion < 3) {
> > +        let currentSetResource = this._rdf.GetResource("currentset");
NaN more ...

Wouldn't that replace "a,qfb-show-filter-bar,b" with "a,,b"?  Do we not care, because this._setPersist will take care of it for us?
Comment 16 Blake Winton (:bwinton) (:☕️) 2012-02-21 09:49:57 PST
The lines of code that was referring to was supposed to be:
    currentSet = currentSet.replace(/(^|,)qfb-show-filter-bar($|,)/,
    "$1$2");
:P
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-02-21 10:02:19 PST
Blake:

I borrowed this technique from the Firefox version of migrateUI, where they do the same thing (see:  http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/nsBrowserGlue.js#1183)

That, coupled with the fact that the migration seems to work and the test passes, suggests to me that setPersist takes care of this for us.

-Mike
Comment 18 Mike Conley (:mconley) - (Needinfo me!) 2012-02-21 11:02:16 PST
Comment on attachment 597947 [details] [diff] [review]
Patch v3

I believe we want this for TB 11.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-02-21 12:10:37 PST
Landed on comm-central as http://hg.mozilla.org/comm-central/rev/78966d586c8a
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-02-21 14:39:48 PST
Landed on comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/80897e9cafe7
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-02-21 18:56:43 PST
Landed on comm-beta as http://hg.mozilla.org/releases/comm-beta/rev/17b0b1d4af4d

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