Closed
Bug 362741
Opened 18 years ago
Closed 18 years ago
Selecting folder with previous view unread, while viewing another unread folder, fails.
Categories
(Thunderbird :: Mail Window Front End, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rkent, Assigned: rkent)
References
(Blocks 1 open bug)
Details
(Keywords: fixed-seamonkey1.1, fixed1.8.1.1)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: version 2 beta 1 (20061203)
In 2.0 beta1 version of Thunderbird (problem does not appear in 1.5.8) I get incorrect results if I select a view that was previously unread, while viewing another folder that is unread. The problem appears reliably in the nightly 2.0b1 download. Error console shows "gSearchInput has no properties chrome://messenger/content/searchBar.js line 622
Using a custom build with debug enabled and gViewDebug = true in commandglue.js, I get the following console output:
In ChangeFolderByURI uri = news://news.mozilla.org/mozilla.dev.tech.ldap sortTyp
e = 18
In reroot folder, sortType = 18viewType = 4
uri = gCurrentFolderToReroot, setting gQSViewIsDirty
uri == current loading folder uri
in folder loaded gVirtualFolderTerms = null
in folder loaded gMsgFolderSelected = news://news.mozilla.org/mozilla.dev.tech.l
dap
searching gDefaultSearchViewTerms and rerootingFolder
************************************************************
* Call to xpconnect wrapped JSObject produced this error: *
[Exception... "'[JavaScript Error: "gSearchInput has no properties" {file: "chro
me://messenger/content/searchBar.js" line: 622}]' when calling method: [nsIFolde
rListener::OnItemEvent]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_W
ITH_DETAILS)" location: "<unknown>" data: yes]
************************************************************
In the treeview of the messages, an incorrect result looks like this: "Unread" is shown as the view in the toolbar, but the actual messages shown are like the "All" view except one or more unread messages are not shown.
If I select "All" then "Unread" as the views while staying with the same folder selected, then I get a proper list of unread messages.
Reproducible: Always
Steps to Reproduce:
1. Select a folder to view, and set the view to "Unread"
2. Select a different folder, and set the view to "Unread"
3. Select again the original folder.
Actual Results:
Displayed list of folders resembles the All view, with one or more unread messages not appearing
Expected Results:
All unread messages, and only unread messages, appear in the message treeview pane.
This problem appears on RSS folders, news folders, and local folders. Not sure of IMAP folders.
Comment 1•18 years ago
|
||
WFM on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061205 Thunderbird/2.0b1 ID:2006120503
Assignee | ||
Comment 2•18 years ago
|
||
By removing a couple of cases where TB bypasses processing when the same view is displayed, I am able to make the problem go away. I don't understand the view code well enough to know if this should be the fix, or is just masking an underlying problem.
Comment 3•18 years ago
|
||
I can't reproduce this either (Windows 2000).
What's the setting for View | Threads for the folder that fails?
What about the three Sort-by settings? Does the other folder match this?
You're not touching the QuickSearch field, right?
Are either of the folders a Saved Search (virtual) folder?
Assignee | ||
Comment 4•18 years ago
|
||
I am not touching the Quicksort field. Neither folder is a virtual folder.
Here are some more details of a sample of the problem. I will use just two folders, the mozilla newsgroups m.d.a.thunderbird and m.d.t.ldap
On each folder, I set View/Sort By/Order Received, then View/Threads/Threads with Unread using the menus. At this point I can use the mouse to change from one folder to another, and all works OK (except the View says "All" in the toolbar.)
Now I select one of the folders, say Thunderbird news. Using the View toolbar, I change the view to Unread. Everything is fine. Now I select the Ldap newsgroup with the mouse. I do not get the error in the error console, however the view I see seems to be showing all messages. Using the View toolbar, I change the view to Unread. It now looks correct, showing me a single unread message (but NOT showing the thread parents). I switch back to the Thunderbird newsgroup, the View toolbar claims the view is "Unread", but I actually see all of the messages (and I get the error in the error console).
Comment 5•18 years ago
|
||
(In reply to comment #4)
> On each folder, I set View/Sort By/Order Received, then View/Threads/Threads
> with Unread using the menus. At this point I can use the mouse to change from
> one folder to another, and all works OK (except the View says "All" in the
> toolbar.)
Meaning, you expect the View to say something else? That setting is independent of the View|Threads setting. (See bug 237164 about the confusion between Messages|All/Unread and Threads|All/Unread.)
> Now I select one of the folders, say Thunderbird news. Using the View
> toolbar, I change the view to Unread. Everything is fine. Now I select the
> Ldap newsgroup with the mouse. I do not get the error in the error console,
> however the view I see seems to be showing all messages. Using the View
> toolbar, I change the view to Unread. It now looks correct, showing me a
> single unread message (but NOT showing the thread parents). I switch back to
> the Thunderbird newsgroup, the View toolbar claims the view is "Unread", but
> I actually see all of the messages (and I get the error in the error
> console).
Excellent set of STR. Unfortunately, I still can't reproduce: TB 2b1-1202, Win2K. Do you have extensions installed? If so, can you still reproduce after running Safe Mode?
Assignee | ||
Comment 6•18 years ago
|
||
I can reproduce with all extensions disabled. Unfortunately I cannot test in safe mode, because the View toolbar button and associated menu item are implemented with a standard extension (is that what you call them?) that goes away in safe mode.
I am not surprised that you cannot reproduce. This problem is severe enough that others would have been screaming about it if it was common. At least in theory I should be able to get to the root cause on my system, since I have a hard error and an earlier example that works. I'll try to do that, and then Scott and David can decide if the situation is likely to recur and worthy of a patch.
> Meaning, you expect the View to say something else?
I guess I was surprised that the View toolbar says "All" when I am clearly and correctly displaying a subset of messages that are related to "Unread" messages. But I see that confusion has been discussed in other bugs, so it is unrelated to this bug.
Assignee | ||
Comment 7•18 years ago
|
||
OK, I think I have now found the root cause.
First, to those trying to reproduce: before doing any of the tests, first remove the quick search toolbar item. I'll bet that you will then see the problem.
The basic issue is that these shortcuts that I deleted in the patch are referring to gSearchInput = document.getElementById("searchInput") But that item is the Quick Search toolbar, which can be disabled. When it is disabled, gSearchInput is null and trying to access its properties fails.
I can try to review the patch and see if I believe it is correct now that I understand the root cause, but it might be better for someone with more experience. Perhaps David or Scott can comment on this when someone is able to reproduce the problem.
Comment 8•18 years ago
|
||
(In reply to comment #7)
> First, to those trying to reproduce: before doing any of the tests, first
> remove the quick search toolbar item. I'll bet that you will then see the
> problem.
With that change, I do see the JavaScript console error, but I'm still getting the expected display in the Thread pane.
Assignee | ||
Comment 9•18 years ago
|
||
The changed line
if (gSearchInput && gCurrentViewValue == result && gDefaultSearchViewTerms)
is fairly clear, that is don't try a search if the search menu is not displayed.
The changes in ViewChange are required because that function is used to display UnRead views when the folder is changed. So the assumption in the comment "bail out early if the user picked the same view" is not relevant, since the function is being used in cases where the folder is being changed rather than the view. If we bail out early, then UnRead folders are not displayed correctly.
Attachment #247567 -
Attachment is obsolete: true
Attachment #247780 -
Flags: review?(mscott)
Comment 10•18 years ago
|
||
JFTR: I can confirm this with TB 2 beta 1 (20061205) on Linux.
> The changes in ViewChange are required because that function is used to
> display UnRead views when the folder is changed. So the assumption in the
> comment "bail out early if the user picked the same view" is not relevant,
> since the function is being used in cases where the folder is being changed
> rather than the view.
> If we bail out early, then UnRead folders are not displayed correctly.
I'm not quite comfortable with that change. Building views can be quite expensive, so we really don't want to do this everytime a view change is requested. We rather should check if the folder has changed as well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Assignee | ||
Comment 11•18 years ago
|
||
< We rather should check if the folder has changed as well.
It seems like the easiest way to do that would be to use separate upstream functions to ViewChange, some used by the view picker functions and others used by folder changes. These checks for unneeded changes could then be moved to the upstream functions. Unfortunately that change would be more extensive, and touch more files, than the simple change I proposed here.
As far as I can tell, the code I removed is only used when someone, from the view picker, selects the same view that is already displayed. You could make the argument that the only reason that they would do that (other than as a mistake) is when they want the folder reloaded, which the existing proposed patch would accomplish.
Could you briefly describe how you would like to see this change done?
Comment 12•18 years ago
|
||
> Could you briefly describe how you would like to see this change done?
The key problem is that folder changes aren't recognized in *both* places.
My approach would be (manual pseudo diff):
In msgViewPickerOverlay.js:
var gMailViewList = null;
+var gCurrentViewFolder = null;
...
// bail out early if the user picked the same view
+ var currentViewFolder = GetFirstSelectedMsgFolder();
+ if (!HaveViewChange(currentViewFolder))
return;
// persist the view
+ gCurrentViewFolder = currentViewFolder;
gCurrentViewValue = aValue;
gCurrentViewLabel = aLabel;
...
+function HaveViewChange(aFolder)
+{
+ return (gCurrentViewFolder != aFolder) || (gCurrentViewValue != aValue);
+}
+
function GetFolderInfo(aFolder)
In msgMail3PaneWindow.js:
// for the old view, just re-use it
var result = GetMailViewForFolder(msgFolder);
+if (gSearchInput && gDefaultSearchViewTerms && !HaveViewChange(msgFolder))
{
viewDebug("searching gDefaultSearchViewTerms and rerootingFolder\n");
Search("");
Comment 13•18 years ago
|
||
> +function HaveViewChange(aFolder)
Should be HaveViewChange(aFolder, aValue) etc.
Assignee | ||
Comment 14•18 years ago
|
||
This patch implements the suggestions from comment #12
Attachment #247780 -
Attachment is obsolete: true
Attachment #247996 -
Flags: review?(mscott)
Attachment #247780 -
Flags: review?(mscott)
Comment 15•18 years ago
|
||
I did some searches to update the message views tracking bug (please see http://wiki.mozilla.org/User:Mnyromyr:MessageViews also) and found that reloading an already selected view would indeed come handy in certain circumstances. This could easily be achieved by Kent's proposal of dropping the early exit in ViewChange - so there's still just this question: would that hurt perf (much) by automatic calls or the like?
ViewChange is called (indirectly) by:
- View -> Messages menu and View dropdown: no automatism
- SwitchView in commandglue.js is only called indirectly by "cmd_viewIgnoredThreads" and when resorting the thread pane due to user interaction: no automatism
- msgMail3PaneWindow.js whenever a folder is loaded - but that's what we want here
- we finished a search in searchBar.js::onSearchDone: should be save, too
Sorry, Kent, for the confusion.
Scott, what do you think?
Blocks: mailviews
Comment 16•18 years ago
|
||
Comment on attachment 247567 [details] [diff] [review]
Remove optimizations when view is unchanged
> var result = GetMailViewForFolder(msgFolder);
...
>+ if (document.getElementById("mailviews-container")) // only load the folder view if the views toolbar is visible
> {
> viewDebug("changing view by value\n");
> ViewChangeByValue(result);
r=me if you move the var definition inside the if clause.
Attachment #247567 -
Attachment is obsolete: false
Attachment #247567 -
Flags: superreview?(mscott)
Attachment #247567 -
Flags: review+
Comment 17•18 years ago
|
||
(In reply to comment #16)
> r=me if you move the var definition inside the if clause.
Don't even need the variable!
Comment 18•18 years ago
|
||
> > r=me if you move the var definition inside the if clause.
>
> Don't even need the variable!
Granted, but I'm not much of a fan of nested function calls. They are a real pita to debug... ;-)
Assignee | ||
Comment 19•18 years ago
|
||
The reviewed patch (Created 2006-12-05 - how do I link to specific patches in comments?) does not include the fixes in the parallel code for SeaMonkey, unlike the later patches. I would like to revise this, and also move the var as suggested. Is there some reason that you reviewed the first patch, and not the second which was more complete?
What is the protocol here for the patch management? Do I as the original patch submitter submit a new patch and ask for review, or edit the old one? Or will changes get done by the people checking in the patch? I'm a little confused about who is responsible for implementing changes on a review+ with changes requested.
Comment 20•18 years ago
|
||
(In reply to comment #19)
> The reviewed patch (Created 2006-12-05 - how do I link to specific patches in
> comments?)
If you click the edit attachment link, and add comments there, bugzilla adds the (From update of attachment xx [edit]). It also autolinks for normal comments if you write the word "attachment" followed by the number.
If nobody says they will make the change before checking in (usually for tiny corrections), you submit an updated patch, and mark the old one as obsolete.
Comment 21•18 years ago
|
||
> The reviewed patch (Created 2006-12-05 - how do I link to specific patches in
> comments?) does not include the fixes in the parallel code for SeaMonkey,
> unlike the later patches.
That was an oversight on my side, SM should be patched accordingly, of course. :)
> I would like to revise this, and also move the var
> as suggested. Is there some reason that you reviewed the first patch, and not
> the second which was more complete?
If we check for "folder and view changed" like in the third patch, this would result to "false" in this code path, we're there just _because_ the folder changed! (Correct me if I'm wrong - with the third patch you never touch the Search("") call, right?)
And code never called is almost always no good. ;-)
> Do I as the original patch submitter submit a new patch and ask for review,
That's the usual way.
> Or will changes get done by the people checking in the patch?
Only if they say so and usually only for very minor changes.
> I'm a little confused about who is responsible for implementing changes on a
> review+ with changes requested.
Such a "conditional r+" means "Make a new patch with the required changes, attach it and set the r+ yourself, it's okay by me. Request additional (super)reviews if needed."
Assignee | ||
Updated•18 years ago
|
Attachment #247996 -
Attachment is obsolete: true
Attachment #247996 -
Flags: review?(mscott)
Assignee | ||
Comment 22•18 years ago
|
||
Comment on attachment 247780 [details] [diff] [review]
Don't try search when gSearchInput is null
Thanks to all for the comments on protocol. However, in reviewing the three patches, it seems to me like this patch (the second) is already fully correct. It does not need the "var ..." moved inside the if, it includes SM, and it allows manual reloading of a view as suggested in comment #15. So I un-obsolete it, and request review again.
Attachment #247780 -
Attachment is obsolete: false
Attachment #247780 -
Flags: review?(mnyromyr)
Assignee | ||
Updated•18 years ago
|
Attachment #247567 -
Flags: superreview?(mscott)
Assignee | ||
Updated•18 years ago
|
Attachment #247567 -
Attachment is obsolete: true
Comment 23•18 years ago
|
||
Kent is right that you shouldn't move the declaration of result - it's used in the if (test), and in the else {} code. So, Karsten, does that patch seem OK to you? We'd like to fix this for TB 2.0 soon...thx!
Comment 24•18 years ago
|
||
Comment on attachment 247780 [details] [diff] [review]
Don't try search when gSearchInput is null
>Index: mail/base/content/msgMail3PaneWindow.js
>===================================================================
>- if (gCurrentViewValue == result && gDefaultSearchViewTerms)
>+ if (gSearchInput && gCurrentViewValue == result && gDefaultSearchViewTerms)
>Index: mailnews/base/resources/content/msgMail3PaneWindow.js
>===================================================================
>- if (gCurrentViewValue == result && gDefaultSearchViewTerms)
>+ if (gSearchInput && gCurrentViewValue == result && gDefaultSearchViewTerms)
You should use
GetSearchInput()
instead of
gSearchInput
to avoid any startup issues.
r=me with that.
Attachment #247780 -
Flags: review?(mnyromyr) → review+
Comment 25•18 years ago
|
||
Comment on attachment 247780 [details] [diff] [review]
Don't try search when gSearchInput is null
Kent, let us know if you need help getting this checked in. Seems like this would be a good candidate for the branch too right?
Attachment #247780 -
Flags: superreview+
Attachment #247780 -
Flags: approval-thunderbird2?
Comment 26•18 years ago
|
||
Comment on attachment 247780 [details] [diff] [review]
Don't try search when gSearchInput is null
we'll need this bug on the branch along with Bug 362990.
Attachment #247780 -
Flags: approval-thunderbird2? → approval-thunderbird2+
Assignee | ||
Comment 27•18 years ago
|
||
Implements request from comment #24. This should be the patch to checkin.
Due to my impending move, I will be offline for an indeterminate amount of time. I would appreciate if someone else would take this patch from here and get it checked it.
Updated•18 years ago
|
Whiteboard: [checkin needed] [checkin needed (1.8 branch)]
Updated•18 years ago
|
Attachment #247780 -
Attachment is obsolete: true
Comment 28•18 years ago
|
||
approval-seamonkey1.1=biesi
Comment 29•18 years ago
|
||
Landed on trunk and MOZILLA_1_8_BRANCH.
Assignee: mscott → kent
Whiteboard: [checkin needed] [checkin needed (1.8 branch)]
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Keywords: fixed-seamonkey1.1,
fixed1.8.1.1
You need to log in
before you can comment on or make changes to this bug.
Description
•