454829 - summary page for collapsed threads and multiple selections

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

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

When a collapsed thread is selected in the thread list the message pane displays the first message.  This behavior tends to make people think that despite their selecting of a collapsed thread, any actions performed will take place on the first message; because it is what is being shown.

With bug 448288 we'll be enabling actions on entire threads and yet we don't have a concept of how to visualize a thread.

Here's a plan for this. 
* When a collapsed thread is selected display a "Thread Summary" in the message pane
* When the thread is expanded individual messages should be displayed
* The "Thread Summary" should contain the thread of messages headers which are currently hidden in the collapsed thread list
* The "Thread Summary" should also display inline thread actions like ( Delete ) ( Tag ) and others.
* Each sender listed in the Thread Summary is linked to the message sent such that clicking on the reply will open the correct message and expand the thread in the thread list.

A basic layout for this Thread Summary could be something like this:

.----------------------------------------------.
|               ( Thread ) ( Actions ) ( Area) |
|                                              |
|  *Thread Subject*                            |
|    Sender Original        {date}             |
|      Sender Reply         {date}             |
|      Sender Reply         {date}             |
|        *Sender Reply*    *{date}*            |
|                                              |
'----------------------------------------------'

I think it would also make sense to look at including the text of the first message in the summary.  Perhaps making the first message text only show a preview of the first couple sentences.

More detailed mockups to come.

Comment 1

[David :Bienvenu]

What do we show if the user selects multiple collapsed threads, or a mix of collapsed threads and messages in expanded threads? In the extreme of selecting all the messages, we might need to limit the number of summaries we show for performance reasons.

Comment 2

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

(In reply to comment #1)
> What do we show if the user selects multiple collapsed threads, or a mix of
> collapsed threads and messages in expanded threads? In the extreme of selecting
> all the messages, we might need to limit the number of summaries we show for
> performance reasons.

I think that is a bit of a separate bug, but I would suggest we use a very similar method.  Currently if a user selects (for example) 3 threads and 2 messages our message pane view goes blank.

I would recommend that we build another "Selection Summary" page that displays the selected threads and messages with inline actions available. 

.----------------------------------------------.
|            ( Selection ) ( Actions ) ( Area) |
|            3 Threads, 2 Messages             |
|                                              |
|  Thread Subject           (10 messages)      |
|    Sender                 {date}             |
|                                              |
|  Thread Subject           (23 messages)      |
|    Sender                 {date}             |
|                                              |
|  Thread Subject           (16 messages)      |
|    Sender                 {date}             |
|                                              |
|  Message Subject                             |
|    Sender                 {date}             |
|                                              |
|  Message Subject                             |
|    Sender                 {date}             |
|                                              |
'----------------------------------------------'

Comment 3

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

Also, this shares a lot of concepts with bug 452440

Comment 4

[Ronald Killmer]

The layout in Comment #0 looks good as far as it goes. I think greater usefulness would be achieved if the Recipients replies stored in the Sent folder were interleaved, probably in a color. 

Make Read headers normal & default or filtered Tag color. The Unread headers bold & unread or filtered Tag color. Then the Recipient replies, italic and colored as Read or Tag color.

I suppose this would require Gloda to permit the inclusion of the Recipient replies.

As for the layout illustrated in Comment #2 I see that as being redundant of what is can be shown in the thread pane when using "Wide Message Pane" layout.  Perhaps with the vertical or the extension provided stacked layout the proposal offers a solution not now possible.

I do suspect there is a case where We will show a blank message pane. That case is the 'Start' page is disabled and *no* mail account set to Auto-Poll it's server.  I see this now in Shredder nightlies. Even the Account Manager page is not displayed until I do a mouse action in the window.

Comment 5

[Magnus Melin [:mkmelin]]

xref bug 364896 for the outlook/the bat multi selection behavior, which arguably has it's sides.

Comment 6

[Magnus Melin [:mkmelin]]

... though that's for multiple single message selection, not threads.

Comment 7

[Onno Ekker [:nONoNonO UTC+1]]

This summary would also fit very nice on mail digests, especially if the individual messages are clickable. See also bug 297088 and bug 147567.

Comment 8

[David :Bienvenu]

Do we want to detect the case where the user has a collapsed thread selected and then expands the thread, and load the top level message in the thread, so the user can read it? 

>* The "Thread Summary" should contain the thread of messages headers which are
>currently hidden in the collapsed thread list

Shouldn't it also contain a line for the top level message in the thread, so the user can click on it to read it, or otherwise manipulate it?

Comment 9

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

> Do we want to detect the case where the user has a collapsed thread selected
> and then expands the thread, and load the top level message in the thread, so
> the user can read it? 

> Shouldn't it also contain a line for the top level message in the thread, so
> the user can click on it to read it, or otherwise manipulate it?

Right and right.

Comment 10

[Mike Cowperthwaite]

(In reply to comment #0)
> When a collapsed thread is selected in the thread list the message pane
> displays the first message.

As far as I know, a collapsed thread is not selected when you click the top message; only the first message is selected.  If you click the thread icon on a collapsed thread, the entire thread is selected and the thread is expanded.  I read bug 448288 comment 4 and I couldn't tell from your "right now" whether you meant in the current builds or with the patch from that bug applied; it seemed like the former.


(In reply to comment #8)
> Do we want to detect the case where the user has a collapsed thread selected
> and then expands the thread, and load the top level message in the thread, so
> the user can read it? 

If the top level message is going to be loaded, then the selection *must* be changed to only include the top-level message.  There is already too much disconnect between what's selected and what's shown in the message pane.  This bug's basic idea of displaying something in the pane when a thread is selected works towards solving some of that disconnect.  A general-purpose multiple-selection display (bug 452440) would be even better.

Introducing more disconnect is counterproductive.  A message displayed in the message pane *must* mean that a single message is selected in the thread pane.
There are several old bugs complaining about a message continuing to be displayed when the selection has been cleared or for other reasons

Comment 11

[Mike Cowperthwaite]

(In reply to comment #10)
> A general-purpose
> multiple-selection display (bug 452440) would be even better.

Er, bug 451251.

Comment 12

[Wayne Mery (:wsmwk)]

Brian, in bug 214111 Comment 18 dmose writes "we probably need to make all such  [collapsed thread] changes in the same release or risk serious confusion." 

Bug 214111 is marked wanted-thunderbird3+, target rc1. But this bug, and it's dependent bug 448288 (which has a patch in the making), are not.  You might want them all to have the same target.  (there may be other such bugs - I didn't search)

Comment 13

[David Ascher (:davida)]

taking because i seem to care about this issue.  we'll see where that takes us.

Comment 14

[David Ascher (:davida)]

Attachment: WIP

checkpointing WIP.  needs attachment 359009 [details] [diff] [review] from bug 448288.

just does summary view, no buttons yet, and lots of rough edges.

Comment 15

[David Ascher (:davida)]

Attachment: WIP v2

Updated version.  does delete, archive, junk. 

I spent a long time ripping out the iframe implementation to make it easier for add-on authors to extend and to simplify the code, but the layout implications didn't make it work -- if one selected "all", then the flex rules forced the message pane to take over, wouldn't scroll, etc.  xul, xul. sigh.

anyway, html works just fine.

Comment 16

[David Ascher (:davida)]

Attachment: screenshot

screenshot of WIP v2.  bunch of details to tweak still.

Comment 17

[David Ascher (:davida)]

I'd like to get this in b2, but i'm going to set milestone at b3 in case wacky things show up.

Comment 18

[David Ascher (:davida)]

Attachment: jquery patch

Comment 19

[David Ascher (:davida)]

Attachment: WIP v3

Updated patch.  This requires a bunch of related patches, including those in bugs 448288 & 476103, and the jquery patch attached here.

It's far from finished, but I'm finding it it useful nonetheless.

Comment 20

[David Ascher (:davida)]

Attachment: WIP v4

It'd be better if I didn't forget files.

Comment 21

[David Ascher (:davida)]

Attachment: WIP v5

redux

Comment 22

[David Ascher (:davida)]

Attachment: WIP v6

this version deals w/ correctly displaying tags as they get changed w/ keyboard shortcuts for example, modulo some gloda bugs.

it doesn't deal w/ detecting read/unread state changes due to bug 479890.

Comment 23

[David Ascher (:davida)]

Attachment: WIP v7

* handle starring, tags of all messages in a thread in the multimessage view.

* display number of messages in the thread view, as per a request from bienvenu

Comment 24

[Magnus Melin [:mkmelin]]

Not that jquery isn't a nice framework, but that doesn't mean we should ship with it...

Comment 25

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

(In reply to comment #24)
> Not that jquery isn't a nice framework, but that doesn't mean we should ship
> with it...

What downsides are there to shipping with it?  Somes updsides are that it enables rapid development and lots of people are familiar with it, which seem like two pretty big upsides.

Comment 26

[David Ascher (:davida)]

Attachment: WIP v8

Version that doesn't require jquery, and w/ a bunch more fixes.

Still left to do: finish l10n, and do non-mac themes.

(I think that jquery in the tb platform makes a great deal of sense, and should be discussed, but this isn't the bug to do it in, or the forum).

Comment 27

[Magnus Melin [:mkmelin]]

I'm not saying we necessarily shouldn't, but it's a decent amount of code we'd essentially have to maintain.
Frameworks are great esp. for inter-browser interoperability but if something in the framework is really essential - shouldn't such a feature belong in core (without any framework) and preferably be standardized?

Comment 28

[David Ascher (:davida)]

Attachment: WIP v9

Two things left to do:

1) if selecting a collapsed thread, then clicking on the name of the first poster, the thread uncollapses (good), the first message is selected (good), but the message pane still shows the group.  I suspect that's because SelectionChanged() isn't called in nsMsgDBView.cpp, but I don't know why.

2) as discussed on IRC, "S" for starring/unstarring only affects the first message in a collapsed thread, not all.

bienvenu, i'd love your suggestions for both of these issues.

Apart from that, AFAICT, it's review-ready.

Comment 29

[David Ascher (:davida)]

Attachment: updated screenshot

Comment 30

[David :Bienvenu]

2) is just a bug with my multi-selection patch - I need to make star turn the selection into msg hdrs like we do for the other commands.

1) I'll have to look into, which I'll do soon, after I finish up some work with gmail integration.

Comment 31

[David Ascher (:davida)]

A couple of additional notes from my bike ride home:

1) I should probably cap the number of messages/threads to display, for those cases when people select 10,000 rows, with that cap picked from a pref or somewhere.

2) I think it'd be good to provide hooks for extension authors to display different content in that frame, both for the multi-select, as well as the folder-select case.  I might have a poke at that, time permitting.

Neither of those should block b3, IMO.

Comment 32

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

I think summarizeSelection should live on nsIMsgDBViewCommandUpdater, not nsIMsgWindowCommands.

Use of gSummary is sketchy but arguably it's no more work to change it from that to the 'right way' when I (or whomever) refactors the thing than if you stored things per-tab.

You create a few JS classes that start with lowercase letters; they should probably have initial caps.  Along the same lines, you add some global functions that start with a lowercase char and some that start with an uppercase character.  Might as well be consistent, whatever you do there.  (I suppose we have a lot of initial-caps ones right now, but I claim they are wrong.  I also am deleting many of them in a refactoring, so arguably they won't be there long.)

More doxygen docs would be appreciated, if only to explain what functionality justifies the existence of the classes/functions.  One of the deficiencies of the current front-end code is that it's not clear why various pieces of code exist, even if the function name and arguments are reasonable.

Comment 33

[David :Bienvenu]

(In reply to comment #28)
> 
> 1) if selecting a collapsed thread, then clicking on the name of the first
> poster, the thread uncollapses (good), the first message is selected (good),
> but the message pane still shows the group.  I suspect that's because
> SelectionChanged() isn't called in nsMsgDBView.cpp, but I don't know why.

SelectionChanged is called from the js code. I see you're calling SelectFolderMsgByKey, which is poking the tree selection, but technically the selection hasn't changed, has it? By first message, do you mean the very first message in the thread? I suppose we need to create some artificial selection changed notification in this case, though it's not clear to me where these smarts should live - does nsMsgDBView really know that something happened with summarizeSelection? (and I'm guessing that SeaMonkey is going to need change its nsIMsgDBViewCommandUpdater, or nsIMsgWindowCommands to add a summarizeSelection method - or does xpconnect simply ignore methods that don't exist when calling into js?)

> 
> 2) as discussed on IRC, "S" for starring/unstarring only affects the first
> message in a collapsed thread, not all.
>
I have a fix for this, but the general fix for other commands is a bit trickier - mark read/unread, thread as read, junk/non junk (especially tricky).

Comment 34

[David Ascher (:davida)]

Attachment: WIP v10

asuth's review comments included.

Note: I include the minimal changes to /suite to make them have API endpoints for the new nsIMsgDBViewCommandUpdater interface, but there is no summarizing in the UI.  I'm hoping that's the right thing to do.

Comment 35

[David :Bienvenu]

I'm not seeing a trash icon, and the archive button doesn't look like a button - looks like the theming isn't working on windows, I guess. I'll look into that. I'm also not seeing Stars in the message pane - I hope that's the theming as well. Are you getting the star from the msg hdr flags, or gloda?

Threads with '&' in the subject don't display because '&' isn't getting escaped. There might be a few other chars that need to be escaped as well.

Comment 36

[David :Bienvenu]

qute doesn't have folder-trash.png

some of the new copyrights refer to 2008

I've fixed a few js warnings locally, and cleaned up some other things. I'll try to figure out how to get just those diffs to you...

Comment 37

[David :Bienvenu]

I see this assertion relatively often, and if assertions pop up a dialog, I'm effectively horked and I have to restart:

 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x5f43a490, const char * aExpr=0x5f43a488, const char * aFile=0x5f43a438, int aLine=0x0000052c)  Line 364 + 0xc bytes	C++
 	gklayout.dll!nsEventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x03eb0e00, nsEvent * aEvent=0x0021dd18, nsIFrame * aTargetFrame=0x03e9802c, nsEventStatus * aStatus=0x0021db14, nsIView * aView=0x03ec9e88)  Line 1324 + 0x2b bytes	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0021dd18, nsIView * aView=0x03ec9e88, nsEventStatus * aStatus=0x0021db14)  Line 6126 + 0x36 bytes	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x03ec9e88, nsGUIEvent * aEvent=0x0021dd18, nsEventStatus * aEventStatus=0x0021db14)  Line 5936 + 0x1a bytes	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x03ec9e88, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0021dd18, int aCaptured=0x00000000)  Line 1397	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0021dd18, nsEventStatus * aStatus=0x0021dc5c)  Line 1353 + 0x22 bytes	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0021dd18)  Line 170	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0021dd18, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1052 + 0xc bytes	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0021dd18)  Line 1073	C++
 	gkwidget.dll!nsWindow::DispatchFocus(unsigned int aEventType=0x0000006b, int isMozWindowTakingFocus=0x00000001)  Line 6677 + 0x11 bytes	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000007, unsigned int wParam=0x00440736, long lParam=0x00000000, long * aRetValue=0x0021e228)  Line 4854 + 0x17 bytes	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00360d14, unsigned int msg=0x00000007, unsigned int wParam=0x00440736, long lParam=0x00000000)  Line 1268 + 0x1d bytes	C++

Comment 38

[David :Bienvenu]

(In reply to comment #28)

> 1) if selecting a collapsed thread, then clicking on the name of the first
> poster, the thread uncollapses (good), the first message is selected (good),
> but the message pane still shows the group.  I suspect that's because
> SelectionChanged() isn't called in nsMsgDBView.cpp, but I don't know why.
> 
I have a fix for this, though it's all in selectionSummaries.js.  Calling selectionChanged from the backend produced the assertions above so I had to do it all in the front end...

Comment 39

[David :Bienvenu]

Attachment: this includes my patch for handling selection, and tweaks davida's patch a bit, including fixing the selection issue

I don't know how happy interdiff will be with this, since it includes an other patch.

Comment 40

[David :Bienvenu]

ok, interdiff very unhappy - I'll try to generate a patch with the same files as are in wip 10

Comment 41

[David :Bienvenu]

Attachment: diff for interdiff

Comment 42

[David :Bienvenu]

ok, interdiff between that last patch and wip10 shows my changes - mostly silencing warnings by declaring variables, and the fix to make the summary page work with selection.

Comment 43

[David Ascher (:davida)]

Attachment: WIP v11

Ok, this version includes bienvenu's fixes, as well as:
 - use the mime2Decoded accessors for subjects & authors
 - works in qute, gnomestripe, and pinstripe

I'm thinking about a possible tweak to the HTML to make it easier for addons to place visualizations and other things like this, as well as something like the 'analyzer' model that I have in the patch for bug 492158, but I'd like bienvenu's thoughts on what else is needed at this point.

(also: andreas, this patch would be a good place for you to work on making it it prettier!)

Comment 44

[David Ascher (:davida)]

Attachment: WIP v12

Use the right patch, luke.

Comment 45

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

andreas: can you hg qimport this patch to get an idea for what it does?  You can either edit the page directly or do the styling outside this patch and we can pull it in.  Thanks!

Comment 46

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

assigning to andreas for now.  once we have a cleaned up look we can throw this back to david to finish up

Comment 47

[David :Bienvenu]

I've landed the grouping actions fix - let me know if there's stuff you need from me for this bug.

Comment 48

[David Ascher (:davida)]

Attachment: patch for review v1

At this point, I think the patch is ready for your review.  I've taken care of asuth's drive-by comments in comment #32.

I suspect andreas and bryan will want to tweak the layout & css, but the code review is orthogonal.

Comment 49

[David :Bienvenu]

Error ``MsgHdrToMimeMessage is not defined'' [x-] in file ``chrome://messenger/content/selectionsummaries.js'', line 394, character 0.

might explain why I'm not seeing snippets

Comment 50

[David :Bienvenu]

I'm also not seeing dumpExc, which looks to be defined in GlodaTestHelper.js but not anywhere relevant to this patch...

Comment 51

[David Ascher (:davida)]

Attachment: patch for review v2

fix subjects containing &'s
fix loading of mimemsg.js

Comment 52

[David :Bienvenu]

Comment on attachment 378944 [details] [diff] [review]
patch for review v2

while I'm waiting for my rebuild, Phil would want me say that we include trailing ';' on our statements; You're missing those in  a few places...

Comment 53

[Phil Ringnalda (:philor)]

http://beaufour.dk/jst-review/?patch=378944 has some advice, too, only some of which is bogus.

Comment 54

[David :Bienvenu]

yes, the W's and X ones at least...

Comment 55

[David Ascher (:davida)]

Attachment: patch for review v3

fix review nits from jst-in-the-cloud
make the behavior depend on the pref as discussed in IRC
fix wrapping oddity when dealing with long subjects
fix unstarring

Comment 56

[David :Bienvenu]

if I turn the pref off, multi-selection doesn't clear the message pane anymore

missing ; at eol on these lines:
+  return document.getElementById(visiblePaneId)

+    for (var i = 0; i < this._msgURIs.length; ++i)
+      this._msgHdrs.push(messenger.msgHdrFromURI(this._msgURIs[i]))

+    if ((senderName[0] == "'" && senderName[senderName.length-1] == "'") ||
+        (senderName[0] == '"' && senderName[senderName.length-1] == '"'))
+      senderName = senderName.slice(1, -1)

+      if (countUnread)
+        _mm_addClass(msg, 'unread')

+      subject.insertBefore(star, subject.firstChild);
+      wrappedsubject.appendChild(subject)

+          label += PluralForm.get(numMsgs, getStr("countUnread")).replace('#1', countUnread);
+        label += ")"

+      if (this._snippetNodes) {
+        let snippetNode = this._snippetNodes[domkey]

+        // for tags, we need to do some fancy stuff, to figure out the set
+        // of tags that correspond to all of the messages in a collapsed
+        // thread.
+        let key = messageKey + glodaMsg.folder.uri

+      msg = htmlpane.contentDocument.createElement("div");
+      let key = msgHdr.messageKey + msgHdr.folder.URI

+      wrappedsender.appendChild(star);
+      wrappedsender.appendChild(sender)

+      if (msgHdr.threadId != firstThreadId) // at least more than one thread
+        return summarizeMultipleSelection(selectedMsgUris)

no need for braces here:
+            if (aMimeMsg == null) {
+              return;
+            }

or here:
+    while (messagesElt.firstChild) {
+      messagesElt.removeChild(messagesElt.firstChild);
+    }
+
or here:
+    if (! gPrefBranch.getBoolPref("mail.operate_on_msgs_in_collapsed_threads")) {
+      return;
+    }

or here:
+  if (mCommandUpdater) {
+    mCommandUpdater->SummarizeSelection();
+  }


need newline at eof here:

+countUnread=, #1 unread;, #1 unread
+Nmessages=(#1 message);(#1 messages)
\ No newline at end of file

the long subject wrapping looks like this:

   Foo
   Bar long line...
  continue long subject

I would expect the second line to line up with the first line, but if the wrapping was intentional, I'll defer to you and Bryan.

Comment 57

[David Ascher (:davida)]

Attachment: patch for review v4

Thanks.  Apologies for the missing trailing semicolons, that's years of Python training biting me.  Nits all fixed, I think.

Pref behavior fixed.  Hanging indent problem fixed through the judicious use of negative text-indent.  I've now reached CSS brown-belt.

Comment 58

[David :Bienvenu]

years of c++ programming have made the missing trailing semicolons easy to spot :-)

This patch fixes the issues I was seeing. I thought I was seeing a lot less snippets than I was before, but I think that older messages just don't have snippets, even though they have gloda id's. I don't think I ran with Andrew's new gloda stuff that invalidates gloda db's on this machine, but I could be wrong.

Ugh, subjects with '&' in them get displayed in the summary as &amp; Much better than throwing an exception, but not ideal :-) < and > show up as &lt; etc.


<cntrl a> is really slow, and I see a ton of warnings on the console: WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLa
youtPhase_FrameC] == 0', file c:\mozilla-build\msys\tbirdhq\mozilla\layout\base\
nsPresContext.h, line 1026

I see this warning whenever I add a message to the selection, but cntrl a adds a lot of messages so I guess it slows down.

Why are we revving the uuid here?

-[scriptable, uuid(7B8F4A65-CFC4-4b3f-BF5C-152AA8D5CD10)]
+[scriptable, uuid(6844818d-297e-40e3-bcaa-273f4aa22d8c)]
 interface nsIMsgWindowCommands : nsISupports {
   void selectFolder(in ACString folderUri);
   void selectMessage(in ACString messageUri);
   void clearMsgPane();
 };

I can't believe I'm saying this, but you should K&R the brace after the else :-)

+      if (! threads[msgHdr.threadId]) {
+        threads[msgHdr.threadId] = [msgHdr];
+        numThreads += 1;
+      }
+      else
+      {
+        threads[msgHdr.threadId].push(msgHdr);
+      }

These are all nits, but I would like to fix the entity stuff before landing.

Comment 59

[David :Bienvenu]

one other small glitch. I did a select all at one point, and the message summary accurately pointed out:

(Note: 2614 messages are selected, the first 100 are shown)

But now it won't stop telling me that, even though I only have two messages selected :-)

Comment 60

[David Ascher (:davida)]

Attachment: patch for review v5

all known issues dealt with (in bug + irc + clarkbw comments), including the fact that we weren't listing authors in the multi-message case, oops.  Stars have moved to the right, where they cause a lot less trouble.

Comment 61

[David :Bienvenu]

I'm still seeing -  (Note: 1478 messages are selected, the first 100 are shown) even if I just have a couple messages selected. I need to restart to get this to go away, after doing a select all.

Comment 62

[David :Bienvenu]

Comment on attachment 379238 [details] [diff] [review]
patch for review v5

needs new patch for the counts issue, and some theming issues davida encountered. We're really close, though - it's a race w/ gloda!

Comment 63

[David Ascher (:davida)]

Attachment: patch for review v6

Ok, so getting the theme include stuff to work right took a long time, but the css looks better now.  The only thing that bugs me a bit about this layout (done w/ clarkbw) is that if you select starred messages & some threads, the stars aren't where I think they should be.  But I think that could land in a subsequent patch, as I expect we'll want to iterate on the css anyway.

Comment 64

[David :Bienvenu]

The archive button on Vista doesn't look good w/ this patch:

http://www.grabup.com/uploads/33002722051920092A0769429C22.png

Comment 65

[David :Bienvenu]

Comment on attachment 379795 [details] [diff] [review]
patch for review v6

This shouldn't be part of this patch, right?

@@ -315,6 +318,7 @@
       let progressCount = 0;
 
       // For each activity that is in progress, get its status.
+      /*
       this._activeProcesses.forEach(function (element) {
           if (element.state ==
               Components.interfaces.nsIActivityProcess.STATE_INPROGRESS &&
@@ -323,7 +327,7 @@
             ++progressCount;
           }
         });
-
+     */

really long subjects don't play well with the buttons - http://www.grabup.com/uploads/98002722051920092A0769539C07.png

Once we deal with the aforemention issues, I think we're good to go.

Comment 66

[David :Bienvenu]

When we're in multi-selection mode, and we have subject and sender for each message, what should clicking on the linkified sender do? It doesn't seem to do anything for me.

Comment 67

[David :Bienvenu]

Sorry to keep spewing issues one by one - if I have three contiguous messages selected, and I click on the second or third subject link to load that message, we first display the first message in the selection, and then the message corresponding to the clicked on link.

Comment 68

[David :Bienvenu]

(In reply to comment #67)
> Sorry to keep spewing issues one by one - if I have three contiguous messages
> selected, and I click on the second or third subject link to load that message,
> we first display the first message in the selection, and then the message
> corresponding to the clicked on link.

This is a result of the way we switch between the iframe and the vbox - we switch to the vbox, which displays the previously displayed message, and then we select and display the newly selected message. Would it be possible to switch after we've loaded the new message, or at least after we've kicked off the load of the new message, to reduce the flicker?

Comment 69

[David Ascher (:davida)]

re: comment #65: right, sorry -- that code was triggering an unrelated JS assertion on linux, so i commented it out to get this patch in shape, but it's unrelated.; will fix wrapping of long subjects


re: comments #67,68: Hmm, all I'm doing there is selectFolderMsgByKey() on the dbview, and the iframe gets swapped out as a result of the call to SelectionChanged() in nsMsgDBView.cpp.  

Could you try something like:

--- a/mail/base/content/msgMail3PaneWindow.js
+++ b/mail/base/content/msgMail3PaneWindow.js
@@ -501,21 +501,21 @@ function LoadCurrentlyDisplayedMessage()
 function LoadCurrentlyDisplayedMessage()
 {
   var scrolled = (gCurrentlyDisplayedMessage != nsMsgViewIndex_None);
   if (scrolled)
   {
     var treeView = gDBView.QueryInterface(Components.interfaces.nsITreeView);
     var treeSelection = treeView.selection;
     treeSelection.select(gCurrentlyDisplayedMessage);
-    if (treeView)
-      treeView.selectionChanged();
     EnsureRowInThreadTreeIsVisible(gCurrentlyDisplayedMessage);
     SetFocusThreadPane();
     gCurrentlyDisplayedMessage = nsMsgViewIndex_None; //reset
+    if (treeView)
+      treeView.selectionChanged();
   }
   return scrolled;
 }
 
 function IsCurrentLoadedFolder(folder)
 {
   var msgfolder = folder.QueryInterface(Components.interfaces.nsIMsgFolder);
   var currentLoadedFolder = GetThreadPaneFolder();


I'm not seeing the flicker myself, probably cause i'm not running a debug build.

Comment 70

[David :Bienvenu]

that tweak didn't help - I'll poke around a little.

Comment 71

[David :Bienvenu]

even w/o your tweak, we're starting the load of the new message before calling summarizeSelection; I guess the issue is that loading is async, and switching to the message pane is sync. We'll just have to see if it's an issue for anyone with release builds but I certainly wouldn't hold up this patch for it.

Comment 72

[David Ascher (:davida)]

Attachment: patch for review v7

Ok, so this should address the subject wrapping, the archive/trash icon button size issues (at least on XP, I don't have vista), and the extraneous commenting out.

Comment 73

[David Ascher (:davida)]

Attachment: [checked in] patch for review v8

somehow i forgot the pinstripe changes to make the trash button size properly.

Comment 74

[David :Bienvenu]

Comment on attachment 379990 [details] [diff] [review]
[checked in] patch for review v8

looks good to me...

Comment 75

[David Ascher (:davida)]

So, before we check this in, I'd like to do a bit more testing on linux -- my last experiment showed some issues w/ the stars, but my linux VM was very unstable.  I may rope asuth or clarkbw in testing the patch if I can't fix that.

Comment 76

[David Ascher (:davida)]

Test plan notes:

1) select multiple messages

--> should see the message pane display a summary of these messages (subject, author, star if starred, tags if tagged, and first few words of the body).

The summary also shows the total size of messages (news messages don't advertise their size, so expect those number to be off for usenet messages)

2) change selection

--> summary should update and reflect the new selection.

3) select a lot of messages

--> summary will show only first 100, and have a note about that at the bottom

4) select fewer messages

--> summary won't show that note at the bottom

5) select multiple message including several in the same thread

--> summary will represent the messages in the thread as one line, indicating how many messages in that thread are selected (not the number of messages in the thread total)

6) select one collapsed thread

--> summary will summarize the thread, showing initial thread title at the top, author of each message, and date of each message.

7) in thread summary, clicking on a message should select it (and display it)

8) in multiple-message summary including a thread, clicking on the thread name will select the entire thread (leaving it collapsed if collapsed, or leaving it uncollaped if not), and display the thread summary

9) in multiple-messages summary including _some_ messages from a thread, clicking on the thread description will select the subset of messages from that thread that are in the current selection, and display the thread summary.

10) if the "mail.operate_on_msgs_in_collapsed_threads" pref is set to false, then the thread summary won't show for collapsed threads.  Note: multiple message summaries won't be shown either if this pref is false.


11) in the multi-message-with-threads condition, thread summaries show stars if any of the selected messages in that thread have stars.  They also show the union of all tags assigned to those messages.

12) in both multi-message and thread summary views, hitting "S" to toggle starred/unstarred, hitting the number keys to set tags, will dynamically update the summary, as long as the full-text indexer (gloda) is enabled.

13) the snippets (first line of body) require gloda to be enabled.

Comment 77

[David Ascher (:davida)]

With a clean build linux checks out just fine.

Comment 78

[Mark Banner (:standard8)]

I'm not convinced this will work with rtl locales:

+      if (numMsgs > 1) {
+        let label = "(";
+        label += PluralForm.get(numMsgs, gSelectionSummaryStrings["numMsgs"]).replace('#1', numMsgs);
+        if (countUnread)
+          label += PluralForm.get(numMsgs, gSelectionSummaryStrings["countUnread"]).replace('#1', countUnread);
+        label += ")";
+        countNode.innerHTML = label;
+      }

However, I think that could be spun off into a follow up patch if it is a problem as I don't think it'll affect those strings already defined.

Comment 79

[Mark Banner (:standard8)]

Comment on attachment 379990 [details] [diff] [review]
[checked in] patch for review v8

Checked in: http://hg.mozilla.org/comm-central/rev/3c87898162a3

Note that I noticed the line endings in mail/themes/qute/jar.mn were messed up, so I fixed those.

Comment 80

[Mark Banner (:standard8)]

Leaving open for now - does Andreas still need to work on the visual design?

Comment 81

[Andreas Nilsson (:andreasn)]

Mark: Working on it right now. Will attach shortly.

Comment 82

[Andreas Nilsson (:andreasn)]

Attachment: mutiple messages mockup

Visual design mockup.
* Tags appear next to message, hopefully saves a bit vertical space.
* Gray rectangle to indicate thread.

Comment 83

[Mark Banner (:standard8)]

(In reply to comment #82)
> Created an attachment (id=380077) [details]
> mutiple messages mockup

I realise this is only a mockup, but to me that 'X' button would be confusing - we normally use 'X' to signify closing something, not deleting.

Comment 84

[Andreas Nilsson (:andreasn)]

Attachment: updated mockup

Mark: yes, I added it as a placeholder and missed to fix it before I attached it to the big. The button should use the theme's delete icon, just as it does now.

Comment 85

[Ludovic Hirlimann [:Usul]]

(In reply to comment #76)
> Test plan notes:
> 
> 1) select multiple messages
> 
> --> should see the message pane display a summary of these messages (subject,
> author, star if starred, tags if tagged, and first few words of the body).
> 
> The summary also shows the total size of messages (news messages don't
> advertise their size, so expect those number to be off for usenet messages)

https://litmus.mozilla.org/show_test.cgi?id=7743
 
> 2) change selection
> 
> --> summary should update and reflect the new selection.

https://litmus.mozilla.org/show_test.cgi?id=7744
 
> 3) select a lot of messages
> 
> --> summary will show only first 100, and have a note about that at the bottom
> 
https://litmus.mozilla.org/show_test.cgi?id=7745
> 4) select fewer messages
> 
> --> summary won't show that note at the bottom
> 

https://litmus.mozilla.org/show_test.cgi?id=7746

> 5) select multiple message including several in the same thread
> 
> --> summary will represent the messages in the thread as one line, indicating
> how many messages in that thread are selected (not the number of messages in
> the thread total)

https://litmus.mozilla.org/show_test.cgi?id=7748


> 6) select one collapsed thread
> 
> --> summary will summarize the thread, showing initial thread title at the top,
> author of each message, and date of each message.

https://litmus.mozilla.org/show_test.cgi?id=7747

> 
> 7) in thread summary, clicking on a message should select it (and display it)

https://litmus.mozilla.org/show_test.cgi?id=7749
 
> 8) in multiple-message summary including a thread, clicking on the thread name
> will select the entire thread (leaving it collapsed if collapsed, or leaving it
> uncollaped if not), and display the thread summary


https://litmus.mozilla.org/show_test.cgi?id=7750

> 9) in multiple-messages summary including _some_ messages from a thread,
> clicking on the thread description will select the subset of messages from that
> thread that are in the current selection, and display the thread summary.


https://litmus.mozilla.org/show_test.cgi?id=7752

> 10) if the "mail.operate_on_msgs_in_collapsed_threads" pref is set to false,
> then the thread summary won't show for collapsed threads.  Note: multiple
> message summaries won't be shown either if this pref is false.

So we will get what happens in TB2 3.0b3 , right ?

https://litmus.mozilla.org/show_test.cgi?id=7751

> 11) in the multi-message-with-threads condition, thread summaries show stars if
> any of the selected messages in that thread have stars.  They also show the
> union of all tags assigned to those messages.

so for a thread , it is stared if any messages is stared and the tags shown is the sum of tags ?

> 
> 12) in both multi-message and thread summary views, hitting "S" to toggle
> starred/unstarred, hitting the number keys to set tags, will dynamically update
> the summary, as long as the full-text indexer (gloda) is enabled.

What happens if gloda is off ?

> 13) the snippets (first line of body) require gloda to be enabled.

dito ?

Comment 86

[David Ascher (:davida)]

(In reply to comment #85)

> > 10) if the "mail.operate_on_msgs_in_collapsed_threads" pref is set to false,
> > then the thread summary won't show for collapsed threads.  Note: multiple
> > message summaries won't be shown either if this pref is false.
> 
> So we will get what happens in TB2 3.0b3 , right ?

Right, and TB2.

> 
> https://litmus.mozilla.org/show_test.cgi?id=7751
> 
> > 11) in the multi-message-with-threads condition, thread summaries show stars if
> > any of the selected messages in that thread have stars.  They also show the
> > union of all tags assigned to those messages.
> 
> so for a thread , it is stared if any messages is stared and the tags shown is
> the sum of tags ?

union - if two messages are starred "important", only one "important shows up.

> 
> > 
> > 12) in both multi-message and thread summary views, hitting "S" to toggle
> > starred/unstarred, hitting the number keys to set tags, will dynamically update
> > the summary, as long as the full-text indexer (gloda) is enabled.
> 
> What happens if gloda is off ?

The messages will be starred, but the display won't be up to date.

> > 13) the snippets (first line of body) require gloda to be enabled.
> 
> dito ?

W/o gloda, no snippets.

Comment 87

[David Ascher (:davida)]

(In reply to comment #84)
> Created an attachment (id=380086) [details]
> updated mockup
> 

So, in earlier code, the stars were on the left as you have them, but it proved really hard for me anyway to do the wrapping right.  Now that I've rediscovered table layouts, though I think we can probably get it to work.  ;-)

Comment 88

[Ludovic Hirlimann [:Usul]]

(In reply to comment #86)
> > > 11) in the multi-message-with-threads condition, thread summaries show stars if
> > > any of the selected messages in that thread have stars.  They also show the
> > > union of all tags assigned to those messages.
> > 
> > so for a thread , it is stared if any messages is stared and the tags shown is
> > the sum of tags ?
> 
> union - if two messages are starred "important", only one "important shows up.

https://litmus.mozilla.org/show_test.cgi?id=7753


> > > 12) in both multi-message and thread summary views, hitting "S" to toggle
> > > starred/unstarred, hitting the number keys to set tags, will dynamically update
> > > the summary, as long as the full-text indexer (gloda) is enabled.
> > 
> > What happens if gloda is off ?
> 
> The messages will be starred, but the display won't be up to date.

https://litmus.mozilla.org/show_test.cgi?id=7755
 
> > > 13) the snippets (first line of body) require gloda to be enabled.
> > 
> > dito ?
> 
> W/o gloda, no snippets.


https://litmus.mozilla.org/show_test.cgi?id=7754

Comment 89

[Phil Ringnalda (:philor)]

Comment on attachment 379990 [details] [diff] [review]
[checked in] patch for review v8

>diff --git a/mail/base/content/multimessageview.xhtml b/mail/base/content/multimessageview.xhtml

>+                    <hbox id="buttonhbox" align="start">
>+                        <button id="archive" class="multimessage button"
>+                                onclick="window.top.MsgArchiveSelectedMessages(null)">&archive.label;</button>

"Warning: XUL box for hbox element contained an inline #text child, forcing all its children to be wrapped in a block."

I suspect you meant label="&archive.label;".

Comment 90

[David Ascher (:davida)]

Attachment: fix for xul warning [checked in]

thanks for catching that, phil.

Comment 91

[Phil Ringnalda (:philor)]

Comment on attachment 380195 [details] [diff] [review]
fix for xul warning [checked in]

Yup, thanks.

Comment 92

[Phil Ringnalda (:philor)]

Comment on attachment 380195 [details] [diff] [review]
fix for xul warning [checked in]

http://build.mozillamessaging.com/mercurial/comm-central/rev/0863fd9a6154

Comment 93

[[:Aureliano Buendía]]

I think that the new feature has broken the usability of the splitter on message pane: I cannot move message pane to top or bottom. This happens also if I dont have select any collapsed thread.

Comment 94

[Mark Banner (:standard8)]

(In reply to comment #93)
> I think that the new feature has broken the usability of the splitter on
> message pane: I cannot move message pane to top or bottom. This happens also if
> I dont have select any collapsed thread.

The splitter works fine for me with any combination of messages selected or not selected at all:

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

Comment 95

[[:Aureliano Buendía]]

Restarting TB also hre work fine. I'm not able to reproduce now.

I try later.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090528 Lightning/1.0pre Shredder/3.0b3pre ID:20090528032146

Comment 96

[Andreas Nilsson (:andreasn)]

Attachment: mockup v4

Realized that we can actually move the number of messages number indicator after the message, if we make the backgrounds for threads use a special color.
This would also give us more space for title, author, tags and message summary for all the other messages.

Comment 97

[Ludovic Hirlimann [:Usul]]

(In reply to comment #96)
> Created an attachment (id=380427) [details]
> mockup v4
> 
Is the red stroked circle suppose to be the trash ?

Comment 98

[Andreas Nilsson (:andreasn)]

> Is the red stroked circle suppose to be the trash ?

Yes, it will use the same icon as it does now (so don't worry, no change in metaphor).
People keep wondering about this, so if I do another mockup, I'll make sure to include the right icon..

Comment 99

[[:Aureliano Buendía]]

Archive and Delete buttons not have the same height: it is desired behavior?

using

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090529 Lightning/1.0pre Shredder/3.0b3pre ID:20090529032206

attached file for details

Comment 100

[[:Aureliano Buendía]]

Attachment: Buttons aren't with same height

Comment 101

[David Ascher (:davida)]

Aureliano: Can you make a separate bug about that?  I see the same thing in the message header in XP, but I understand that may not be true in Vista.

Comment 102

[[:Aureliano Buendía]]

Ok, that's bug https://bugzilla.mozilla.org/show_bug.cgi?id=495478

Comment 103

[David Ascher (:davida)]

Attachment: patch making things faster and prettier

This patch does a few things:

1) uses a variation of andreas's css, which works much better, and is much prettier to boot.  I was getting sick of getting lost in the DOM manipulations, so I used e4x to template the message rows, which works reasonably as an alternative.

2) figures outs everything from the selected messages w/o requiring gloda, although it still kicks of a gloda collection and uses that to get notified of changes to the messages (tagging, stars, read/unread status).  This required a bunch of refactoring which turned out well.

It's not quite review ready yet because there's an XXX there which is a question to Andrew about how we want to refactor the logic that asks gloda attribute providers to do the whittling (see mimeMsgToContentSnippet).  For now I just copied the function mostly as is, but it's clearly not the final refactoring.  

Note: there is a possible problem (whether w/this bug or without) if a single message in a collapsed thread among multiple thread selections changes, where gloda won't tell me enough about the change to know accurately what tags to display.  Given the rarity, and the lack of pernicious effects, I decided to live witht that.

Apologies to localizers, but after talking to bryan we simplified the heading text, which meant a string change.

One design question for Andreas/Bryan is how to display the messages in the right place with a header that can grow if the subject is super super long.  The current setup works ok for 1/2 lines, but at 3 lines fails a bit.  Maybe that's ok?  (oh, also I can't get the individual tags to wrap, don't know why). 

David: with this patch, the failure to stream (bug 495304) still occurs for me, with imap folders and local folders both, which should make it easier to track down I hope (no gloda bug masking that one).

I haven't looked at this patch on windows or linux yet -- would appreciate css comments if anyone does try it before I get there.

Comment 104

[David Ascher (:davida)]

Attachment: _correct_ patch

I'd forgotten to refresh.  This patch passes the test suite and looks nicer.

Comment 106

[David Ascher (:davida)]

Attachment: Patch including connotent refactoring

I'd like asuth to review the gloda/connotent refactoring, and he may as well look at the other changes as I know he's been dealing w/ this code in his front-end patch.  bienvenu, sounds ok?

Comment 107

[David :Bienvenu]

sounds fine, since connotent sent me to the dictionary anyway :-)

Comment 108

[Andreas Nilsson (:andreasn)]

Attachment: mockup v5

David wanted to figure out what we should do with attachments in the mockup.
This version differ between single and multiple messages.
* 1 attachment, big mimetype icon.
* 2 or more attachments, small mimetype icons (a maximum of 4 though)

This mockup also include a line around threads, instead of a colored area (looked inactive, and gray text was invisible)

Comment 109

[Ronald Killmer]

(In reply to comment #71)
> even w/o your tweak, we're starting the load of the new message before calling
> summarizeSelection; I guess the issue is that loading is async, and switching
> to the message pane is sync. We'll just have to see if it's an issue for anyone
> with release builds but I certainly wouldn't hold up this patch for it.

I am seeing an odd behavior related to this issue.  Used with a news thread where the first post in the thread has an inline image attachment. The summary was initially displayed as designed. What happened next was wrong. I clicked the next message in the thread pane, a single posting with no replies.

What displayed was the Image from msg one of the thread previously summarized. The image did not go away to alow the new selection to display. I had to navigate away to another message and come back to read the single posting.  If I move beck and forth between the first thread and the single posting the Image of msg one of the thread was flashing in the message pane before the posting was displayed. This was more than a flicker, perhaps 1/4 second before the repaint replaced the message pane content.

OS is Vista Win32, and build tested the 06/01/2009 nightly.

Comment 110

[martin.guillon]

First i want to say how happy i am to see this new feature. That was the one thing i was missing from mail.app.

Now there is one thing really bugging me which is the sort order. In mail.app most recent mail appears first which is very cool when the conversation has more than 100 mails....
WOuld it be possible to be able to choose the sort order in the threads and consequently in the summary?
May be i should create a feature request for it?

Great work anyway

Comment 111

[David Ascher (:davida)]

Martin: thanks for the kind words. Do file a bug, and mark it blocking on this one.  Feel free to cc: me to your bug.

Comment 112

[martin.guillon]

i did(bug 495965) thanks a lot David !

Comment 113

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

Comment on attachment 380930 [details]
mockup v5

This looks really good.

I like the outline for threads, that is a nice improvement.  

The attachment icons looks pretty good.  I'm wondering if we should overlay our generic 'attachment icon' on the right top of the mime-type icons to give the hint that these are attached.

Also all these different systems are looking fairly similar I'm wondering if we should be using different colour header backgrounds for each.  Something to give a quick visual cue that you're looking at a selection of messages vs. a thread summary vs. account central.

Nice work!

Comment 114

[Mark Banner (:standard8)]

I think we said this could do with a dev doc due to the fact the message pane is now within an extra vbox, adding dev-doc-needed keyword.

Comment 115

[David Ascher (:davida)]

Attachment: revised patch w/ thread circling and css splitting

1) use the rounded border around threads
2) change the connotent refactoring to return the meta dict, so that things like the gp-bugzilla plugin can send data back to the summary viewer.  
3) move some of the css to a sharedsummary.css file, which will be used by the folder/account summaries.

Comment 116

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

Comment on attachment 381166 [details] [diff] [review]
revised patch w/ thread circling and css splitting

>diff --git a/mail/base/content/selectionsummaries.js b/mail/base/content/selectionsummaries.js
>--- a/mail/base/content/selectionsummaries.js
>+++ b/mail/base/content/selectionsummaries.js
>+      // for tags, there's a minor problem in that if _some_ of the items in a
>+      // thread got modified 

>diff --git a/mailnews/db/gloda/modules/connotent.js b/mailnews/db/gloda/modules/connotent.js
>--- a/mailnews/db/gloda/modules/connotent.js
>+++ b/mailnews/db/gloda/modules/connotent.js

>+/**
>+ * Given a MimeMsg, return the whittled content string, suitable for summarizing
>+ * a message.
>+ *
>+ * @param aMimeMsg: the MimeMessage instance
>+ * @param folder: the nsIMsgDBFolder
>+ * @param length: optional number of characters to trim the whittled content.
>+ */

Please document the return value using the @returns tag.

Please mention the automatic inclusion of ellipsis when truncating when length is specified, and whether the ellipsis makes the total number of characters length or length+1.

>+function mimeMsgToContentSnippetAndMeta(aMimeMsg, folder, length) {
>+  let content = new GlodaContent();
>+  let meta = {subject: aMimeMsg.get("subject")};
>+  let bodyLines = aMimeMsg.coerceBodyToPlaintext(folder).split(/\r?\n/);
>+
>+  for each (let [, attrProvider] in Iterator(whittlerRegistry.getWhittlers()))
>+    attrProvider.contentWhittle(meta, bodyLines, content);

Please rename attrProvider; it made sense in its orginal context, but makes less sense here.

>+  if (length) {
>+    let text = content.getContentSnippet(length + 1);
>+    if (text.length > length)
>+      text = text.substring(0, length) + "\u2026"; // ellipsis
>+    return [text, meta];
>+  } 
>+  return [content, meta];

I find it confusing that if you don't specify length, you get the content object, which is definitely not a string.  Suggest making this method always return a string representation, but only truncating if length is specified and truncation is required.

I realize this change was made to avoid code duplication with the getMessageContent method, but it turns out weird.  Perhaps a better strategy is to also introduce a mimeMsgToContentAndMeta which this method calls?

>+WhittlerRegistry.prototype = {
>+  /** Add a provider as a content whittler.
>+  */

nit: Pleas format it like this:
/**
 * Add a provider as a content whittler.
 */

>+  getWhittlers: function whittler_registry_getWhittlers() {
>+    return this._whittlers.reverse();

Reverse is mutating (it just returns the same object).  The original code used blah.concat().reverse() to avoid rep exposure and mutation of the underlying list.

>diff --git a/mailnews/db/gloda/modules/gloda.js b/mailnews/db/gloda/modules/gloda.js
>--- a/mailnews/db/gloda/modules/gloda.js
>+++ b/mailnews/db/gloda/modules/gloda.js
>@@ -320,32 +320,8 @@
>+  getMessageContent: function gloda_ns_getMessageContent(aGlodaMessage, aMimeMsg) {
>+    return mimeMsgToContentSnippet(aMimeMsg, aGlodaMessage.folderMessage.folder);
>   },

I can't help but notice that this method does not actually exist.  I am therefore suspicious as to whether this passes the unit tests.  Please verify that the unit tests pass.

Comment 117

[David :Bienvenu]

driving by (actually, running with the patch to try to track down bug 495304), I noticed some missing ; at the end of js lines...

Comment 118

[David Ascher (:davida)]

Attachment: review comments fixed

Thanks for the review.  This patch addresses all review comments, and the gloda test suite passes.

Comment 119

[M.J.G.]

Trying the version currently on comm-central, I noticed:

When moving to an unread thread, the thread overview is displayed but the first message is marked read (even though it is clearly not being read). This has the adverse side effect that hitting 'N' (next unread) moves to the second message in the thread. In order to actually read the first one, one would have to expand it (right arrow).

Keeping the first message unread would solve all issues. I'm commenting here because the bug is still in progress. Please tell me whether I should rather file a new bug.

Comment 120

[Mark Banner (:standard8)]

(In reply to comment #119)
> Keeping the first message unread would solve all issues. I'm commenting here
> because the bug is still in progress. Please tell me whether I should rather
> file a new bug.

Take a look at list of bugs this one depends on and you'll find bug 495642 which already covers your issue.

Comment 121

[M.J.G.]

(In reply to comment #120)
> (In reply to comment #119)
> > Keeping the first message unread would solve all issues. I'm commenting here
> > because the bug is still in progress. Please tell me whether I should rather
> > file a new bug.
> 
> Take a look at list of bugs this one depends on and you'll find bug 495642
> which already covers your issue.

Thanks for the hint and sorry for the noise.

Comment 122

[David Ascher (:davida)]

Attachment: works better with all the files

andreas pointed out i forgot a file.  i want hg qpostbz....

Comment 123

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

Comment on attachment 381320 [details] [diff] [review]
works better with all the files

>diff --git a/mail/base/content/selectionsummaries.js b/mail/base/content/selectionsummaries.js
>--- a/mail/base/content/selectionsummaries.js
>+++ b/mail/base/content/selectionsummaries.js
>@@ -513,34 +527,66 @@
...
>+    for (let i = 0; i < this._msgURIs.length; ++i) {
...
>+      MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
>+          if (aMimeMsg == null) /* shouldn't happen, but sometimes does? */
>+            return;
>+          let [text, meta] = mimeMsgToContentSnippetAndMeta(aMimeMsg, aMsgHdr.folder, 200)

nit, missing semicolon.

Thus concludes my delegated review.  gloda currently lives in mailnews though, so SR signoff required.

Comment 124

[David :Bienvenu]

Comment on attachment 381320 [details] [diff] [review]
works better with all the files

sr=me on the gloda parts

one nit - this second line is a little over-indented:

+  if (length && text.length > length)
+      text = text.substring(0, length-1) + "\u2026"; // ellipsis

Comment 125

[David Ascher (:davida)]

Attachment: [checked in] nits addressed

carrying forward r+ and sr+

Comment 126

[Mark Banner (:standard8)]

Comment on attachment 381364 [details] [diff] [review]
[checked in] nits addressed

Checked in: http://hg.mozilla.org/comm-central/rev/81998e37543a

Comment 127

[M.J.G.]

Hmm. With 81998e37543a I get the following new behaviour when moving (mouse click or key navigation) to the first message in an unread collapsed thread:

- The new style head pane appears with the thread subject and message count.
- The message pane (thread summary) lists the first message's author only.

If more messages in the thread are read then the summary lists more authors (not necessarily those of all read messages).

The same effect happens on old read threads. Once I reread those messages the summary page lists all authors but no snippets. I get snippets only for e-mail, not for news. Similarly, the issue above with missing authors seems to appear for news only.

This was different without 81998e37543a.

Comment 128

[Richard Marti (:Paenglab)]

I can confirm Comment #127.

I am also unable to read the first message, when I click on the sender in the summary. This works only after I read this message and then collapse the thread and click on the sender again.

Comment 129

[David Ascher (:davida)]

Attachment: workaround to deal w/ remote messages (esp. news)

I hadn't taken into account the fact that MsgHdrToMimeMessage will throw an exception when faced w/ a remote message.  This is a workaround, but I wonder if we want a different API to said function.

Comment 130

[David Ascher (:davida)]

argh, didn't mean to include the debug dump stuff.  ignore that stuff for review.

Comment 131

[Andreas Nilsson (:andreasn)]

Attachment: some css fixes

Here are some small css fixes. Put a gradient on the header (the color is switchable, so it would be easy to change if we went with the idea of different colors for different views), made the thread summaries slightly nicer looking and changed the objects using the inactive color in gnomestripe to use gray instead.
Not sure what pieces belong in the theme vs. everywhere, but the header might be something we could move to theme-specific places if it turns out it looks out of place on the other platforms.

Comment 132

[Olli Pettay [:smaug][[e-mail]]]

The current UI is strange. All I want to see is the first message in the thread to figure out whether I'm interested in the message thread.
When I first got the new UI (which contains the subject, the name of the sender and the size of the messages) I thought there was some strange bug.
Then I almost randomly clicked something and figured out that clicking the
expand-the-thread-button allows me to see the message.

As mentioned in the comment 0 in this bug, please, please include the text of 
the first message in the "thread summary" page.

For now I use mail.operate_on_msgs_in_collapsed_threads=false.

Comment 133

[Mark Banner (:standard8)]

Comment on attachment 381577 [details] [diff] [review]
workaround to deal w/ remote messages (esp. news)

I found several instances where this patch didn't work, for instance, have a collapsed unread thread where the bodies haven't been downloaded. Select it once, and you'll get "the message body hasn't been downloaded yet", select a different message then the collapsed thread again and you won't get any information.

Talking with asuth on irc:

asuth: er, un-ohhhh. but I was thinking bienvenu's new code might change our behavior there, perhaps
asuth: like, the first time you tried to cause this bug to happen, it said the right thing
asuth: but then the activeStreamListeners table got the URI put in it
Standard9: yeah second time
Standard9: it doesn't display the fact its not been downloaded
Standard9: and it runs that code with no exceptions and doesn't call the callback
asuth: right, because its callback is getting added to the list of dudes listening on that URI in activeSTreamListeners
asuth: Here is what I think:
asuth: 1) we should fix that bug where I am lazy about the unit test
asuth: 2) that code should be r-'d for using a catch-all exception as a standard control-flow concept
asuth: it should check the result of the exception to verify that it is the exception that streaming produces for things that are not offline
asuth: or better yet, the signature for MsgHdrToMimeMessage should do that itself and have a flag that says whether you want to force the streaming, etc.

Comment 134

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

The bug I was referencing was bug 478167, which is what stops the JS Mime Emitter from working on news messages.

Comment 135

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #79)
> (From update of attachment 379990 [details] [diff] [review])
> Checked in: http://hg.mozilla.org/comm-central/rev/3c87898162a3
> 
> Note that I noticed the line endings in mail/themes/qute/jar.mn were messed up,
> so I fixed those.

This checkin may have regressed bug 498227.

Comment 136

[Dan Mosedale (:dmosedale, :dmose)]

As far as I can tell, the feature patches that were being tracked by this bug are in the tree and work, meaning that this bug is effectively now a meta bug.  We don't generally block on meta bugs, so setting blocking- and wanted-.  Please nominate individual dependent bugs to block if appropriate.

Comment 137

[Mark Banner (:standard8)]

(In reply to comment #136)
> As far as I can tell, the feature patches that were being tracked by this bug
> are in the tree and work, meaning that this bug is effectively now a meta bug. 
> We don't generally block on meta bugs, so setting blocking- and wanted-. 
> Please nominate individual dependent bugs to block if appropriate.

There is still ongoing work in this bug - we need the patch to deal with remote messages (i.e. newsgroups), as otherwise this makes the summary pane pretty much useless in newsgroups. As bug 478167 moved out to b4, I think we should try and get this in b3, especially as we have a skeleton of a patch anyway.

Comment 138

[martin.guillon]

i thought i should mention how Postbox does it. Actually i think they found the perfect way to go. 
A few things i love about their approach:
- they dont indent messages in the message list. Very useful when you have more than a 10 messages. With indentation you just dont see anything anymore.
- In the message list, messages from the current selected conversation have a blue background. Very useful
- Their summary allows you to read message in the same way Gmail does. Very nice.
- Maybe the most important point for me, they order messages the other way around. Here i have newest mail on top, and in a conversation the newest message is also on top. I miss that a lot in Thunderbird.

I really think you should see how postbox does it if you didnt already. I really would love to see some of postbox features in thunderbird.
www.postbox-inc.com
What do you think?

Comment 139

[Alfred Kayser]

On the last point fo comment 138: Just click on the 'Date' label of the data column to sort on Date, with newest on top.
Other points:
ad 1: could be implemented as a preference, because I like (and need!) the indenting: create a new bugzilla entry for this.
Ad 2: Another feature, that also requires a separate bugzilla entry.
Ad 3: Not clear what you precisely are looking for? Anyway, each new feature requires a bugzilla entry.
Ad 4: Existing functionality.

Please don't use this bug or bugzilla as a discussion forum. If you want new features, first check if the application doesn't already provide it, then check if there is already a bugreport for it, and then if there is no bugreport, create one with a clear description of the request.

Comment 140

[martin.guillon]

sorry but it was not a feature request. I just thought it should be good for people who are developping this great feature in thunderbird, to know how it s down on a very similar software.
I wont comment more except for one thing. 
Sort on date is not what i am looking for. What i am looking for is for the order within a thread to be the inversed of the order within the list.
You can see that in mail.app and postbox. I already created a feature request for that

Comment 141

[David Ascher (:davida)]

Martin: I heard your request to be able to sort the thread by the inverse chronological sort order from what we have here. I'll see if I can come up with a good way to do that.

Comment 142

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with davida, removing blocking+ from this bug, as the last concrete work represented here has been spun off into bug 502839, which is a blocker.

Comment 143

[Thomas Stache]

(In reply to comment #141)
> Martin: I heard your request to be able to sort the thread by the inverse
> chronological sort order from what we have here. I'll see if I can come up with
> a good way to do that.

I think what Martin said sounds like bug 478989, a conversation view.

Comment 144

[raoul bhatia]

what about bug 474199?

Comment 145

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #136)
> As far as I can tell, the feature patches that were being tracked by this bug
> are in the tree and work, meaning that this bug is effectively now a meta bug. 

Sorry if I'm a bit slow on my response to this -- given that the quoted comment was from June 29 -- but while the feature patches being tracked by this are in the tree, they don't all work.

As mentioned in comment #135, this changeset (http://hg.mozilla.org/comm-central/rev/3c87898162a3) which was checked in as of comment #79, broke keyboard accessibility for deleting messages within threads (bug 498227).

Comment 146

[Mark Banner (:standard8)]

(In reply to comment #145)
> As mentioned in comment #135, this changeset
> (http://hg.mozilla.org/comm-central/rev/3c87898162a3) which was checked in as
> of comment #79, broke keyboard accessibility for deleting messages within
> threads (bug 498227).

You don't need to comment about that here. Just set it as blocking this one (as I am doing) and it will be tracked - it is filed so we know about it.

Comment 147

[raoul bhatia]

Q: should "summary" a) be taken literally as in "condensed presentation" or b) as the thunderbird feature which previews multiple emails in the preview-pane?

Comment 148

[Nomis101]

Yes, I know, this is an old bug, but I think you misspelled something ;-)
http://mxr.mozilla.org/comm-central/search?string=David+Ascerh

Comment 149

[foudfou]

Comment on attachment 381364 [details] [diff] [review]
[checked in] nits addressed

+      let msgContents = <div class="row">
+                          <div class="star"/>
+                          <div class="header">
+                            <div class="wrappedsubject">
+                              <div class="author">{author}</div>
+                              <div class="subject link">{subject}</div>
+                              <div class="count">{countstring}</div>
+                              <div class="tags"></div>
+                            </div>
+                            <div class="snippet"></div>
+                          </div>
+                        </div>;
...
+      let msgContents = <div class="row">
+                          <div class="star"/>
+                          <div class="header">
+                            <div class="wrappedsender">
+                              <div class="sender link">{senderName}</div>
+                              <div class="date">{date}</div>
+                              <div class="tags"></div>
+                            </div>
+                            <div class="snippet"></div>
+                          </div>
+                        </div>;

This is weird

Comment 150

[Magnus Melin [:mkmelin]]

This has been fixed since long. Closing.