Closed Bug 500216 Opened 13 years ago Closed 13 years ago

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

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: cilias, Assigned: Bienvenu)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached file 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
I see this too, but not sure if this is intended. Ludo / Wayne?
I have collapsed thread actions prefed off and don't see it - so that's a good thing :)
Flags: wanted-thunderbird3?
Definitely not intended. For large threads this could be very annoying.
Flags: wanted-thunderbird3? → blocking-thunderbird3+
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 3.0b3
Should be fallout from bug 448288.
Blocks: 448288
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...
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
Whiteboard: [needs input clarkbw, patch, review]
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...
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.
That sounds right to me; I suspect we want these both for beta3.
2) is already true, afaik. I can't reproduce what's in the screen cast. Can anyone else?
Whiteboard: [needs input clarkbw, patch, review] → [needs patch, review]
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?
The latter gets my vote.
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...
Whiteboard: [needs patch, review] → [needs ui-review clarkbw, patch, review]
(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 :)
(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
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.
Whiteboard: [needs ui-review clarkbw, patch, review] → [needs patch, review]
Attached patch proposed fix (obsolete) — Splinter Review
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 ;-)
Attachment #387339 - Flags: superreview?(bugzilla)
Attachment #387339 - Flags: review?(sid.bugzilla)
Sid, I figure you know this code really well at this point :-).
Whiteboard: [needs patch, review] → [has patch, needs review]
Attachment #387339 - Flags: review?(sid.bugzilla) → review+
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.
Flags: in-testsuite?
carrying forward r+, requesting sr. I've added some more comments explaining what's going on...
Attachment #387339 - Attachment is obsolete: true
Attachment #387687 - Flags: superreview?(bugzilla)
Attachment #387687 - Flags: review+
Attachment #387339 - Flags: superreview?(bugzilla)
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.
Attachment #387687 - Flags: superreview?(bugzilla) → superreview-
Whiteboard: [has patch, needs review] → [needs new patch]
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.
Yeah, that's it. I think I need to change the sense of mSelectionSummarized to mSummarizeFailed and only set it when summarizeSelection fails unexpectedly.
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).
Attachment #387687 - Attachment is obsolete: true
Attachment #387761 - Flags: superreview?(bugzilla)
Whiteboard: [needs new patch] → [needs sr]
Comment on attachment 387761 [details] [diff] [review]
fix regression sumarizing selections

That's better, sr=Standard8
Attachment #387761 - Flags: superreview?(bugzilla) → superreview+
I've pushed this so that it is in today's nightly: http://hg.mozilla.org/comm-central/rev/0e2f251b82fa
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs sr]
(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.
if the news group isn't configured for offline use, you won't see any snippets, as I understand it.
You need to log in before you can comment on or make changes to this bug.