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
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)
Depends on: 730072
Blocks: tb-tabsontop
  Show dependency treegraph
Reported: 2012-01-11 09:08 PST by Mike Conley (:mconley)
Modified: 2012-02-23 12:44 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

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

Description User image Mike Conley (:mconley) 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 User image Mike Conley (:mconley) 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 User image 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 User image 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 User image 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 User image Mike Conley (:mconley) 2012-01-11 11:01:34 PST

Did you have some input on this?

Comment 6 User image 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 User image Mike Conley (:mconley) 2012-02-16 09:40:09 PST
Jb wants this for 11.
Comment 8 User image Mike Conley (:mconley) 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 User image Mike Conley (:mconley) 2012-02-16 11:29:01 PST
Try builds coming in here:
Comment 10 User image Mike Conley (:mconley) 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 User image Mike Conley (:mconley) 2012-02-16 11:39:50 PST
Try builds for Patch v2 coming in here:
Comment 12 User image Mike Conley (:mconley) 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

Comment 13 User image 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 User image Mike Conley (:mconley) 2012-02-17 06:23:21 PST

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.

Comment 15 User image 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 User image 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($|,)/,
Comment 17 User image Mike Conley (:mconley) 2012-02-21 10:02:19 PST

I borrowed this technique from the Firefox version of migrateUI, where they do the same thing (see:

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.

Comment 18 User image Mike Conley (:mconley) 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 User image Mike Conley (:mconley) 2012-02-21 12:10:37 PST
Landed on comm-central as
Comment 20 User image Mike Conley (:mconley) 2012-02-21 14:39:48 PST
Landed on comm-aurora as
Comment 21 User image Mike Conley (:mconley) 2012-02-21 18:56:43 PST
Landed on comm-beta as

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