211289 - select all / delete leaves the thread pane in a bad state (dots in thread pane)

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[(not reading, please use [e-mail] instead)]

select all / delete leaves the thread pane in a bad state

I think this will end up be a regression of mine, from my fix for bug #205877

the comment from that checkin:

"when you toggle junk status from thread pane icon,
and you've got it set to move on manual mark, assertion and painting problems.

the problem is that ::OnDeleteCompleted() uses the selection to determine which
view indices to call with ::NoteChange().

the junk column icon triggers a delete (or move) and that row might not be selected.

instead, we save of the indices (we can't save keys, as the keys are already removed
before ::OnDeleteCompleted() is called), and use the saved indices when calling
::NoteChange()"

from the console, when I get this problem:

###!!! ASSERTION: selected indices is not equal to num of msg selected!!!: 'numS
elected == selection.GetSize()', file c:/trees/trunk/mozilla/mailnews/base/src/n
sMsgDBView.cpp, line 993

it's not a simple painting issue:  min-max the window doesn't fix it, nor does
resizing horizontally.

interesting resizing vertically does.

my wild pre-debugging guess:  something must be off with the row count?

Comment 1

[(not reading, please use [e-mail] instead)]

since I think bug #205877 introduced this, and that's one the 1.4 branch, I'd
want this fix (when we have it) for the 1.4 branch too.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 3

[Jason M. Abels]

Is this at all a duplicate of bug 169481?

Comment 4

[Matthias Versen [:Matti]]

no dupe because this is a very recent regression.

Comment 5

[James Rome]

*** Bug 213294 has been marked as a duplicate of this bug. ***

Comment 6

[James Rome]

This also gets triggered when you ask an IMAP box to be viewed by thread.
It is impossible to find it using the summary of the bug. I would suggest the
summary be enhanced.

Comment 7

[Matthias Versen [:Matti]]

*** Bug 213752 has been marked as a duplicate of this bug. ***

Comment 8

[R.K.Aa.]

are these duplicates? bug 213401, bug 212388

Comment 9

[Matthias Versen [:Matti]]

*** Bug 213401 has been marked as a duplicate of this bug. ***

Comment 10

[Matthias Versen [:Matti]]

*** Bug 212388 has been marked as a duplicate of this bug. ***

Comment 11

[Mike Cowperthwaite]

*** Bug 214247 has been marked as a duplicate of this bug. ***

Comment 12

[Scott MacGregor]

*** Bug 214308 has been marked as a duplicate of this bug. ***

Comment 13

[[e-mail]]

I've noticed that Empty Trash asserts in debug builds.

Most thread pane view changes are implemented by creating a new view object,
which works well. However, other changes should always call RowCountChanged.

There is, however, another workaround: the batching mechanism that Jan used to
improve the performance of multiple RowCountChanged calls (I have a patch in bug
210755 to do this for multiple deletions from the thread pane) does not require
you to call RowCountChanged, although in this case you should be sure to clear
the selection manually instead.

Comment 14

[Scott MacGregor]

*** Bug 214444 has been marked as a duplicate of this bug. ***

Comment 15

[Scott MacGregor]

*** Bug 214604 has been marked as a duplicate of this bug. ***

Comment 16

[Matthias Versen [:Matti]]

*** Bug 213714 has been marked as a duplicate of this bug. ***

Comment 17

[Kai de Leeuw]

is bug 214603 in some way related to this?

Comment 18

[David :Bienvenu]

taking for now.

Comment 19

[Scott MacGregor]

From the mozilla zine forums this bug is still around. Showing up in the 8/3
build from last week.

"I AM seeing the return of the delete messages and you still see the dots and
gridlines - this happened when I emptied my trash. Might be a trunk regression
on this. "

Comment 20

[David :Bienvenu]

Attachment: fix case for deleting top level msg in a thread

I'm going to clean up all the asserts from the tree control, in the hope that
will fix the problems. This first patch cleans up the assert when you delete
the top level message in a collapsed thread.

Comment 21

[Scott MacGregor]

Comment on attachment 129627 [details] [diff] [review]
fix case for deleting top level msg in a thread

Thanks for looking into this David. I'll happily put an sr=mscott on this first
part of the bug fix. I assume more will follow as you fix additional cases.

Comment 22

[David :Bienvenu]

Attachment: fix for handling multiple delete case

just use batching

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 129627 [details] [diff] [review]
fix case for deleting top level msg in a thread

r/a=sspitzer for 1.5 beta

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 129660 [details] [diff] [review]
fix for handling multiple delete case

r/sr/a=sspitzer for 1.5 beta

Comment 25

[David :Bienvenu]

these two patches checked in. I'll see if there are any remaining assertions,
and if I can reproduce the dot problem.

Comment 26

[[e-mail]]

Comment on attachment 129660 [details] [diff] [review]
fix for handling multiple delete case

Actually you should special case multiple deletes, because batching causes full
tree invalidation - see bug 210755.

Comment 27

[David :Bienvenu]

thx, I'll add a check for multiple delete before batching.

Comment 28

[David :Bienvenu]

Attachment: fix for empty trash case

this patch fixes the empty trash case, and the optimization that Neil pointed
out for only batching with multiple delete.

Comment 29

[David :Bienvenu]

Attachment: better fix

don't need the invalidate call anymore, so remove, and fix comment.

Comment 30

[Oliver Klee]

*** Bug 215018 has been marked as a duplicate of this bug. ***

Comment 31

[[e-mail]]

Comment on attachment 129680 [details] [diff] [review]
fix for empty trash case

>+    mTree->RowCountChanged(0, -saveSize);
>+    mTree->Invalidate();
Nit: RowCountChanged should already do sufficient invalidating.

Comment 32

[[e-mail]]

Comment on attachment 129680 [details] [diff] [review]
fix for empty trash case

>   // clear the existing selection.
>   if (mTreeSelection) {
>     mTreeSelection->ClearSelection();
>   }
> 
>   // this will force the tree to ask for the cell values
>   // since we don't have a db and we don't have any keys, 
>   // the thread pane goes blank
>-  if (mTree) mTree->Invalidate();
>+  if (mTree) 
>+  {
>+    mTree->RowCountChanged(0, -saveSize);
Actually, this will also clear the selection, so you can take out that call too
(just goes to show why it should have been done properly in the first place ;-)

Comment 33

[[e-mail]]

Maybe I should read all my bugmail before commenting next time :-[

Comment 34

[Scott MacGregor]

*** Bug 215988 has been marked as a duplicate of this bug. ***

Comment 35

[[e-mail]]

Comment on attachment 129683 [details] [diff] [review]
better fix

>+  // tell the tree all the rows have gone away
>+  if (mTree) 
>+    mTree->RowCountChanged(0, -saveSize);
Ok, so...

>+        if (numIndices > 1)
>           mTree->BeginUpdateBatch();
>         for (PRUint32 i=0;i<numIndices;i++)
>           NoteChange(mIndicesToNoteChange[i], -1, nsMsgViewNotificationCode::insertOrDelete);
>+        if (numIndices > 1)
>           mTree->EndUpdateBatch(); 
you probably need to check that mTree exists here too.

Comment 36

[David :Bienvenu]

Attachment: fix addressing Neil's comments

this addresses Neil's comments. I suspect mTree will never be null anymore -
the checks were added years ago due to one instance of mOutliner being null,
but it's too risky to remove those checks now.

Comment 37

[(not reading, please use [e-mail] instead)]

Comment on attachment 129737 [details] [diff] [review]
fix addressing Neil's comments

r/sr/a=sspitzer for 1.5 beta

Comment 38

[[e-mail]]

I'm thinking of the stand alone message window - doesn't that have a dbview
without a tree?

Comment 39

[David :Bienvenu]

good point, that's true. I'll remove the now bogus comment about why we're
checking for a null tree.

Comment 40

[David :Bienvenu]

fix checked in 

Comment 41

[Kai de Leeuw]

is this related to bug 214603? is it maybe the same?

Comment 42

[Stephen Walker]

*** Bug 216756 has been marked as a duplicate of this bug. ***

Comment 43

[Matthias Versen [:Matti]]

*** Bug 218643 has been marked as a duplicate of this bug. ***

Comment 44

[Mike Cowperthwaite]

*** Bug 221441 has been marked as a duplicate of this bug. ***

Comment 45

[Mike Kaply [:mkaply]]

bienvenu:

can you prpare a 1.4 branch patch that sums everything up?

Thanks

Comment 46

[David :Bienvenu]

Attachment: cumulative fix for 1.4.2

Comment 47

[Mike Kaply [:mkaply]]

Comment on attachment 141334 [details] [diff] [review]
cumulative fix for 1.4.2

a=mkaply for 1.4.2.

I trust you david :)

Please get this in ASAP and mark fixed1.4.2

Thanks!