Closed Bug 211289 Opened 21 years ago Closed 21 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect)

Product:
SeaMonkey
Component:
MailNews: Message Display
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: sspitzer, Assigned: Bienvenu)

References

Details

(Keywords: fixed1.4.2)

Attachments

(5 files, 2 obsolete files)

screen shot
12.63 KB, image/gif
Details
fix case for deleting top level msg in a thread
2.87 KB, patch
sspitzer
: review+
mscott
: superreview+
sspitzer
: approval1.5b+
Details | Diff | Splinter Review
fix for handling multiple delete case
808 bytes, patch
sspitzer
: review+
sspitzer
: superreview+
sspitzer
: approval1.5b+
Details | Diff | Splinter Review
fix addressing Neil's comments
1.75 KB, patch
sspitzer
: review+
sspitzer
: superreview+
sspitzer
: approval1.5b+
Details | Diff | Splinter Review
cumulative fix for 1.4.2
4.86 KB, patch
mkaply
: approval1.4.2+
Details | Diff | Splinter Review 
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?
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.
Whiteboard: [wanted for 1.4.1]
Is this at all a duplicate of bug 169481?

no dupe because this is a very recent regression.

*** Bug 213294 has been marked as a duplicate of this bug. ***
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.
*** Bug 213752 has been marked as a duplicate of this bug. ***
are these duplicates? bug 213401, bug 212388
*** Bug 213401 has been marked as a duplicate of this bug. ***
*** Bug 212388 has been marked as a duplicate of this bug. ***
*** Bug 214247 has been marked as a duplicate of this bug. ***
Blocks: bms
Summary: select all / delete leaves the thread pane in a bad state → select all / delete leaves the thread pane in a bad state (dots in thread pane)
*** Bug 214308 has been marked as a duplicate of this bug. ***
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.
*** Bug 214444 has been marked as a duplicate of this bug. ***
*** Bug 214604 has been marked as a duplicate of this bug. ***
*** Bug 213714 has been marked as a duplicate of this bug. ***
is bug 214603 in some way related to this?
taking for now.
Assignee: sspitzer → bienvenu
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. "
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 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.
Attachment #129627 - Flags: superreview+
just use batching
Comment on attachment 129627 [details] [diff] [review]
fix case for deleting top level msg in a thread

r/a=sspitzer for 1.5 beta
Attachment #129627 - Flags: review+
Attachment #129627 - Flags: approval1.5b+
Comment on attachment 129660 [details] [diff] [review]
fix for handling multiple delete case

r/sr/a=sspitzer for 1.5 beta
Attachment #129660 - Flags: superreview+
Attachment #129660 - Flags: review+
Attachment #129660 - Flags: approval1.5b+
these two patches checked in. I'll see if there are any remaining assertions,
and if I can reproduce the dot problem.
Status: NEW → ASSIGNED
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.
thx, I'll add a check for multiple delete before batching.
Attached patch fix for empty trash case (obsolete) — — Splinter Review 
this patch fixes the empty trash case, and the optimization that Neil pointed
out for only batching with multiple delete.
Attached patch better fix (obsolete) — — Splinter Review 
don't need the invalidate call anymore, so remove, and fix comment.
Attachment #129680 - Attachment is obsolete: true
*** Bug 215018 has been marked as a duplicate of this bug. ***
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 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 ;-)
Maybe I should read all my bugmail before commenting next time :-[
*** Bug 215988 has been marked as a duplicate of this bug. ***
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.
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.
Attachment #129683 - Attachment is obsolete: true
Comment on attachment 129737 [details] [diff] [review]
fix addressing Neil's comments

r/sr/a=sspitzer for 1.5 beta
Attachment #129737 - Flags: superreview+
Attachment #129737 - Flags: review+
Attachment #129737 - Flags: approval1.5b+
I'm thinking of the stand alone message window - doesn't that have a dbview
without a tree?
good point, that's true. I'll remove the now bogus comment about why we're
checking for a null tree.
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
is this related to bug 214603? is it maybe the same?
*** Bug 216756 has been marked as a duplicate of this bug. ***
Flags: blocking1.4.2?
*** Bug 218643 has been marked as a duplicate of this bug. ***
*** Bug 221441 has been marked as a duplicate of this bug. ***
bienvenu:

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

Thanks
Flags: blocking1.4.2? → blocking1.4.2+

    
    

    
  
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!

        Attachment #141334 -
        Flags: approval1.4.2+

    
  
Keywords: fixed1.4.2
Whiteboard: [wanted for 1.4.1]
Product: Browser → Seamonkey

    
  
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: