189007 - Switch group without clearing QuickSearch resets view (to All from Threads with Unread)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[jean-max Reymond]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030110
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030110

When I use the field  "Subject or sender contains", it erases the
"View/Message/Threads with unread" option and set automatically to All

Reproducible: Always

Steps to Reproduce:
1. In View/Message, select the Threads with unread option
2. search a news whith Subject or sender contains
3. change to another newsgroup
4. go back to the previous document
5. The View/Message is set to All

Actual Results:  
erase my preference

Expected Results:  
preserve my preference

Comment 1

[laurel]

Just more info:
-- The view is not reset upon clearing QuickSearch and staying in the same
folder/group
-- The view is not reset switching folder/group without using QuickSearch
-- In this case the view truly does reset to All (we haven't fully integrated
the Threaded views and new mailviews menus and sometimes the Views dropdown is
out of sync with dropdown -- see bug 187990)

Comment 2

[Samir Gehani]

Mail triage team: nsbeta1-

Comment 3

[Frank Schönheit]

this seems to be a regression from the fix for bug 163773, so I'm cc'ing the
author of the fix ....
I'd be interested in fixing this (it's annoying), but I am not sure 'bout the
best way to do so. I'd have a small patch which improves the situation so that
the view is not reset when the folder is left without a current quciksearch term
(i.e. the users needs to reset the quicksearch before leaving the folder). For
the full solution, I'd have an idea (make the qucik search view _not_ storing
the viewType and viewFlags in the FolderInfo), but not sure if it's a valid
approach.

Comment 4

[(not reading, please use [e-mail] instead)]

neil says:  "I think that opening a view shouldn't change the default view type
for a folder, instead the code to set the view type should be in JS, probably in
SwitchView, what do you think?"

I think his idea sounds reasonable.  
doing a QS should not change the default view type.
and I agree, neither should switching the mail view.

right now, we seem to save them off in nsMsgDBView::Open(), and we also have
nsMsgDBView::SaveSortInfo (not scriptable, but we probably should make it
scriptable and part of the interface, for the fix for this bug) which we call
when you sort.

but changing the sort (in a view, or after a QS, doesn't matter) should persist.

Comment 5

[Frank Schönheit]

Seth, thanks for the comments. I'll try to go the way you sketched (wasn't aware
of the sort thingie, thanks for pointing out) as soon as I have some free time
slot ...

Comment 6

[[e-mail]]

*** Bug 195741 has been marked as a duplicate of this bug. ***

Comment 7

[[e-mail]]

Attachment: Proposed patch

Comment 8

[[e-mail]]

Comment on attachment 120694 [details] [diff] [review]
Proposed patch

Reviews, anyone?

Comment 9

[[e-mail]]

Oh, and if this patch doesn't work you can have the full patch where I
completely rewrote Sort() instead :-P

Comment 10

[[e-mail]]

Attachment: Forgot to include nsThreadedDBView.cpp last time :-(

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 120694 [details] [diff] [review]
Proposed patch

clearing sr request, since this patch is obsoleted.

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 120694 [details] [diff] [review]
Proposed patch

obsolete, clearing request

Comment 13

[[e-mail]]

Attachment: Fixed malformed patch

Last patch was malformed, sorry.

Comment 14

[Mike Cowperthwaite]

*** Bug 198478 has been marked as a duplicate of this bug. ***

Comment 15

[parish]

Neil: you may need to look at this again. I have had your patch in my loacal
tree for a few weeks and it's worked fine until the last couple of days.

What happens is that when you click the View dropdown Moz crashes in
gklayout.dll. This happens with Classic (and also Skypilot) but doesn't /appear/
to affect Modern as long as you delete localstore.rdf and XUL.mfl after
switching to Modern but before restarting Moz.

I backed out the patch and it works OK with all themes (after deleting
localstore.rdf and XUL.mfl).

Same problem using a new profile.

Could it be the fix for bug 189490 (View state persists across sessions) that
has broken your patch?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030510

Comment 16

[parish]

Hmmm, 10 minutes after my last comment it did the same thing in my build without
your patch, although I had been using it for about half an hour without problems.

Deleted loaclastore.rdf and XUL.mfl, restarted and it is behaving again, but for
how long?

I'm going to add a comment in bug 189490 but it seems that it is worse with your
patch.

Comment 17

[Mike Cowperthwaite]

I wouldn't mind seeing this bug fixed, it's one of the long-standing problems in 
Mail/News that I still have to work around periodically: every time I use a 
quick-search in news (where my default is Threads with Unread) I have to go back 
and re-select my choice from the View|Threads submenu.

Comment 1 states: "The view is not reset upon clearing QuickSearch and staying 
in the same folder/group" -- but in fact, clearing Quicksearch does revert to 
the original view.  It only loses the default view on returning to the folder.  
However, if you re-select the desired view from the menu (even tho it is already 
selected) *before* leaving the folder, it will stick.

Re: Seths' comment 4: "doing a QS should not change the default view type [for a 
folder].  and I agree, neither should switching the mail view."
The mail-view part of that statement is obsoleted now, isn't it, since Mail-View 
is now the same thing as the View|Messages menu?

Is this bug related to the more recent bug 206131, which was brought about by 
the alignment of MailViews and View|Messages?

Comment 18

[Frank Schönheit]

Neil, your patch from attachment 122196 [details] [diff] [review] worked fine, I think it only needs some
polishing to update it to the recent code base. Would be great if you'd find
some time to update it to head.
Since Seth seems to be a busy man recently, perhaps we can approach somebody
else for r/sr?

Comment 19

[[e-mail]]

Attachment: Hopefully updated for bitrot

I've got some other patches making it difficult to update this patch :-(
I hope I got it right...

Comment 20

[Frank Schönheit]

Neil, sorry, didn't find the time to try this 'til now.

Unfortunately, the new patch works in some situations only. The very original
situation as described in the bug is fixed. However, what still gets lost if you
switch the view with an "active quicksearch" is
- the "View|Messages|Threaded" setting
- the "View|Threads|Unread" setting
("View|Threads|Threads with Unread" does persist).

In your first patch, both things still worked. The reason that it doesn't work
with the new patch is that in nsMsgDBView::SaveSortInfo, now also the view flags
are stored in the FolderInfo - in your first patch, this wasn't the case. Since
the view flags control the two settings above, and since the quicksearch view
resets the view flags to 0, the bug still happens.

Uncommenting the line
  folderInfo->SetViewFlags(m_viewFlags);
in nsMsgDBView::SaveSortInfo solves this, but I assume this line is in for a
reason? Perhaps we can condition it with a

  nsMsgViewTypeValue viewType;
  nsresult rv = GetViewType(&viewType);
  NS_ENSURE_SUCCESS(rv, rv);
  if (viewType != nsMsgViewType::eShowQuickSearchResults)
    ...

? I suppose this would be valid, since the quick search view seems to reset the
flags to 0, anyway, so there's no gain in storing this 0.

Comment 21

[[e-mail]]

>Uncommenting the line
>  folderInfo->SetViewFlags(m_viewFlags);
>in nsMsgDBView::SaveSortInfo solves this
Doh, I missed one? Actually that might not be enough, if you change the sort
order while in quick search :-( What I'll have to do is to set the view flags
only for threaded views. David, you've really mucked me up here :-/ Perhaps it
would be easier to rewrite sorting from scratch ;-)

Comment 22

[David :Bienvenu]

I'm a little confused why this is so difficult. Quicksorts shouldn't persist the
sort or view at all. I'll look into this.

Comment 23

[[e-mail]]

Attachment: Another try

OK, so this one only lets quick search views save sort info, not view info.

Comment 24

[Frank Schönheit]

this patch works, thanks!
(small issue: it seems you forgot to include nsMsgThreadedDBView.h in the diff,
I had to manually add the declaration of SaveViewInfo and SaveSortInfo after
applying)

Comment 25

[[e-mail]]

Attachment: Added missing header

Oops :-[

Comment 26

[David :Bienvenu]

Comment on attachment 134164 [details] [diff] [review]
Added missing header

this looks ok

Comment 27

[David :Bienvenu]

Attachment: alternate fix

as Frank mentioned, this will also fix the problem. This could be extended to
not save sort info either, but that's optional, I guess.

Comment 28

[[e-mail]]

Comment on attachment 134213 [details] [diff] [review]
alternate fix

Actually you'd probably want to save sort info, but not view flags.

Comment 29

[David :Bienvenu]

yeah, I tend to agree. So I think my patch would be sufficient.

Comment 30

[[e-mail]]

Comment on attachment 134213 [details] [diff] [review]
alternate fix

Um... I said not view flags... your patch currently only doesn't save view
type.

Comment 31

[David :Bienvenu]

oh, got it, I'll fix that.

Comment 32

[David :Bienvenu]

Attachment: fix adressing neil's comment

Comment 33

[Frank Schönheit]

ehm - David, your latest fix accesses |viewType| before it has been initialized
- it thus doesn't work. Moving the GetViewType before the "if" fixes this bug here.

(Honestly, I like Neil's patch more :), because it makes the saving of flags,
type etc an explicit action instead of this "implicitly do it on open", and
because there no base classes need to know about their derived classes - which
somehow is the case with the newly introduced "if I'm no quick search view, then
...". But that's only my very personal taste :))

Could you both agree which patch should make it, so we can start some r/sr?
Would be great :).

Comment 34

[David :Bienvenu]

thx for catching that, Frank. I'll attach a new patch.

Neil's patch is OK, but here's why I prefer my approach:

1. It's simpler, so it's less likely to cause regressions
2. I like saving the settings in the c++ code, in case the js files fork between
Mozilla Suite and Tbird, for example.

Checking the view type doesn't make me that happy either...an other way  to fix
that is to add a virtual method to save those settings, and make
nsMsgQuickSearchDBView override that method, and not do anything.

Comment 35

[Frank Schönheit]

okay, especially 2 sounds like a strong point. And yes, a virtual method is also
what I thought would be cleanest, but probably isn't worth it here ... Not sure.

Comment 36

[[e-mail]]

Would moving the save view type setting to nsMsgThreadedDBView::Init help?

Comment 37

[[e-mail]]

Attachment: Yet another approach

Comment 38

[David :Bienvenu]

Comment on attachment 134363 [details] [diff] [review]
Yet another approach

sure, that works, r=bienvenu

Comment 39

[Frank Schönheit]

the beauty of simplicity ... who are we going to approach for sr? :)

Comment 40

[Mike Cowperthwaite]

*** Bug 197900 has been marked as a duplicate of this bug. ***

Comment 41

[Henry Jia]

Comment on attachment 134363 [details] [diff] [review]
Yet another approach

sr=Henry

Comment 42

[[e-mail]]

Fix checked in.

Comment 43

[Mike Cowperthwaite]

Working as expected in
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031114

Yay.

Comment 44

[Mike Cowperthwaite]

Could the fix for this bug have also fixed bug 217329?

Comment 45

[Mike Cowperthwaite]

*** Bug 230583 has been marked as a duplicate of this bug. ***