62033 - Should display |References| links in message display window

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[David :Bienvenu]

In 4.x, if a message was a reply to a message that we had header information
for, we would display the reference header and linkify the references, e.g., 1,
2, 3 in the message display header area. Clicking on the link would load the
appropriate message, and adjust the thread pane to select the same message.

Comment 1

[laurel]

I think there may be an existing bug on this...

Comment 2

[(not reading, please use [e-mail] instead)]

adding brendan to the ccl ist.

Comment 3

[Brendan Eich [:brendan]]

Seth, should this be Future/helpwanted too?

/be

Comment 4

[(not reading, please use [e-mail] instead)]

yes.  adding helpwanted, marking future.

Comment 5

[Keyser Sose]

*** This bug has been marked as a duplicate of 23114 ***

Comment 6

[laurel]

verified as duplicate.

Comment 7

[Ben Bucksch (:BenB)]

*** Bug 61006 has been marked as a duplicate of this bug. ***

Comment 8

[Ben Bucksch (:BenB)]

No, this bug is not a dup. Read
<http://bugzilla.mozilla.org/show_bug.cgi?id=23114#c14> for an explanation.

Comment 9

[Ben Bucksch (:BenB)]

This might actually be a dup of another bug, but I don't know the number.

Adding dependency on bug 37654. It's not a hard dependency - 4.x lived without
it, IIRC -, but that bug will make this bug vastly more useful.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

This is a duplicate of bug 61521  :
no 'reference' list in mail subject bar

Comment 11

[Jesse Ruderman]

*** Bug 61521 has been marked as a duplicate of this bug. ***

Comment 12

[Tuukka Tolvanen (sp3000)]

*** Bug 130868 has been marked as a duplicate of this bug. ***

Comment 13

[Boris 'pi' Piwinger]

*** Bug 148658 has been marked as a duplicate of this bug. ***

Comment 14

[laurel]

*** Bug 158157 has been marked as a duplicate of this bug. ***

Comment 15

[Matthias Bockelkamp]

Works for me with Build 2003011808 on WinXP using Character Coding "Western
(ISO-8859-1).

Comment 16

[Matthias Bockelkamp]

Comment #15:

Sorry, entered my text in wrong tab :-(

Comment 17

[Jo Hermans]

*** Bug 202960 has been marked as a duplicate of this bug. ***

Comment 18

[Matthias Bockelkamp]

Attachment: patch

This is a very simple patch analog to Bug #23126. It adds a hidden pref
"mailnews.headers.showReferences" which controls whether the References header
field is displayed or not.

Comment 19

[Ben Bucksch (:BenB)]

Thanks for supplying a patch, but from what I see, it doesn't add links, it only
shows the raw header field, i.e. the raw message ids. I think that is no good
for an end-user product, not even as hidden pref. You could just view the
message source, where you can also copy them. This bug is about making clickable
links, not directly showing the message ID, but just "1, 2, 3" etc. like 4.x did
or any other form that makes sense to the user. Maybe what's being done for
email addresses gves a clue, I don't know.

Comment 20

[Matthias Bockelkamp]

Yes, as I wrote there are just header fields. But I think Bug #37653 has to be
resolved next to complete this. Only if the messages can be retrieved by their
message id (=references), linkifying makes sense. I'll have a look at that
during the week.

Comment 21

[Ben Bucksch (:BenB)]

If you could fix bug 37653, that would be really great, it shoudl help in many
cases, not just this one. However, see comment 9.

Comment 22

[Karsten Düsterloh]

Regarding the linkification:
Markus Hossner (http://messageidfinder.mozdev.org) and I
(http://mnenhy.mozdev.org) are working on this, currently we're hoping to get
this into next our Add-on builds to get some "real world" testing.
I have been / am willing to get this into the core then, so maybe you're
interested in joining forces, Matthias?

The add-on solution has just one severe limitation, that patches like yours
address: the core sends either all headers or just the few needed for minimal
(aka "normal") display, so what we need is way to tell the core what headers to
send to the JS front end.

A single pref for every single possible header (like we already have for
user-agent and organization) would be pretty awkward.

Comment 23

[Ronald Killmer]

We need this IN thunderbird, not just as a Kitchen Sink addon extension. Just from the multimedia testing on secnews.netscape.com\netscape.test.mulimedia during the best years of the NC4.x suite; the value of linking to other posts was proven.  Whether the link is for reference or for including elements from the referenced post, the result was more effective and efficient discussion the subjects issues.

Comment 24

[Wayne Mery (:wsmwk)]

*** Bug 101866 has been marked as a duplicate of this bug. ***

Comment 25

[Markus Hossner]

Attachment: Patch

Patch adds clickable references to the message header ("1, 2, 3" or full message IDs).

Click on a message id opens the message in the existing or a new window. The new reference header also allows to copy single message ids or open them in the browser with a free to choose url (e.g. Google Groups)

Comment 26

[David :Bienvenu]

One issue with this patch is that it could end up opening all the dbs in all the newsgroups, if it can't find a message-id, and leaves them open. Generally, it's best to clear the msgDatabase pointer on an nsIMsgFolder if you didn't actually cause the db to get opened, or if the folder isn't open in the ui.

In particular, this function:

+function CheckForMessageIdInFolder(folder, messageId)
+{

There are several places in the code where we do something similar, but all in c++ I think. Here's one example:

http://mxr.mozilla.org/mailnews/source/mailnews/news/src/nsNewsDownloader.cpp#483

Comment 27

[[e-mail]]

Comment on attachment 272567 [details] [diff] [review]
Patch

How do you show the in-reply-to header?

Does setCursor actually do anything? I thought it was only really usefor for async stuff.

I'm not convinced about changing the view or folder just so that we can load the message.

Comment 28

[David :Bienvenu]

Markus, did my comment about opening all the dbs make sense? Basically, nsMsgDBFolder caches open db's, so when you call GetMsgDatabase, you're leaving  the db open.

this line of code in particular:

+  var messageDatabase = folder.getMsgDatabase(msgWindow);

so for every folder this is called for, the db will get left open. This could consume a ton of memory, and file handles. Ideally, we should have some background process that closes db's that are basically idle, but we don't have that now.

Comment 29

[Markus Hossner]

> How do you show the in-reply-to header?

The in-reply-to header is shown in the "view all header" mode.


> I'm not convinced about changing the view or folder just so that we can load
> the message.

But if you load a message in the mail window you want it to be selected in the thread pane and the folder to be selected in the folder pane, don't you? But if you don't like this you could set "mailnews.messageid.openInNewWindow" and the message is opened in a new window without changing view and folder.


> Basically, nsMsgDBFolder caches open db's, so when you call GetMsgDatabase,
> you're leaving the db open.

And what do you suggest?


1. m_currentFolder->SetMsgDatabase(nsnull);

2. messageDatabase.Close(true);

3. messageDatabase.forceFolderDBClosed(folder);

Comment 30

[David :Bienvenu]

please see #26 again - I suggested that you close the db, if the newsgroup in question is not open in a window. The mxr link shows you how to determine that, at least from c++, though it should work fine from js as well.

Comment 31

[Markus Hossner]

Attachment: Patch 2

New Patch clears the msgDatabase pointer after checking for a message id (see #26)

(taking over r+ from Patch)

Comment 32

[David :Bienvenu]

Comment on attachment 274619 [details] [diff] [review]
Patch 2

I'm running with this patch now. It works pretty well, but I came across a problem - clicking a reference link to a msg that didn't exist threw a js exception in SearchForMessageIdInSubFolder because the current folder didn't have sub-folders, so subFolders.currentItem() threw an exception. This is because I have a news server with nothing subscribed.

Anyway, the fix is:

  // search subfolders recursively
  var done = !folder.hasSubFolders;

Also, there are a couple places where you do this:

+      gDBView.selectMsgByKey(messageHeader.messageKey);
+      gDBView.loadMessageByMsgKey(messageHeader.messageKey);

You shouldn't need the second call - selectMsgByKey in the thread pane loads the message.

There are several places in the code where local vars are used just once, so you really don't need them, unless it's critical for readability.

For example:

+  var menuitem  = document.getElementById("messageIdContext-messageIdTarget");
+  var messageid = messageIdNode.getAttribute("messageid");
+
+  if (messageIdNode)
+    menuitem.setAttribute("label", messageid);

could just be:
  if (messageIdNode)
    document.getElementById("messageIdContext-messageIdTarget")
        .setAttribute("label", messageIdNode.getAttribute("messageid");

I don't think var startFolder is needed here, since it's only used once:

+function OpenMessageForMessageId(messageId)
+{
+  var startFolder = msgWindow.openFolder;
+  var startServer = msgWindow.openFolder.server;
+  var messageHeader;
+
+  window.setCursor("wait");
+
+  // first search in current folder for message id
+  var messageHeader = CheckForMessageIdInFolder(startFolder, messageId);

similarly, openInNewWindow here:

+  // else show error message
+  if (messageHeader)
+  {
+    var openInNewWindow = pref.getBoolPref("mailnews.messageid.openInNewWindow");
+
+    OpenMessageByHeader(messageHeader, openInNewWindow);
+  }

So if you could just make one last pass tightening up the code, I'll be happy to sr it. Thx for the patch!

Comment 33

[David :Bienvenu]

one other thing - with this patch, by default, we're showing reference links for mail messages. I don't think we want that. I can see showing them for news messages by default, but not mail. Yes, I know mailing lists are sometimes the same as newsgroups, but I still don't think we want to show references by default for mail messages.

Comment 34

[Daniel Küstner]

(In reply to comment #33)
> one other thing - with this patch, by default, we're showing reference links
> for mail messages. I don't think we want that.

I think, this is useful also for mail, because this way you can easily follow a conversation. It used to be default in Netscape 4.

Comment 35

[David :Bienvenu]

I understand it's useful for some users and situations. I'm just saying as a default, I think most users aren't going to want that, and it clutters the message header area quite a bit.

Comment 36

[Daniel Küstner]

(In reply to comment #35)
Yes, some users might not want to have it. And it's would be ok to have it not on by default. But I think, it even makes more sense with mail than with news, because:

While in news you usually have all messages of one thread (including the ones you wrote yourself) visible in the header pane, in a mail conversation with one partner, your own messages are in the "Sent" folder and his messages in the Inbox. Thus, in the mail scenario it makes more sense to have references links in order to easily switch to the referred message and to follow conversation than when reading news.

Comment 37

[Ronald Killmer]

(In reply to comment #36)
> While in news you usually have all messages of one thread (including the ones
> you wrote yourself) visible in the header pane, in a mail conversation with one
> partner, your own messages are in the "Sent" folder and his messages in the
> Inbox.

A very valuable use of the ref headers happens frequently in mozilla.support.Fx or Tb when an orphan RE: pops up due to a post to a thread that has rolled off my headers list after 6 days. Having clickable Ref header links will cause reloading of a prior post to establish a context for the orphan RE:.  I can cite other examples for there use except they are specialty cases not frequently needed outside a testing methodology.

Comment 38

[David :Bienvenu]

My opinion is that there should be two prefs, one for mail, one for news; news should default to true, and mail to false.

Comment 39

[Ronald Killmer]

I concur with your views on default settings. I suggest that the Newsworthy add-in is a good initial UI for the news default.  Options > Advanced may be a good location for the mail default toggle. I do not like sending newbie users roaming through prefs with the about:config tools when doubng U2U support if there is a UI available.

If the Newsworthy add-in gets integrated in Options we can get a footprint win without the extension package overhead.  

Comment 40

[Markus Hossner]

Attachment: Patch 3

Fixed problems mentioned by David in comment #32

References are now shown for news messages and could be activated for mail messages too by pref.

I think one pref for activating references in mail message is enough. There are already too many prefs for activating and deactivating special header fields and two prefs for just one header field would be really a bit too much.

(taking over r+ from Patch)

Comment 41

[David :Bienvenu]

Comment on attachment 275101 [details] [diff] [review]
Patch 3

thx, Markus. Yes, I'm OK w/ not having a pref for news.

Comment 42

[Karsten Düsterloh]

Landed on trunk. Yay!

Comment 43

[Steffen Wilberg]

> References are now shown for news messages and could be activated for mail
> messages too by pref.
And that pref is mailnews.headers.references. Cool!

Comment 44

[Caio Tiago Oliveira (asrail)]

"mailnews.headers.showReferences"

Comment 45

[Robert Kaiser]

(In reply to comment #44)
> "mailnews.headers.showReferences"

How we have a UI pref in SeaMonkey for that one?

Comment 46

[Scott MacGregor]

Looks like this caused Bug 391005, can you take a look Markus? Thanks!

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Markus, since you're fixing the message header pane, could I ask you to 
look at these other bugs in the message header page?
bug 355662 
bug 355893
bug 382642