Closed
Bug 454829
Opened 16 years ago
Closed 1 years ago
summary page for collapsed threads and multiple selections
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(thunderbird_esr102 unaffected, thunderbird_esr115 unaffected, thunderbird115 unaffected)
RESOLVED
FIXED
Thunderbird 3.0b3
Tracking | Status | |
---|---|---|
thunderbird_esr102 | --- | unaffected |
thunderbird_esr115 | --- | unaffected |
thunderbird115 | --- | unaffected |
People
(Reporter: clarkbw, Assigned: andreasn)
References
(Depends on 3 open bugs, Blocks 2 open bugs)
Details
(Keywords: dev-doc-needed, Whiteboard: [andreasn to tweak css][test plan comment 26])
Attachments
(7 files, 33 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
davida
:
review+
davida
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Bienvenu
:
review+
standard8
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
(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} |
| |
'----------------------------------------------'
Reporter | ||
Comment 3•16 years ago
|
||
Also, this shares a lot of concepts with bug 452440
Comment 4•16 years ago
|
||
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.
Updated•16 years ago
|
Comment 5•16 years ago
|
||
xref bug 364896 for the outlook/the bat multi selection behavior, which arguably has it's sides.
Hardware: PC → All
Comment 6•16 years ago
|
||
... though that's for multiple single message selection, not threads.
Comment 7•16 years ago
|
||
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•16 years ago
|
||
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?
Reporter | ||
Comment 9•16 years ago
|
||
> 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•16 years ago
|
||
(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•16 years ago
|
||
(In reply to comment #10)
> A general-purpose
> multiple-selection display (bug 452440) would be even better.
Er, bug 451251.
Comment 12•16 years ago
|
||
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•16 years ago
|
||
taking because i seem to care about this issue. we'll see where that takes us.
Assignee: nobody → david.ascher
Flags: wanted-thunderbird3+
Comment 14•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #359010 -
Attachment is obsolete: true
Comment 16•16 years ago
|
||
screenshot of WIP v2. bunch of details to tweak still.
Comment 17•16 years ago
|
||
I'd like to get this in b2, but i'm going to set milestone at b3 in case wacky things show up.
Target Milestone: --- → Thunderbird 3.0b3
Comment 18•16 years ago
|
||
Comment 19•16 years ago
|
||
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.
Attachment #359227 -
Attachment is obsolete: true
Comment 20•16 years ago
|
||
It'd be better if I didn't forget files.
Attachment #363757 -
Attachment is obsolete: true
Comment 22•16 years ago
|
||
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.
Attachment #363765 -
Attachment is obsolete: true
Updated•16 years ago
|
Flags: blocking-thunderbird3+
Summary: thread summary page on collapsed threads → summary page for collapsed threads and multiple selections
Comment 23•16 years ago
|
||
* 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
Attachment #363785 -
Attachment is obsolete: true
Comment 24•16 years ago
|
||
Not that jquery isn't a nice framework, but that doesn't mean we should ship with it...
Comment 25•16 years ago
|
||
(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•16 years ago
|
||
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).
Attachment #363756 -
Attachment is obsolete: true
Attachment #375428 -
Attachment is obsolete: true
Comment 27•16 years ago
|
||
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?
Updated•16 years ago
|
Whiteboard: [needs revised patch]
Comment 28•16 years ago
|
||
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.
Attachment #375625 -
Attachment is obsolete: true
Comment 29•16 years ago
|
||
Attachment #359229 -
Attachment is obsolete: true
Comment 30•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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.
Updated•16 years ago
|
Priority: -- → P1
Comment 33•16 years ago
|
||
(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•16 years ago
|
||
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.
Attachment #375733 -
Attachment is obsolete: true
Attachment #375918 -
Flags: review?(bienvenu)
Comment 35•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
I don't know how happy interdiff will be with this, since it includes an other patch.
Comment 40•16 years ago
|
||
ok, interdiff very unhappy - I'll try to generate a patch with the same files as are in wip 10
Comment 41•16 years ago
|
||
Comment 42•16 years ago
|
||
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•16 years ago
|
||
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!)
Attachment #375918 -
Attachment is obsolete: true
Attachment #376768 -
Attachment is obsolete: true
Attachment #376779 -
Attachment is obsolete: true
Attachment #375918 -
Flags: review?(bienvenu)
Comment 44•16 years ago
|
||
Use the right patch, luke.
Attachment #377225 -
Attachment is obsolete: true
Reporter | ||
Comment 45•16 years ago
|
||
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!
Whiteboard: [needs revised patch] → [needs visual design]
Reporter | ||
Comment 46•16 years ago
|
||
assigning to andreas for now. once we have a cleaned up look we can throw this back to david to finish up
Assignee: david.ascher → nisses.mail
Comment 47•16 years ago
|
||
I've landed the grouping actions fix - let me know if there's stuff you need from me for this bug.
Comment 48•16 years ago
|
||
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.
Attachment #378926 -
Flags: review?(bienvenu)
Comment 49•16 years ago
|
||
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•16 years ago
|
||
I'm also not seeing dumpExc, which looks to be defined in GlodaTestHelper.js but not anywhere relevant to this patch...
Comment 51•16 years ago
|
||
fix subjects containing &'s
fix loading of mimemsg.js
Attachment #377240 -
Attachment is obsolete: true
Attachment #378926 -
Attachment is obsolete: true
Attachment #378944 -
Flags: review?(bienvenu)
Attachment #378926 -
Flags: review?(bienvenu)
Comment 52•16 years ago
|
||
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•16 years ago
|
||
http://beaufour.dk/jst-review/?patch=378944 has some advice, too, only some of which is bogus.
Comment 54•16 years ago
|
||
yes, the W's and X ones at least...
Comment 55•16 years ago
|
||
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
Assignee: nisses.mail → david.ascher
Attachment #378944 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #378999 -
Flags: review?
Attachment #378944 -
Flags: review?(bienvenu)
Comment 56•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #378999 -
Attachment is obsolete: true
Attachment #379013 -
Flags: review?
Attachment #378999 -
Flags: review?
Updated•16 years ago
|
Whiteboard: [needs visual design] → [needs visual design andreasn][needs review bienvenu]
Comment 58•16 years ago
|
||
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 & Much better than throwing an exception, but not ideal :-) < and > show up as < 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•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #379013 -
Attachment is obsolete: true
Attachment #379238 -
Flags: review?(bienvenu)
Attachment #379013 -
Flags: review?
Comment 61•16 years ago
|
||
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•15 years ago
|
||
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!
Attachment #379238 -
Flags: review?(bienvenu) → review-
Updated•15 years ago
|
Whiteboard: [needs visual design andreasn][needs review bienvenu] → [needs visual design andreasn][needs new patch]
Comment 63•15 years ago
|
||
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.
Attachment #379238 -
Attachment is obsolete: true
Attachment #379795 -
Flags: review?(bienvenu)
Comment 64•15 years ago
|
||
The archive button on Vista doesn't look good w/ this patch:
http://www.grabup.com/uploads/33002722051920092A0769429C22.png
Comment 65•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #379795 -
Flags: review?(bienvenu) → review-
Comment 68•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
that tweak didn't help - I'll poke around a little.
Comment 71•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #379795 -
Attachment is obsolete: true
Attachment #379971 -
Flags: review?(bienvenu)
Comment 73•15 years ago
|
||
somehow i forgot the pinstripe changes to make the trash button size properly.
Attachment #379971 -
Attachment is obsolete: true
Attachment #379990 -
Flags: review?(bienvenu)
Attachment #379971 -
Flags: review?(bienvenu)
Updated•15 years ago
|
Attachment #379990 -
Flags: review?(bienvenu) → review+
Comment 74•15 years ago
|
||
Comment on attachment 379990 [details] [diff] [review]
[checked in] patch for review v8
looks good to me...
Comment 75•15 years ago
|
||
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•15 years ago
|
||
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.
Flags: in-litmus?
Comment 77•15 years ago
|
||
With a clean build linux checks out just fine.
Keywords: checkin-needed
Whiteboard: [needs visual design andreasn][needs new patch] → [needs visual design andreasn]
Comment 78•15 years ago
|
||
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.
Whiteboard: [needs visual design andreasn] → [needs visual design andreasn][test plan comment 26]
Comment 79•15 years ago
|
||
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.
Attachment #379990 -
Attachment description: patch for review v8 → [checked in] patch for review v8
Comment 80•15 years ago
|
||
Leaving open for now - does Andreas still need to work on the visual design?
Keywords: checkin-needed
Whiteboard: [needs visual design andreasn][test plan comment 26] → [needs visual design andreasn][test plan comment 26][main patch landed]
Assignee | ||
Comment 81•15 years ago
|
||
Mark: Working on it right now. Will attach shortly.
Assignee | ||
Comment 82•15 years ago
|
||
Visual design mockup.
* Tags appear next to message, hopefully saves a bit vertical space.
* Gray rectangle to indicate thread.
Comment 83•15 years ago
|
||
(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.
Assignee | ||
Comment 84•15 years ago
|
||
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.
Attachment #380077 -
Attachment is obsolete: true
Comment 85•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
(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
Flags: in-litmus? → in-litmus+
Comment 89•15 years ago
|
||
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•15 years ago
|
||
thanks for catching that, phil.
Attachment #380195 -
Flags: review?(philringnalda)
Updated•15 years ago
|
Attachment #380195 -
Flags: review?(philringnalda) → review+
Comment 91•15 years ago
|
||
Comment on attachment 380195 [details] [diff] [review]
fix for xul warning [checked in]
Yup, thanks.
Updated•15 years ago
|
Keywords: checkin-needed
Comment 92•15 years ago
|
||
Comment on attachment 380195 [details] [diff] [review]
fix for xul warning [checked in]
http://build.mozillamessaging.com/mercurial/comm-central/rev/0863fd9a6154
Attachment #380195 -
Attachment description: fix for xul warning → fix for xul warning [checked in]
Updated•15 years ago
|
Keywords: checkin-needed
Comment 93•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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
Assignee | ||
Comment 96•15 years ago
|
||
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.
Attachment #380086 -
Attachment is obsolete: true
Comment 97•15 years ago
|
||
(In reply to comment #96)
> Created an attachment (id=380427) [details]
> mockup v4
>
Is the red stroked circle suppose to be the trash ?
Assignee | ||
Comment 98•15 years ago
|
||
> 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..
Updated•15 years ago
|
Comment 99•15 years ago
|
||
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•15 years ago
|
||
Comment 101•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #380445 -
Attachment is obsolete: true
Comment 102•15 years ago
|
||
Ok, that's bug https://bugzilla.mozilla.org/show_bug.cgi?id=495478
Comment 103•15 years ago
|
||
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•15 years ago
|
||
I'd forgotten to refresh. This patch passes the test suite and looks nicer.
Updated•15 years ago
|
Attachment #380685 -
Attachment is obsolete: true
Comment 106•15 years ago
|
||
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?
Attachment #380861 -
Flags: review?(bugmail)
Updated•15 years ago
|
Attachment #380686 -
Attachment is obsolete: true
Updated•15 years ago
|
Whiteboard: [needs visual design andreasn][test plan comment 26][main patch landed] → [andreasn to tweak css][test plan comment 26][newer patch needs review]
Comment 107•15 years ago
|
||
sounds fine, since connotent sent me to the dictionary anyway :-)
Assignee | ||
Comment 108•15 years ago
|
||
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)
Attachment #380427 -
Attachment is obsolete: true
Comment 109•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
i did(bug 495965) thanks a lot David !
Reporter | ||
Comment 113•15 years ago
|
||
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•15 years ago
|
||
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.
Keywords: dev-doc-needed
Comment 115•15 years ago
|
||
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.
Attachment #380861 -
Attachment is obsolete: true
Attachment #381166 -
Flags: review?(bugmail)
Attachment #380861 -
Flags: review?(bugmail)
Comment 116•15 years ago
|
||
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.
Attachment #381166 -
Flags: review?(bugmail) → review-
Comment 117•15 years ago
|
||
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•15 years ago
|
||
Thanks for the review. This patch addresses all review comments, and the gloda test suite passes.
Attachment #381166 -
Attachment is obsolete: true
Attachment #381197 -
Flags: review?(bugmail)
Comment 119•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
andreas pointed out i forgot a file. i want hg qpostbz....
Attachment #381197 -
Attachment is obsolete: true
Attachment #381320 -
Flags: review?(bugmail)
Attachment #381197 -
Flags: review?(bugmail)
Comment 123•15 years ago
|
||
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.
Attachment #381320 -
Flags: superreview?(bienvenu)
Attachment #381320 -
Flags: review?(bugmail)
Attachment #381320 -
Flags: review+
Updated•15 years ago
|
Attachment #381320 -
Flags: superreview?(bienvenu) → superreview+
Comment 124•15 years ago
|
||
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•15 years ago
|
||
carrying forward r+ and sr+
Attachment #381364 -
Flags: superreview+
Attachment #381364 -
Flags: review+
Updated•15 years ago
|
Attachment #381320 -
Attachment is obsolete: true
Updated•15 years ago
|
Keywords: checkin-needed
Comment 126•15 years ago
|
||
Comment on attachment 381364 [details] [diff] [review]
[checked in] nits addressed
Checked in: http://hg.mozilla.org/comm-central/rev/81998e37543a
Attachment #381364 -
Attachment description: nits addressed → [checked in] nits addressed
Updated•15 years ago
|
Keywords: checkin-needed
Comment 127•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #381577 -
Flags: review?(bienvenu)
Comment 130•15 years ago
|
||
argh, didn't mean to include the debug dump stuff. ignore that stuff for review.
Updated•15 years ago
|
Attachment #381577 -
Flags: review?(bienvenu) → review+
Updated•15 years ago
|
Whiteboard: [andreasn to tweak css][test plan comment 26][newer patch needs review] → [andreasn to tweak css][test plan comment 26]
Assignee | ||
Comment 131•15 years ago
|
||
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.
Reporter | ||
Updated•15 years ago
|
Attachment #382159 -
Attachment is patch: true
Attachment #382159 -
Attachment mime type: application/octet-stream → text/plain
Comment 132•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #381577 -
Flags: review-
Comment 134•15 years ago
|
||
The bug I was referencing was bug 478167, which is what stops the JS Mime Emitter from working on news messages.
Depends on: 478167
Comment 135•15 years ago
|
||
(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•15 years ago
|
||
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.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Comment 137•15 years ago
|
||
(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.
Flags: blocking-thunderbird3- → blocking-thunderbird3+
Comment 138•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Flags: blocking-thunderbird3+
Comment 143•15 years ago
|
||
(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.
Updated•15 years ago
|
Comment 144•15 years ago
|
||
what about bug 474199?
Comment 145•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
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?
Updated•15 years ago
|
Assignee: david.ascher → nisses.mail
Comment 148•15 years ago
|
||
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•12 years ago
|
||
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
Updated•9 years ago
|
Priority: P1 → --
Updated•2 years ago
|
Severity: normal → S3
Comment 150•1 years ago
|
||
This has been fixed since long. Closing.
Status: ASSIGNED → RESOLVED
Closed: 1 years ago
status-thunderbird115:
--- → unaffected
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•