Closed
Bug 189007
Opened 22 years ago
Closed 21 years ago
Switch group without clearing QuickSearch resets view (to All from Threads with Unread)
Categories
(SeaMonkey :: MailNews: Message Display, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jmreymond, Assigned: sspitzer)
References
Details
Attachments
(3 files, 6 obsolete files)
13.13 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
869 bytes,
patch
|
Details | Diff | Splinter Review | |
2.39 KB,
patch
|
Bienvenu
:
review+
Henry.Jia
:
superreview+
|
Details | Diff | Splinter Review |
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: nsbeta1
Summary: using the fast search erase my view preferences → Switch group without clearing QuickSearch resets view (to All from Threads with Unread)
Comment 3•22 years ago
|
||
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.
Assignee | ||
Comment 4•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
*** Bug 195741 has been marked as a duplicate of this bug. ***
Comment 7•21 years ago
|
||
Comment 8•21 years ago
|
||
Comment on attachment 120694 [details] [diff] [review] Proposed patch Reviews, anyone?
Attachment #120694 -
Flags: superreview?(sspitzer)
Attachment #120694 -
Flags: review?
Comment 9•21 years ago
|
||
Oh, and if this patch doesn't work you can have the full patch where I completely rewrote Sort() instead :-P
Comment 10•21 years ago
|
||
Attachment #120694 -
Attachment is obsolete: true
Assignee | ||
Comment 11•21 years ago
|
||
Comment on attachment 120694 [details] [diff] [review] Proposed patch clearing sr request, since this patch is obsoleted.
Attachment #120694 -
Flags: superreview?(sspitzer)
Assignee | ||
Comment 12•21 years ago
|
||
Comment on attachment 120694 [details] [diff] [review] Proposed patch obsolete, clearing request
Attachment #120694 -
Flags: review?
Comment 13•21 years ago
|
||
Last patch was malformed, sorry.
Attachment #120841 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #122196 -
Flags: superreview?(sspitzer)
Attachment #122196 -
Flags: review?(sspitzer)
Comment 14•21 years ago
|
||
*** Bug 198478 has been marked as a duplicate of this bug. ***
Comment 15•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
I've got some other patches making it difficult to update this patch :-( I hope I got it right...
Updated•21 years ago
|
Attachment #122196 -
Attachment is obsolete: true
Comment 20•21 years ago
|
||
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•21 years ago
|
||
>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•21 years ago
|
||
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•21 years ago
|
||
OK, so this one only lets quick search views save sort info, not view info.
Attachment #132347 -
Attachment is obsolete: true
Comment 24•21 years ago
|
||
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)
Updated•21 years ago
|
Attachment #134164 -
Flags: review?(bienvenu)
Comment 26•21 years ago
|
||
Comment on attachment 134164 [details] [diff] [review] Added missing header this looks ok
Attachment #134164 -
Flags: review?(bienvenu) → review+
Comment 27•21 years ago
|
||
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•21 years ago
|
||
Comment on attachment 134213 [details] [diff] [review] alternate fix Actually you'd probably want to save sort info, but not view flags.
Comment 29•21 years ago
|
||
yeah, I tend to agree. So I think my patch would be sufficient.
Comment 30•21 years ago
|
||
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•21 years ago
|
||
oh, got it, I'll fix that.
Comment 32•21 years ago
|
||
Updated•21 years ago
|
Attachment #134213 -
Attachment is obsolete: true
Comment 33•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Would moving the save view type setting to nsMsgThreadedDBView::Init help?
Comment 37•21 years ago
|
||
Comment 38•21 years ago
|
||
Comment on attachment 134363 [details] [diff] [review] Yet another approach sure, that works, r=bienvenu
Attachment #134363 -
Flags: review+
Comment 39•21 years ago
|
||
the beauty of simplicity ... who are we going to approach for sr? :)
Comment 40•21 years ago
|
||
*** Bug 197900 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Attachment #122196 -
Flags: superreview?(sspitzer)
Attachment #122196 -
Flags: review?(sspitzer)
Comment 41•21 years ago
|
||
Comment on attachment 134363 [details] [diff] [review] Yet another approach sr=Henry
Attachment #134363 -
Flags: superreview+
Comment 42•21 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 43•21 years ago
|
||
Working as expected in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031114 Yay.
Comment 44•21 years ago
|
||
Could the fix for this bug have also fixed bug 217329?
Comment 45•21 years ago
|
||
*** Bug 230583 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•