499020 - If a thread is collapsed in the folder pane, deleting one message from a message window "offers" to delete the whole thread when selection has advanced to that thread

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[David A. Cobb]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090612 Minefield/3.6a1pre AutoPager/0.5.2.0 (http://www.teesoft.info/)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090613 Shredder/3.1a1pre ID:20090613030921

While triaging a folder, deleting old or uninteresting messages, I don't necessarily expand all the threads.  If I delete a message, and the thread on the folder pane is unexpanded, I get a warning (thank you for that!) that I'm about to delete all the unseen messages in the thread.  Since I do not have the folder view visible (I'm reading in a full-screen message window), that is an unpleasant surprise.

Reproducible: Always

Steps to Reproduce:
1. In folder view, collapse a thread.
2. Select the first message and Open-in-New-Window.
3. Delete the message.
Actual Results:  
Warning that I am about to delete all the "hidden" messages in the thread.

Expected Results:  
Delete the CURRENT message, and display the next.

This behavior makes sense when deleting from the folder view, although explicitly selecting a whole thread to delete it is not much more work.

Comment 1

[Ludovic Hirlimann [:Usul]]

I don't get that on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090617 Shredder/3.0b3pre Only the opened message get deleted.

Comment 2

[Magnus Melin [:mkmelin]]

How exactly do you get just one message opened? Seems if the thread is collapsed, double clicking the first msg now open all messages in new windows.

Comment 3

[David A. Cobb]

OK.  I wasn't paying close enough attention when this first happened. 
Here are the steps:

1. Find a mailbox you don't mind playing in :-)
2. Set view to Threaded.  Pick one thread and make sure it's collapsed;
3. Select the PREVIOUS single message (last of a different thread, perhaps;
4. Right-mouse "Open Message in New Window"
5. DELETE that message
[ "currency" moves now to the top of the collapsed thread, displays first message]

6. Select "DELETE"
---
The friendly warning about deleting the whole thread is displayed.

As a "nit", somehow "Apply" doesn't seem the appropriate button label on that box.
"Delete all" would make more sense.

My original point was that, with the Message-List covered by the Message Window, I cannot really tell much about the expanded/collapsed state of threads that were "further down the list" from where I started.

IMNSHO, The "currency" state should not follow the collapsed/expanded status when working from a Message Window.  Each successive message should be opened and dealt with.  Thread-deletion needs to be done from the Message-List Pane.

Comment 4

[Magnus Melin [:mkmelin]]

Ah, I see what you mean now. After deleting the first message selection advances to the collapsed thread, but since we're in the standalone msg window only the one msg is shown. I suppose we should never act on threads while in the standalone msg window.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I can't reproduce this on OS X in tonight's build.  When I press the "DELETE" button in the standalone window the first time, the message does disappear from the threadpane in the main window, and the selection there does move to the next thread as described.  However, the deleted message is actually still displayed in the standalone window (this seems like it itself is a separate bug).  Then when I press the "DELETE" button the second time in the standalone window, nothing happens at all.

That said, as described, this sounds like a dataloss bug.  Assuming it's reproducible, probably wants to block both Tb3 and 3.0b3, since it's (effectively) a dataloss regression from 3.0b2.  Marking it blocking now so that we don't lose track of it.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

David/Magnus: are you guys able to reproduce this in branch nightlies?  Or is it possible this is trunk only?

Adding qawanted to see if anyone else can reproduce this on branch and trunk builds.

Comment 7

[David :Bienvenu]

taking - I'm sure this has to do with the changes to operate on collapsed threads. I agree that it shouldn't happen in the stand-alone msg window.

Comment 8

[Magnus Melin [:mkmelin]]

I'm using branch nightlies.

Comment 9

[David :Bienvenu]

This may have been a regression from bug 474701, specifically the selection handling in the stand-alone message window. This bug was filed on the 6/13 build, and bug 474701 landed on 6/12.

Comment 10

[David :Bienvenu]

Yes, prior to bug 474710, the backend view code knew that if there was no mTreeSelection, it was a stand-alone message window. Now that the front end fakes an mTreeSelection for the stand-alone message window, the backend view code needs to figure out some other way to handle this. One thought is to make it so we don't operate on msgs in collapsed threads if we're displaying a single message - this ties the message summary stuff to the collapsed thread operation, at least for the single message selected case, which I think is OK. The other choice is to have the view know it's in stand-alone message window mode.  Unless there are any objections, I'll try the first option first.

Comment 11

[David :Bienvenu]

Attachment: proposed fix

Comment 12

[David :Bienvenu]

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

this breaks thread summary

Comment 13

[David :Bienvenu]

ugh, the fake js selection stuff so completely fakes everything out that I can't tell a stand-alone message window from a 3-pane ui in the db view code.

Comment 14

[David :Bienvenu]

Attachment: proposed fix

while I go work on a mozmill test for this, I wanted to run the basic approach by asuth.

Comment 15

[David :Bienvenu]

Attachment: add mozmill test

this adds a test for this to the mozmill test test-message-window.js.

This test fails (actually, it puts up a prompt in the failure case) w/o the other patch, and succeeds with it.

I'm not sure how much we care about the failure case - I could make the test twiddle the pref that controls whether we throw up the dialog or not, or we could just treat the test hanging with a dialog as a failure.

Comment 16

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

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

The wonderful thing about unit tests is that they make me okay with what otherwise seems like a dangerous game of whack-a-mole/chicken.

This would be dangerous in the tab setup where we the selections live a more exciting life, but should be fine here.

Comment 17

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

Comment on attachment 386407 [details] [diff] [review]
add mozmill test

I'm not really concerned about the dialog at this point.  I'm sure lots of tests have random failure modes that might involve dialogs. 

The test-window-helpers.js has event-driven hooks that we could use to log a failure whenever any unexpected window opens.  It could also kill the window automatically if it's modal.  But that's a task for another day.

Comment 18

[David :Bienvenu]

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

requesting sr for the nsMsgDBView.cpp part of the patch.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #17)
> (From update of attachment 386407 [details] [diff] [review])
> I'm not really concerned about the dialog at this point.  I'm sure lots of
> tests have random failure modes that might involve dialogs. 

Because these tests are automated, the pain of random failures is amplified,
and the combinatorics of multiple non-deterministic tests gets ugly relatively
quickly.  As Firefox / Gecko development scaled up over the Firefox 3 and 3.5
cycles, a very large number of person-hours were lost to this problem.  This
includes sheriff time, developer time, and ultimately QA time in a multi-month
project slowly tracking down and wading through the various failures.  Let's
not fall into this same trap as we're trying to scale up.

Comment 20

[David :Bienvenu]

I don't think Andrew meant random in the sense that you took it as - I think he means that if a regression happens, the failure modes will be somewhat random - test hangs, test errors, dialogs popping up, etc. Not that the tests will randomly fail w/o there being some sort of code regression cause.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Ah, indeed, I didn't read closely enough.  Comment withdrawn!

Comment 22

[David :Bienvenu]

fix and test checked in.