500216 - Reply to OP on collapsed thread opens a reply for every message in the thread

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Chris Ilias [:cilias]]

Attachment: screencast

1. Collapse a thread.
2. With the first post of the collapsed thread selected, click Reply.

Result:
A message composition window will open for every message in the thread.

Expected result:
A message composition window should only open for the selected post.

Using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090624 Shredder/3.0b3pre

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I see this too, but not sure if this is intended. Ludo / Wayne?

Comment 2

[Wayne Mery (:wsmwk)]

I have collapsed thread actions prefed off and don't see it - so that's a good thing :)

Comment 3

[Magnus Melin [:mkmelin]]

Definitely not intended. For large threads this could be very annoying.

Comment 4

[Magnus Melin [:mkmelin]]

Should be fallout from bug 448288.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I'm on the fence about whether we'd really hold b3 if this were the last bug.  Do other folks have strong opinions here?  This certainly seems related to the stuff discussed in bug 494811 (if there are any more changes that get made as a result of that discussion).  

It's also interesting to me that the underlying conceptual model seems to be want to be inconsistent across the actions (eg move and delete want to act on the whole thread, reply wants to act on a single message, and forward wants to bundle up the whole thread into a single message).  But maybe that's not problem.  I'd be interested to hear clarkbw's thoughts here...

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

I might be a little confused here.  I don't think reply is going to make much sense with the thread summary view.  Specifically with the thread summary page from bug 454829 giving an overview of a conversation I think there will be less expectations that a reply to a whole conversation would be meaningful.  When a collapsed thread shows the first message in the thread instead of the thread summary view I think there is an expectation that reply works as you're looking at a single message.

I believe the consistent piece here is what we display in the message view and what actions are available based on that display.  This is why having folder summaries and thread summaries are important to give a set of actions that are possible based on what is selected.  The thread list view does give some indications of what is selected but they are often not easy understand, not as much as the message reader area.

Is the thread summary view turned off for this screencast?  I wasn't even aware it could be turned off...

Comment 7

[David Ascher (:davida)]

The thread summary shouldn't be turn offable independently of the 'act on grouped messages' feature.  So there's two sub-bugs, I think:

1) in threaded summary view, reply should be disabled.
2) if threaded summary pref is off, reply should act as it did in tb2.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

That sounds right to me; I suspect we want these both for beta3.

Comment 9

[David :Bienvenu]

2) is already true, afaik. I can't reproduce what's in the screen cast. Can anyone else?

Comment 10

[David :Bienvenu]

So, in TB 2, I could select two messages and press reply, and get two compose windows. Are we saying we should disable that behavior? Or are we saying we should disable reply if and only if one or more of the lines selected in the thread pane is a collapsed thread, and threaded summary is preffed on?

Comment 11

[David Ascher (:davida)]

The latter gets my vote.

Comment 12

[David :Bienvenu]

I suppose the trivial hack is to compare the number of selected indices in the tree w/ what the view tells us is the number of selected messages, and if they differ, disable reply...

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #10)
> Or are we saying we
> should disable reply if and only if one or more of the lines selected in the
> thread pane is a collapsed thread, and threaded summary is preffed on?

yes :)

Comment 14

[Chris Ilias [:cilias]]

(In reply to comment #6)
> Is the thread summary view turned off for this screencast?  I wasn't even aware
> it could be turned off...

Nope. I just tried it in a new profile, and get the same result.
http://screencast.com/t/jg0XBWBchw

Comment 15

[David :Bienvenu]

ah, thx, that's an issue somewhat specific to news - since summaries don't work on news, you're seeing the top-level message, but we think the collapsed messages are included in the selection. I thought you had disabled the thread summary pref, but it's merely that you're clicking on a news thread.

So it feels like we really need to check if summarizing failed, which is kind of annoying, but the better part of valor. 

I've got a patch that disables reply when a top level collapsed thread is selected, but I need to poke that the code that prevents us from updating command status when the selection hasn't changed in sufficiently interesting ways.

Comment 16

[David :Bienvenu]

Attachment: proposed fix

this disables a bunch of commands when a thread summary is shown, and doesn't include collapsed messages if summarizeSelection was false.

This introduces a small inconsistency between GetHeadersFromSelection and GetNumSelected, when summarizeSelection fails unexpectedly, in that GetNumSelected will include the collapsed messages, and GetHeadersFromSelection won't. Since summarizeSelection is going to stop failing unexpectedly, this should be OK ;-)

Comment 17

[David :Bienvenu]

Sid, I figure you know this code really well at this point :-).

Comment 18

[Rain [:rain1] (inactive)]

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

>diff --git a/mail/base/content/mail3PaneWindowCommands.js b/mail/base/content/mail3PaneWindowCommands.js
>-        if (GetNumSelectedMessages() > 0)
>+        if (numSelected > 0)
>         {
>           if (!gFolderDisplay.getCommandStatus(nsMsgViewCommandType.cmdRequiringMsgBody))
>+            return false;
>+          if (gFolderDisplay.selectedIndices.length != numSelected &&
>+              command != "cmd_applyFiltersToSelection" &&
>+              gDBView && gDBView.currentlyDisplayedMessage == nsMsgViewIndex_None)

Please add a comment explaining what you're looking for here.

Could you also add a comment somewhere (maybe to the IDL? <http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgDBView.idl#332> looks like a good place) explaining the GetNumSelected/GetHeadersForSelection inconsistency?

This seems to be a good candidate for mozmill testing, but we don't have any helpers for the toolbar right now. Setting in-testsuite? to keep track.

Comment 19

[David :Bienvenu]

Attachment: fix addressing comments, by adding comments

carrying forward r+, requesting sr. I've added some more comments explaining what's going on...

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 387687 [details] [diff] [review]
fix addressing comments, by adding comments

This breaks summarising. I've got a thread with 4 messages in (note its the only thread I've tested it with). If I start up Thunderbird and select the thread as the first thing, then it shows all four messages in the summary. If I then select another message, then go back to the thread, it shows only the first message.

Comment 21

[David :Bienvenu]

mmm, I thought that was just summarizeSelection being sensitive, but it does seem to be caused by this patch - perhaps its getting confused about how many messages to summarize, after going from a single message to a collapsed thread.

Comment 22

[David :Bienvenu]

Yeah, that's it. I think I need to change the sense of mSelectionSummarized to mSummarizeFailed and only set it when summarizeSelection fails unexpectedly.

Comment 23

[David :Bienvenu]

Attachment: fix regression sumarizing selections

thx for catching that. This separates out the handling of summarize failing (which means when it stops failing, perhaps we can rip out the code).

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 387761 [details] [diff] [review]
fix regression sumarizing selections

That's better, sr=Standard8

Comment 25

[Mark Banner (:standard8)]

I've pushed this so that it is in today's nightly: http://hg.mozilla.org/comm-central/rev/0e2f251b82fa

Comment 26

[Joe Sabash [:JoeS1]]

(In reply to comment #15)
> ah, thx, that's an issue somewhat specific to news - since summaries don't work
> on news, 

I'm confused by this statement.
Summaries seem to be working somewhat in news for me. Should they be ?

If I select a collapsed thread, I see the first message in the thread.
After expanding the thread,then collapsing that thread, selecting another thread, then selecting the original thread, I see the summary.

Displaying thread summaries in news is a desirable feature IMO
Although the summary should be more easily, and predictably displayed.

Comment 27

[David :Bienvenu]

if the news group isn't configured for offline use, you won't see any snippets, as I understand it.