502767 - selection of "thread" column not persisted in custom view

Status: RESOLVED DUPLICATE of bug 545955

(Thunderbird :: Folder and Message Lists, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

I have a "Last 5 Days" custom view defined that I use most of the time.  I select that for many folders, and it is persisted across folder changes.  When I also click on the "thread" column in the message list for one of those folders, that is not persisted across folder switches which quickly gets aggravating.  If I switch from my custom view to the default "All" view, the bug disappears.

This appears to be different from bug 498109.

Marking as blocking bug 497199, as this is almost certainly some sort of fallout from that view changes.  I wonder, though, if it was actually bug 497279 that caused this.  Adding regressionwindow-wanted, since that info might turn out to be helpful.

I want to target this at b3, since it's frustrating for those of us who depend on custom views a lot.  However, since it's not the default, I can't bring myself to say that we'd hold for it.  That said, if someone feels like taking a run at this after they're done with their b3 bugs, it seems worthy of considering for post-slushy-freeze approval.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

This is slightly less bad than I thought.  The actual problem is that any changes made when in the Last 5 Days view  (or any view other than All?) don't persist.  So to workaround the issue, one can go back to the All view, click on the thread column header, and then go back to the Last 5 Days view that one actually intends to use.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

doh.

in nsMsgQuickSearchDBView::Open:
  m_viewFolder = nsnull;

in nsMsgDBView::SetViewFlags:
  if (m_viewFolder)
    folderInfo->SetViewFlags(aViewFlags)

I do not know why it is nulling out viewFolder, but that is the problem.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

And I should note that we explicitly set viewFolder if we are in a virtual folder/saved search folder, which makes it work for those guys.  We could always just set viewFolder, but I would like to know why that dude is nulled out before we do that.

Comment 4

[David :Bienvenu]

There was definitely a reason we wanted m_viewFolder to be temporarily null during Open of a quick search but I don't remember what it was...

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

It looks like the intent was probably that changes made in quick-search would not be persisted (viewFolder == null), but changes made in a virtual folder would be (viewFolder gets set to something by chrome JS), since they use the same view type.  (FWIW, the origin of the code is attachment #198598 [details] [diff] [review] on bug 263180, but the bug itself doesn't shed any light itself, and the patch only modified nsMsgDBView::SetViewFlags to use m_viewFolder instead of m_folder.)

This chrome JS logic appears to be correctly replicated in DBViewWrapper, or at least a cursory examination indicates that the logic was guarded by a check of gVirtualFolderTerms andd DBViewWrapper uses its "isVirtual" check as a guard.  I forget whether gVirtualFolderTerms moved in mysterious ways or not... it might have had mail-views folded in even when non-virtual.

The question is then whether we want to avoid persisting setting changes made while in a quick-search or not.  If we want to persist settings made in quick-search, we can just set viewFolder in all cases.  If we only want to add mail-views to the party, we will set it unless there are user terms.  We should probably avoid touching the C++ code that clears viewFolder to avoid seamonkey impact.

Adding clarkbw in case he wants to make an authoritative UX decision on this.  If I force my gut to make a decision, it says we should not persist changes made while in a quick-search because the quick-search itself is not persisted (while the mail-view is), but my gut doesn't actually care.

Comment 6

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

This is an ugly enough regression that I think it wants to block 3.1.  Assigning to clarkbw, since he needs to make the UX decision described in comment 5.  Once that's made, I bet this bug wants to live with Andrew again.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

We're resetting the blocking flag for 3.1 on this bug and instead setting the wanted-thunderbird+ flag. We have too many blocking-3.1 bugs, to the point where it doesn't mean much, and managing the list is making it hard to actually work on closing bugs, which helps no one.

Thunderbird 3.1's primary purpose is to allow us to offer a prompted major update to Thunderbird 2 users, to ensure their continued ability to safely use Thunderbird.  Thunderbird 2 is built on an outdated version of Gecko, and our long-term ability to maintain the users' safety for Thunderbird 2 users is limited.

If you think this bug meets the requirements below, please renominate with a detailed explanation of how it meets the following two criteria, and we will reconsider.  To qualify, this bug must either:

a) make the upgrade experience from TB2 very painful for a large number of users

or

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

Just because this bug doesn't block TB3.1 doesn't mean it can't or won't make the release.  Once they're done with their blockers (if any), we encourage developers to keep working on non-blocking bugs, and to try to land them as early in the cycle as possible, as non-blocking bugs will become increasingly difficult to land in the later stages of the cycle.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

I am fixing this issue as part of bug 545955 on the basis of interface consistency.  Duping over to there.