After delete or move the top Message, Layout of the Message Pane breaks, Group Headline disappear

RESOLVED FIXED in Thunderbird 3.0b3


MailNews Core
9 years ago
2 years ago


(Reporter: Helmut Hartmann, Assigned: Bienvenu)


(Blocks: 1 bug)

Thunderbird 3.0b3
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(6 attachments)



9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv: Gecko/2009022800 SUSE/3.0.7-1.2 Firefox/3.0.7
Build Identifier: Delete or Move Message breaks Layout Message Pane

Layout of Message Pane ist sort by date descending
Group by sort is on

when i delete the first mail under the group headline, the headline disappear. after switch to another folder and back the layout looks fine.

Reproducible: Always

Steps to Reproduce:
1. sort by date
2. sort by group first message under group headline 
4.delete message
5.headline dissappear
6.switch to another folder and back headline is shown

Thunderbird: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090308 Lightning/1.0pre Shredder/3.0b3pre

OS: Opensuse 11.1
X: KDE 4.2

Comment 1

9 years ago
Created attachment 366266 [details]
Layout before delete Message


9 years ago
Attachment #366266 - Attachment mime type: image/png → image/jpg

Comment 2

9 years ago
Created attachment 366267 [details]
Layout after delete Message


9 years ago
Attachment #366266 - Attachment mime type: image/jpg → image/jpeg


9 years ago
Version: unspecified → Trunk


9 years ago
OS: Linux → All

Comment 3

9 years ago
problem reproduced using this build on OpenSolaris 5.11 i86pc

thunderbird-3.0b3pre.en-US.solaris2.11-i386.tar.bz2	29-Mar-2009 12:38 	 17M

version 3.0b3pre
Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9.1b4pre) Gecko/20090329
Ever confirmed: true

Comment 4

9 years ago
I think this was caused by the changes to the view code for gloda-based search views that I landed recently.  In particular, GetIndexOfFirstDisplayedKeyInThread is returning the dummy row index, not the row index for the message. This throws the rest of the code in nsMsgGroupView::OnHdrDeleted off. I'll go back and look at that change again...
Assignee: nobody → bienvenu
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b3

Comment 5

9 years ago
Created attachment 370667 [details] [diff] [review]
proposed fix

This makes nsMsgDBView::FindHdr do the right thing if we're not allowing dummies, and makes nsMsgGroupView::OnHdrDeleted realize that the viewThreadIndex is now the dummy row index.

This really needs a test case, along the lines of creating a view with a few messages, grouping by date, and verifying that removing/adding headers produces the right msg hdr and view flags in the view. We would need to tweak the dates on the msg hdrs using SetDate in order to produce groups with existing test messages, but that's easy.
Attachment #370667 - Flags: superreview?(neil)
Attachment #370667 - Flags: review?(bugmail)
Flags: in-testsuite?
Created attachment 370836 [details] [diff] [review]
v1 unit test

This unit test does what you say, methinks.  One limitation on unit testing the views that we probably want to deal with is that getCellText cannot be (easily) used.  nsMsgDBView and friends use getIdConst which is not scriptable, which means we can't just dummy-out the column definition.  Brief investigation suggests that nsTreeColumns and nsTreeColumn are too tied into the UI framework to be directly instantiated on their own.

Possibilities to resolve that are to 1) add a slow-path but scriptable for GetCellText on nsMsgDBView and children wherein we can just pass the column id as a string, or 2) create our own nsITreeColumn impl for testing purposes that does the right thing for getIdConst.

The getCellText limitation is not a problem for this unit test, it's just a heads up limitation should we want to verify that the dummy header is saying the right thing, etc.  We could test that with mozmill but I feel like that testing should still be in xpcshell land.
Attachment #370836 - Flags: review?(bienvenu)
Comment on attachment 370836 [details] [diff] [review]
v1 unit test

I add some more unit testing framework, Standard8 should take a look...
Attachment #370836 - Flags: review?(bugzilla)
Attachment #370667 - Flags: review?(bugmail) → review+
Comment on attachment 370667 [details] [diff] [review]
proposed fix

passes unit test
Component: Mail Window Front End → Backend
Product: Thunderbird → MailNews Core
QA Contact: front-end → backend


9 years ago
Attachment #370667 - Flags: superreview?(neil) → superreview+

Comment 9

9 years ago
Comment on attachment 370836 [details] [diff] [review]
v1 unit test

excellent, thx!

The column stuff doesn't worry me that much since if we get the hdrs and flags and levels right, displaying the cells is generally not a big problem area. But we can certainly expose it somehow.
Attachment #370836 - Flags: review?(bienvenu) → review+

Comment 10

9 years ago
fixed (modulo the unit test). I checked that the unit test fails w/o this patch, and succeeds with it.
Last Resolved: 9 years ago
Resolution: --- → FIXED
Attachment #370836 - Flags: review?(bugzilla) → review+
Comment on attachment 370836 [details] [diff] [review]
v1 unit test

> // this will be migrated out of gloda soon...
> load("../../mailnews/resources/messageGenerator.js");

nit: please remove the comment that I obviously forgot to remove.

>+function async_run(aArgs) {
>+  let result = aArgs.func.apply(aArgs.dis || null, aArgs.args || []);
>+  if (result && {
>+    asyncGeneratorStack.push(result);
>+    return _async_driver();
>+  }
>+  else {
>+    if (result === undefined)
>+      return true;
>+    else
>+      return result;
>+  }

nit: I'd prefer just "return (result === undefined);" here. If you don't like that, then at least drop the else clauses which aren't necessary due to the returns.
This change is potentially bit by bug 487551 in nsTArray.IndexOf.  The unit test fails intermittently for me because it is dependent on non-initialized (or rather non-deterministically-initialized) data in the array.

Reopening because we should either work-around the problem or wait for it to land on 1.9.1 or something.
Depends on: 487551
Resolution: FIXED → ---
Now that bug 487551 is fixed on 1.9.1, I have landed the unit test.
Created attachment 373334 [details] [diff] [review]
ThreadInexOfMsgHdr's FindHdr call should probably allow dummies - checked in.

The call to GetIndexOfFirstDisplayedKeyInThread allows dummies, so the call to FindHdr should probably allow dummies too...

(Right now, for the grouped case that this bug is fixing, what actually happens is the FindHdr call fails because all that exists is the dummy, but then the GetIndexOfFirstDisplayedKeyInThread gets called and it is okay with the dummy.)
Attachment #373334 - Flags: review?(bienvenu)
Attachment #373334 - Attachment is patch: true
Attachment #373334 - Attachment mime type: application/octet-stream → text/plain
Created attachment 373631 [details] [diff] [review]
fix unit test to use last week bucket rather than unreliably using last day

The unit test was failing when run between 1a-2a because the group-by-date logic quantizes to the current day (and does not use a sliding window, which I somehow assumed it was using, despite having read that code before.)  This change makes the unit test use the 'last week' bucket in such a way that the test will never break because of the current time or (reasonable) changes in the current time.
Attachment #373631 - Flags: review?(bugzilla)
Attachment #373631 - Flags: review?(bugzilla) → review+
pushed revised unit test:

Comment 17

9 years ago
Comment on attachment 373334 [details] [diff] [review]
ThreadInexOfMsgHdr's FindHdr call should probably allow dummies - checked in.

sorry for the delay...
Attachment #373334 - Flags: review?(bienvenu) → review+

Comment 18

9 years ago
I've landed Andrew's patch to get this bug off the blocking list.
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED


9 years ago
Attachment #373334 - Attachment description: ThreadInexOfMsgHdr's FindHdr call should probably allow dummies → ThreadInexOfMsgHdr's FindHdr call should probably allow dummies - checked in.
Attachment #373334 - Flags: superreview+
Blocks: 725948
Flags: in-testsuite?


2 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.