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)

x86
All
defect
Not set
normal

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)

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.
WFM on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061205 Thunderbird/2.0b1 ID:2006120503
Attached patch Remove optimizations when view is unchanged (obsolete) (deleted) — Splinter Review
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.
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?
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).
(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?
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.
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.
(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.
Attached patch Don't try search when gSearchInput is null (obsolete) (deleted) — Splinter Review
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)
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
< 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?
> 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("");
> +function HaveViewChange(aFolder) Should be HaveViewChange(aFolder, aValue) etc.
Attached patch Revised patch per mnyromyr suggestions (obsolete) (deleted) — Splinter Review
This patch implements the suggestions from comment #12
Attachment #247780 - Attachment is obsolete: true
Attachment #247996 - Flags: review?(mscott)
Attachment #247780 - Flags: review?(mscott)
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 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+
(In reply to comment #16) > r=me if you move the var definition inside the if clause. Don't even need the variable!
> > 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... ;-)
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.
(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.
> 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."
Attachment #247996 - Attachment is obsolete: true
Attachment #247996 - Flags: review?(mscott)
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)
Attachment #247567 - Flags: superreview?(mscott)
Attachment #247567 - Attachment is obsolete: true
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 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 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?
Blocks: 362990
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+
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.
Whiteboard: [checkin needed] [checkin needed (1.8 branch)]
Attachment #247780 - Attachment is obsolete: true
approval-seamonkey1.1=biesi
Landed on trunk and MOZILLA_1_8_BRANCH.
Assignee: mscott → kent
Whiteboard: [checkin needed] [checkin needed (1.8 branch)]
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Depends on: 364060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: