80897 - multiple delete: poking outliner one row at a time

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

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

multiple delete:  poking outliner one row at a time

delete 100 messages and you'll see.

when this is fixed, I need to check that #80543 (which will have landed by 
then) is ok.

Comment 1

[Håkan Waara]

While looking into the source of this bug, I debugged all the SelectionChanged()
calls we process and one is from msgMail3PaneWindow.js.  It's a hack from
mscott, and commenting it out seems to make no difference -- I found that after
some testing, nothing regressed that I could see. Can someone r= or give me a clue?

Index: resources/content/msgMail3PaneWindow.js
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/resources/content/msgMail3PaneWindow.js,v
retrieving revision 1.169
diff -u -r1.169 msgMail3PaneWindow.js
--- msgMail3PaneWindow.js	2001/11/01 04:05:39	1.169
+++ msgMail3PaneWindow.js	2001/11/01 19:45:44
@@ -270,12 +270,6 @@
       if (gNextMessageViewIndexAfterDelete != -1) 
       {
         outlinerSelection.select(gNextMessageViewIndexAfterDelete);
-        // since gNextMessageViewIndexAfterDelete probably has the same value
-        // as the last index we had selected, the outliner isn't generating a new
-        // selectionChanged notification for the outliner view. So we aren't
loading the 
-        // next message. to fix this, force the selection changed update.
-        if (outlinerView)
-          outlinerView.selectionChanged();
         EnsureRowInThreadOutlinerIsVisible(gNextMessageViewIndexAfterDelete); 
       }
       else 

Comment 2

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

hmm, better wait on that until we figure out why it's there.  (do the cvs
checkin logs give a bug #?)

something I noticed, that might be related:

we're not properly setting selection in the thread pane when you:

1) delete a msg from the std alone msg window, and the message was selected in
the thread pane.

2) delete a msg from the search window, and the message was the one selected in
the thread pane.

I'd start by investigating that, and once that's fixed, see if that selection
change is not needed.

Comment 3

[Håkan Waara]

I have batch-deleting kind of working. It's not just very reliable (it's
sometimes crashing in nsUint32Array::RemoveAt()), but it's a good
proof-of-concept.  Work-in-progress patch coming up.  Any ideas on the crash are
helpful.

Comment 4

[Håkan Waara]

Attachment: Patch, work-in-progress

Comment 5

[Håkan Waara]

Attachment: Patch v2

My last patch was unnecessarily complicated, and had a few problems, so here
comes a much simpler and more straightforward patch.

It moves the NoteChange() call (that will update the outliner with the new
info) from the RemoveByIndex() function which will get called an arbitrary
number of times to OnStopCopy(), which will be fired when a delete/copy batch
is done. If a delete batch is done, we'll send an event to the outliner to
update the UI.

Since the amount of messages being deleted is arbitrary (unless you cache index
numbers and and reiterate later, such like in my last patch -- ugh!), we will
send an |all| event to NoteChange() instead of the |indexOrDelete|.

This, of course, makes deleting multiple messages much faster than before,
because we wait for the batch to finish before we update the outliner.

Comment 6

[Håkan Waara]

->me

Comment 7

[Håkan Waara]

Comment on attachment 58243 [details] [diff] [review]
Patch v2

I think I really need to send the right type of event (i.e. |insertOrDelete|)
because otherwise the outliner won't know that the rowcount has changed and not
always update properly.

Comment 8

[Håkan Waara]

The threaded dbview likes to crash when I cache the first selected message index
and number of indices (in order to send the inserOrDelete event)... still trying
to figure out a better patch.

Comment 9

[Håkan Waara]

I have everything working except for reflowing the outliner correctly after the
mass-delete.

Basically, calling |mOutliner->RowCountChanged(firstLineChanged, numChanged)|
where numChanged is a number other than 1 doesn't always repaint it like it should.

If I am not doing something totally incorrect, I suspect this is an outliner bug.

Comment 10

[Jan Varga [:janv]]

It's a little bit more difficult.
Currently, for each deleted header it's called RowCountChanged(index, -1)
I think it should be called only for each solid group of selected headers.
e.g. RowCountChanged(startIndexOfThisGroup, -numberOfHeadersInThisGroup)

Comment 11

[Håkan Waara]

Anyone with more spare time than me can feel free to take this. It shouldn't be
hard to get this working...

Comment 12

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

taking back, for now.  I hope to get to multiple delete for 0.9.8

Comment 13

[Navin Gupta]

taking

Comment 14

[Navin Gupta]

hwaara, your patch may not work because it assumes selection is contiguous, 
that may not be true in all cases. 

Comment 15

[Navin Gupta]

Attachment: proposed fix for imap (delete mode- 'move to trash')

The fix is to delay the notification that delete is complete
(NotifyFolderEvent(mDeleteOrMoveMsgCompleted) to the
listener's(nsIMsgCopyServiceListener) OnStopyCopy(), this will keep the delete
selection intact and will enable us to poke outliner in blocks. 

Also for drag and drop of message we do not go through the view and
m_deletingMsgs will tell us if that is true.  

This fix is just for imap (one delete mode "move to trash"), same will
have to be done for other delete modes, local and news. david, please review
the
general idea.

Comment 16

[David :Bienvenu]

Ok, as you might have guessed, I'm not excited by the idea that the move/copy
code has abdicated its responsibility to send the deleteMoveMsgCompleted
notification and assumes that any listener is going to do that (that's what's
happening, right?) - sure, the view listener does it, but in general, other
listeners might not. So I'm going to have to apply this patch and figure out if
there's some other way of fixing that part.

Why call NoteEndChange instead of NoteChange? Yes, they're equivalent right now,
but I don't see anyone calling NoteStartChange, so you shouldn't call
NoteEndChange. The idea is that they should balance each other out, so you
wouldn't call NoteEndChange multiple times like that anyway. I'd just stick with
NoteChange.

Now, does the outliner guarantee that the ranges are monotonically increasing?
In other words, does the first range contain the lowest numbered indices? It
would have to for your code to work, I think. Once you remove the first indices
in the first range, are the other ranges still valid? Or does the selection
manager handle all that? It might - I can look into it if you don't know...

Comment 17

[David :Bienvenu]

Attachment: alternate patch, that just moves the mDeleteOrMoveMsgComplete notification to after OnStopyCopy

Comment 18

[David :Bienvenu]

Attachment: same as last one, alternate patch

Comment 19

[Navin Gupta]

Attachment: proposed fix

This patch batches outliner changes on delete/move based on the notification
mMoveOrDeleteMsgCompleted/failed. I have to store the ranges in an array
before notifying any changes for deleted row and also for imapdelete (mark it 
deleted) this routine OnDeleteCompleted will not apply because rows are not
removed from the view. Moved out nsIMsgCopyServiceListener to searchView where
it is needed. 

I have tested for all imap delete modes, local and news. this will not apply to

the search window and d&D of messages. please review.

Comment 20

[David :Bienvenu]

looks pretty good. One suggestion: instead of checking the delete model where
you do, on the delete notification, instead, only set m_deletingRows if the imap
delete model isn't imapDelete. Then, you don't have to check the delete model in
OnEndDelete. Since we won't be deleting rows when the delete model is imap
delete, it's more proper to not set m_deletingRows to true. Does that make
sense? Do that, and r=bienvenu. This is also one of those changes you might want
to run with for a couple days, just to be on the safe side. And you should make
sure there aren't any funny interactions when deleting message from the advanced
search results view, when you have the same folder open in the 3-pane ui. There
shouldn't be, but it's worth a quick check.

Comment 21

[Navin Gupta]

Attachment: proposed fix

changed m_deletingMsgs to m_deletingRows, don't set it for imap-delete.
checked 3pane and advanced search work fine.

Comment 22

[Scott MacGregor]

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

I'll do an r=mscott and let David put the final sr stamp once he sees the
latest patch.

Comment 23

[David :Bienvenu]

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

sr=bienvenu, good job.

Comment 24

[Navin Gupta]

fixed

Comment 25

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED

Windows 98, on a Pentium II 266, 128 megs of RAM, build ID:  2001-12-28 vs.
2001-12-26...

------------------------------------------
Build ID      # msgs        time to delete   
12-26           219           2.92 seconds     
12-28           219           2.27 seconds
12-26          1003          12.47 seconds
12-28          1003           8.64 seconds

looks like we shaved off 30% in deleting 1000+ messages, and around 22% for 219
messages.  

Comment 26

[scottputterman]

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

Comment 27

[David Brittain]

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