Should display |References| links in message display window

RESOLVED FIXED

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
17 years ago
8 years ago

People

(Reporter: Bienvenu, Assigned: Markus Hossner)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

3.74 KB, patch
Details | Diff | Splinter Review
72.66 KB, patch
Markus Hossner
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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

17 years ago
I think there may be an existing bug on this...
QA Contact: esther → laurel
adding brendan to the ccl ist.

Updated

17 years ago
Keywords: mozilla1.0
Seth, should this be Future/helpwanted too?

/be
yes.  adding helpwanted, marking future.
Keywords: helpwanted
Target Milestone: --- → Future

Updated

17 years ago
Keywords: 4xp

Comment 5

17 years ago

*** This bug has been marked as a duplicate of 23114 ***
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Comment 6

16 years ago
verified as duplicate.
Status: RESOLVED → VERIFIED

Comment 7

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

Comment 8

16 years ago
No, this bug is not a dup. Read
<http://bugzilla.mozilla.org/show_bug.cgi?id=23114#c14> for an explanation.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---

Comment 9

16 years ago
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.
Depends on: 37654

Updated

16 years ago
Summary: should display reference links if message has them in message display window → Should display |References| links in message display window

Updated

16 years ago
Depends on: 37653
No longer depends on: 37654
This is a duplicate of bug 61521  :
no 'reference' list in mail subject bar

Comment 11

16 years ago
*** Bug 61521 has been marked as a duplicate of this bug. ***
OS: Windows NT → All
Hardware: PC → All
*** Bug 130868 has been marked as a duplicate of this bug. ***

Comment 13

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

Comment 14

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

Comment 15

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

Comment 16

15 years ago
Comment #15:

Sorry, entered my text in wrong tab :-(

Comment 17

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

Updated

14 years ago
Blocks: 176238

Comment 18

14 years ago
Created attachment 126239 [details] [diff] [review]
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

14 years ago
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

14 years ago
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

14 years ago
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

14 years ago
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.

Product: Browser → Seamonkey

Updated

12 years ago
Assignee: sspitzer → mail
Status: REOPENED → NEW

Comment 23

11 years ago
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.
*** Bug 101866 has been marked as a duplicate of this bug. ***

Updated

10 years ago
Blocks: 377250

Updated

10 years ago
Priority: P3 → --
QA Contact: laurel
Target Milestone: Future → ---
(Assignee)

Comment 25

10 years ago
Created attachment 272567 [details] [diff] [review]
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)
Attachment #272567 - Flags: superreview?(bienvenu)
Attachment #272567 - Flags: review?(neil)

Updated

10 years ago
Assignee: mail → markushossner
QA Contact: mail
(Reporter)

Comment 26

10 years ago
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

10 years ago
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.
Attachment #272567 - Flags: review?(neil) → review+
(Reporter)

Comment 28

10 years ago
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.
(Assignee)

Comment 29

10 years ago
> 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);
(Reporter)

Comment 30

10 years ago
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.
(Assignee)

Comment 31

10 years ago
Created attachment 274619 [details] [diff] [review]
Patch 2

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

(taking over r+ from Patch)
Attachment #272567 - Attachment is obsolete: true
Attachment #274619 - Flags: superreview?(bienvenu)
Attachment #274619 - Flags: review+
Attachment #272567 - Flags: superreview?(bienvenu)
(Reporter)

Comment 32

10 years ago
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!
Attachment #274619 - Flags: superreview?(bienvenu) → superreview-
(Reporter)

Comment 33

10 years ago
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.
Assignee: markushossner → mail
QA Contact: mail

Comment 34

10 years ago
(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.
(Reporter)

Comment 35

10 years ago
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.

Updated

10 years ago
Assignee: mail → markushossner
QA Contact: mail

Comment 36

10 years ago
(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

10 years ago
(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.
(Reporter)

Comment 38

10 years ago
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

10 years ago
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.  
(Assignee)

Comment 40

10 years ago
Created attachment 275101 [details] [diff] [review]
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)
Attachment #274619 - Attachment is obsolete: true
Attachment #275101 - Flags: superreview?(bienvenu)
Attachment #275101 - Flags: review+
(Reporter)

Comment 41

10 years ago
Comment on attachment 275101 [details] [diff] [review]
Patch 3

thx, Markus. Yes, I'm OK w/ not having a pref for news.
Attachment #275101 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 42

10 years ago
Landed on trunk. Yay!
Status: NEW → RESOLVED
Last Resolved: 17 years ago10 years ago
Keywords: checkin-needed, helpwanted
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 391004

Updated

10 years ago
Depends on: 391006

Comment 43

10 years ago
> 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!
"mailnews.headers.showReferences"

Comment 45

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

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

Comment 46

10 years ago
Looks like this caused Bug 391005, can you take a look Markus? Thanks!
Blocks: 391005
Duplicate of this bug: 229463
Duplicate of this bug: 229463
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

Updated

8 years ago
Blocks: 482048
You need to log in before you can comment on or make changes to this bug.