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

RESOLVED FIXED in Thunderbird 3.0b3


10 years ago
9 years ago


(Reporter: superbiskit, Assigned: Bienvenu)


(Blocks: 1 bug, {dataloss, regression})

Thunderbird 3.0b3
dataloss, regression
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090612 Minefield/3.6a1pre AutoPager/ (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.
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.
Component: General → Message Reader UI
QA Contact: general → message-reader
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

9 years ago
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.
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.
Ever confirmed: true
Flags: blocking-thunderbird3?
OS: Linux → All
Hardware: x86 → All
Summary: If a thread is collapsed in the folder pane, deleting one message from a message window "offers" to delete the whole thread → 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
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Whiteboard: dataloss regression
Target Milestone: --- → Thunderbird 3.0b3
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.
Keywords: dataloss, qawanted, regression
Whiteboard: dataloss regression → [needs feedback superbiskit/mkmelin]
Whiteboard: [needs feedback superbiskit/mkmelin] → [needs feedback superbiskit/mkmelin][needs qa]

Comment 7

9 years ago
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.
Assignee: nobody → bienvenu
I'm using branch nightlies.
Whiteboard: [needs feedback superbiskit/mkmelin][needs qa] → [needs feedback superbiskit][needs qa]

Comment 9

9 years ago
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.
Blocks: 497199

Comment 10

9 years ago
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

9 years ago
Created attachment 386170 [details] [diff] [review]
proposed fix
Attachment #386170 - Flags: superreview?(bugzilla)
Attachment #386170 - Flags: review?(bugzilla)


9 years ago
Whiteboard: [needs feedback superbiskit][needs qa] → [has patch for review standard8]


9 years ago
Attachment #386170 - Attachment is obsolete: true
Attachment #386170 - Flags: superreview?(bugzilla)
Attachment #386170 - Flags: review?(bugzilla)

Comment 12

9 years ago
Comment on attachment 386170 [details] [diff] [review]
proposed fix

this breaks thread summary


9 years ago
Whiteboard: [has patch for review standard8]

Comment 13

9 years ago
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.


9 years ago
Keywords: qawanted

Comment 14

9 years ago
Created attachment 386358 [details] [diff] [review]
proposed fix

while I go work on a mozmill test for this, I wanted to run the basic approach by asuth.
Attachment #386358 - Flags: review?(bugmail)

Comment 15

9 years ago
Created attachment 386407 [details] [diff] [review]
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.
Attachment #386407 - Flags: review?(bugmail)
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.
Attachment #386358 - Flags: review?(bugmail) → review+
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.
Attachment #386407 - Flags: review?(bugmail) → review+

Comment 18

9 years ago
Comment on attachment 386358 [details] [diff] [review]
proposed fix

requesting sr for the nsMsgDBView.cpp part of the patch.
Attachment #386358 - Flags: superreview?(bugzilla)
(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

9 years ago
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.
Ah, indeed, I didn't read closely enough.  Comment withdrawn!
Attachment #386358 - Flags: superreview?(bugzilla) → superreview+

Comment 22

9 years ago
fix and test checked in.
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.