Closed Bug 481824 Opened 15 years ago Closed 15 years ago

nsMsgDBView grouping logic is buggy, needs unit tests

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: asuth, Assigned: Bienvenu)

References

Details

(Whiteboard: [b3ux])

Attachments

(1 file, 1 obsolete file)

I am splitting the nsMsgDBView changes off from bug 474701.  Without these fixes, when using a search view, group-by logic is both crashy and incorrect.  

The provided patch provides a first-pass at a unit test that wants to at least verify my changes and the use case from bug 474701.

I was originally turfing this to bienvenu because I thought this was crashy, but it turns out that I don't know of any crashers with this patch (The crash I was seeing was because I had not updated the patch properly to account for changes on trunk.)  So now I'm turfing it because bienvenu hinted at wanting better searching logic, and because we really really need unit tests for nsMsgDBView and friends and I don't have time to take it further or to clean up the changes I have already made.  (nsMsgDBView has crazy many variants and permutations and needs coverage.)
Flags: blocking-thunderbird3?
The very limited set of changes on messageGenerator.js have been mooted by http://hg.mozilla.org/comm-central/rev/99320ed86776.  The rejections should be ignored.
Since this blocks [b3ux] work, it needs to be [b3ux] itself.  Getting it on the radar and targetting.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Whiteboard: [b3ux]
Target Milestone: --- → Thunderbird 3.0b3
I've been running with this a bit - I'll try to figure out why the db isn't valid.
Whiteboard: [b3ux] → [b3ux] [haspatch]
Do you mean why I needed to add m_db guards?  If so, it's because gloda is creating a search view which doesn't have an m_db.  We just cram stuff in via onSearchHit.
No, I meant why the db you try to get after getting the folder loaded notification  after parsing the msg db wasn't valid. aka the GAAAAAAAA comment :-)
updateFolder does not currently return an error when the db is invalid and will be reparsed, but it always sends a folderLoaded notification at the appropriate time (either immediately, or when the folder has been reparsed). I don't know when it stopped returning an error - perhaps jcranmer or rkent or I changed it at some point. But if I remove all the needToWait logic from mailTestUtils.js, then I can get rid of the timeout in mailTestUtils.js, and the one in test_nsMsgDBView.js, and all the tests pass. So I'm probably going to go with that approach.
Attached patch updated fixSplinter Review
this cleans up the test code so that we don't need settimeouts - I'm going to have a look through this patch and probably r/sr it if it looks ok, and land it since it's blocking the search bar stuff.
Attachment #365840 - Attachment is obsolete: true
Attachment #367230 - Flags: superreview+
Attachment #367230 - Flags: review+
patch checked in,  so other search bar stuff can be unblocked.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Whiteboard: [b3ux] [haspatch] → [b3ux]
Depends on: 616229
Depends on: 646901
Depends on: 620343
You need to log in before you can comment on or make changes to this bug.