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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Helmut Hartmann]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.0.7) 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
3.select 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
Kernel: 2.6.27.19-3.2-pae
X: KDE 4.2

Comment 1

[Helmut Hartmann]

Attachment: Layout before delete Message

Comment 2

[Helmut Hartmann]

Attachment: Layout after delete Message

Comment 3

[zivamo]

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
from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/contrib/latest-trunk/

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

Comment 4

[David :Bienvenu]

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...

Comment 5

[David :Bienvenu]

Attachment: 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.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Attachment: 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.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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

I add some more unit testing framework, Standard8 should take a look...

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 370667 [details] [diff] [review]
proposed fix

passes unit test

Comment 9

[David :Bienvenu]

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.

Comment 10

[David :Bienvenu]

fixed (modulo the unit test). I checked that the unit test fails w/o this patch, and succeeds with it.

Comment 11

[Mark Banner (:standard8)]

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

> // this will be migrated out of gloda soon...
> load("../../mailnews/resources/messageGenerator.js");
>+load("../../mailnews/resources/asyncTestUtils.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 && result.next) {
>+    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.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Now that bug 487551 is fixed on 1.9.1, I have landed the unit test.
http://hg.mozilla.org/comm-central/rev/1118ea4c545c

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Attachment: 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.)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Attachment: 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.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

pushed revised unit test:
http://hg.mozilla.org/comm-central/rev/e566900c420a

Comment 17

[David :Bienvenu]

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

sorry for the delay...

Comment 18

[David :Bienvenu]

I've landed Andrew's patch to get this bug off the blocking list.