243532 - When move/copy of messages finishes, selection jumps

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Stewart Gordon]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040503
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040503

I often file messages when reading newsgroups.  The copying operation happens in
the background, so I can continue reading more messages while it's doing it. 
However, it likes to tell me it's done copying by reselecting the first copied
message.  This means that I can easily lose my place in the thread I am reading.

The same thing happens whether I am copying from a newsgroup, or moving or
copying between local folders.

Reproducible: Always
Steps to Reproduce:
1. Select some messages.
2. Drag them into a folder.
3. While waiting for the messages to transfer, select another message.

Actual Results:  
If messages were copied, then the first message copied becomes selected once it
has finished.  If moved, the selection jumps to the message immediately below
where it has moved from.

Expected Results:  
Left the selection where I put it.  If the selection as at the end of a move
operation consists only of messages that have been moved, then and only then
should it select the next message.

Comment 1

[Lorenzo Colitti]

This also happens with IMAP messages, and on all platforms.

Comment 2

[Lorenzo Colitti]

Attachment: patch v1

This patch makes it so that gNextMessageViewIndexAfterDelete is set to -2 when
a message is displayed in the message pane, so when the move or delete
operation completes the selection does not "snap back" to the message chosen by
SetNextMessageAfterDelete() when the operation started.

Comment 3

[Lorenzo Colitti]

Comment on attachment 152276 [details] [diff] [review]
patch v1

Neil, can you look at this?

Comment 4

[Lorenzo Colitti]

It's true that there are other ways to change the selection than displaying a
message, but I think this is the most important case because the user is
probably trying to read the message he just selected and doesn't want to be
interrupted.

Comment 5

[Lorenzo Colitti]

CC'ing Scott on this usability issue

Comment 6

[Lorenzo Colitti]

The perfect fix would be to set gNextMessageViewIndexAfterDelete to -2 whenever
the tree selection changes. But I can't figure out a way to do that.

Comment 7

[Lorenzo Colitti]

Cancelling review request based on Neil's comments over IRC.

Neil suggests using ThreadPaneSelectionChanged in threadPane.js to check if the
selection has changed. This is needed for 3-pane with a collapsed message pane,
and for the search pane, which don't load messages. But the original fix needs
to be implemented for the standalone message window.

I will post an updated patch soon.

Comment 8

[Lorenzo Colitti]

Hmm, it turns out it's not as simple as that. ThreadPaneSelectionChanged can be
called for other reasons than the user changing the selection himself. For
example, it's called by gDBView.onDeleteCompleted in
HandleDeleteOrMoveMsgCompleted when a message is deleted and from elsewhere in
C++ code when a message is moved. We could add hacks to both these places, but
I'm not sure that would be a good idea.

Comment 9

[[e-mail]]

When I tried moving a message the select handler got called via
HandleDeleteOrMoveMsgCompleted's call to onDeleteCompleted.

Comment 10

[Lorenzo Colitti]

When I move a message from one IMAP to another, I see this stack before the one
containing onDeleteCompleted:

#0  nsTreeSelection::FireOnSelectHandler()
#1  in nsTreeSelection::AdjustSelection(int, int)
#2  in nsTreeBodyFrame::RowCountChanged(int, int)
#3  in nsTreeBoxObject::RowCountChanged(int, int)
#4  in nsMsgDBView::NoteChange(unsigned, int, int)
#5  in nsMsgDBView::RemoveByIndex(unsigned)
#7  in nsMsgDBView::OnKeyDeleted(unsigned, unsigned, int, nsIDBChangeListener*)
#8  in nsMsgDatabase::NotifyKeyDeletedAll(unsigned, unsigned, int,
nsIDBChangeListener*)
#9  in nsMsgDatabase::DeleteHeader(nsIMsgDBHdr*, nsIDBChangeListener*, int, int)
#10 in nsMsgDatabase::DeleteMessages(nsMsgKeyArray*, nsIDBChangeListener*)
#12 in nsImapMailFolder::OnStopRunningUrl(nsIURI*, unsigned)

so it doesn't look like it's possible to find out whether the user changed the
selection himself or something else did it.

I think we should go for the 90% fix in attachment 152276 [details] [diff] [review], or something like it.
I'll do some more testing and see how it goes.

Comment 11

[[e-mail]]

http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgDBView.cpp#2509
RemoveByIndex shouldn't call NoteChange in the middle of a move or delete...

Comment 12

[David :Bienvenu]

>RemoveByIndex shouldn't call NoteChange in the middle of a move or delete

the m_deletingRows var is used to determine if the user deleted the messages by
pressing delete - was this move done by drag+drop? Or the File menu? Perhaps we
just need to set that var (as much as I hate that whole m_deletingRows stuff) in
those cases...assuming we still get the OnDeleteCompleted notification.

Re the news issue, you're really doing a copy, because delete isn't supported
for newsgroups - that sounds like a potentially separate issue.

Comment 13

[Lorenzo Colitti]

(In reply to comment #12)
> >RemoveByIndex shouldn't call NoteChange in the middle of a move or delete
> the m_deletingRows var is used to determine if the user deleted the messages
> by pressing delete - was this move done by drag+drop? Or the File menu?

Yes, this was done by dragging the message with the mouse.

> Perhaps we just need to set that var (as much as I hate that whole 
> m_deletingRows stuff) in those cases...assuming we still get the
> OnDeleteCompleted notification.

Hmm, set which var in what cases? I'm not sure I get your point.

> Re the news issue, you're really doing a copy, because delete isn't supported
> for newsgroups - that sounds like a potentially separate issue.

I see. Maybe something similar would work there too...

Comment 14

[[e-mail]]

Yes, I see that trace via drag&drop, but we should improve drag&drop rather than
not fixing this just because drag&drop doesn't do things correctly...

Comment 15

[Stewart Gordon]

How the user initiated the move/copy/delete operation seems immaterial to me. 
Am I missing something?

Comment 16

[[e-mail]]

Yes, we need to distinguish between deletes initiated in the current window and
deletes initiated in another window, or in the case of IMAP, another client.

Comment 17

[Lorenzo Colitti]

Attachment: same patch for branch thunderbird

This is the same patch as before except for branch (thunderbird only).

It's a 90% fix that takes care of the common (and most annoying) case where a
user deletes or moves one or more messages by dragging them with the mouse and
then starts reading another message.

Comment 18

[Lorenzo Colitti]

Comment on attachment 156130 [details] [diff] [review]
same patch for branch thunderbird

Requesting review.

Scott, shall we go for this 90% fix for now? I don't think I will be able to
come up with something better in the near future.

Comment 19

[Lorenzo Colitti]

Note that this also fixes the Local Folders case originally reported, but I am
unable to reproduce the newsgroup case with a current build of thunderbird.

Comment 20

[Lorenzo Colitti]

Comment on attachment 156130 [details] [diff] [review]
same patch for branch thunderbird

I've been running with this for quite a while now with no strange behaviour.
Scott, could you look at it and check it in if you like it?

Comment 21

[Lorenzo Colitti]

Comment on attachment 156130 [details] [diff] [review]
same patch for branch thunderbird

Oops, really requesting review

Comment 22

[Scott MacGregor]

Comment on attachment 156130 [details] [diff] [review]
same patch for branch thunderbird

I'm ok with this 90% fix for Thunderbird for now as it does make things much
better in the short term.

It sounds like Neil may have some questions for fixing this all the way if I
read the bug right. I'll defer to him on what	to do for seamonkey.

Comment 23

[David :Bienvenu]

Comment on attachment 156130 [details] [diff] [review]
same patch for branch thunderbird

sr=bienvenu - however, do you really need to check that
gNextMessageViewIndexAfterDelete is >= 0? Can you just always set it to -2 here
and save a little bit of code?

Comment 24

[Stewart Gordon]

Lorenzo - I'm a bit puzzled.  You took out the review request on attachment
152276 [details] [diff] [review], and then requested a review on attachment 156130 [details] [diff] [review], even though they're
exactly the same apart from the position in the CVS tree.

So, are we still going to have the fix for plain Mozilla?

Comment 25

[Lorenzo Colitti]

Attachment: patch with Bienvenu's suggestion

Same as the previous patch for branch Thunderbird, but with Bienvenu's
suggestion. I've been running with this for a while and it seems to work fine.

Comment 26

[Lorenzo Colitti]

Comment on attachment 161775 [details] [diff] [review]
patch with Bienvenu's suggestion

Carrying forward review requests.

Scott, David, could one of you check this in? Thanks!

Comment 27

[David :Bienvenu]

you need this checked into the trunk for seamonkey and thunderbird, and for the
aviary branch for thunderbird? I can do that...

Comment 28

[David :Bienvenu]

fixed trunk, tbird and seamonkey, branch tbird. Thanks, Lorenzo.