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

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)

Mail triage team: nsbeta1-

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.

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.

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 ...

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

Comment on attachment 120694 [details] [diff] [review] Proposed patch Reviews, anyone?

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

Comment on attachment 120694 [details] [diff] [review] Proposed patch clearing sr request, since this patch is obsoleted.

Comment on attachment 120694 [details] [diff] [review] Proposed patch obsolete, clearing request

Last patch was malformed, sorry.

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

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

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.

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?

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?

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

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.

>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 ;-)

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.

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

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)

Oops :-[

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

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 on attachment 134213 [details] [diff] [review] alternate fix Actually you'd probably want to save sort info, but not view flags.

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

Comment on attachment 134213 [details] [diff] [review] alternate fix Um... I said not view flags... your patch currently only doesn't save view type.

oh, got it, I'll fix that.

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 :).

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.

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.

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

Comment on attachment 134363 [details] [diff] [review] Yet another approach sure, that works, r=bienvenu

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

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

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

Fix checked in.

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

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

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