support Atom Threading Extensions - RFC 4685

RESOLVED FIXED in Thunderbird 27.0

Status

enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: NotR, Assigned: alta88)

Tracking

Thunderbird 27.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb31features])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.4) Gecko/20100101 Firefox/10.0.4
Build ID: 20120420145309

Steps to reproduce:

Some Atom servers such as IBM (Lotus) Connections provide <thr:in-reply-to> elements to specify the threading of entries, per RFC 4685.


Actual results:

Thunderbird does not parse these elements into headers for use in threading entries.


Expected results:

The REF attribute should be parsed into a References: header so that entries can be threaded properly.
Reporter

Updated

7 years ago
Severity: normal → enhancement
Component: General → Feed Reader
Product: Thunderbird → MailNews Core
QA Contact: general → feed.reader
Assignee

Comment 1

6 years ago
Posted patch atomThread.patch (obsolete) — Splinter Review
threading can be tested by subcribing to any random blogger feed and also its comments feed, eg:
http://blbooks.blogspot.com/feeds/posts/default
http://blbooks.blogspot.com/feeds/comments/default
Assignee: nobody → alta88
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #811729 - Flags: review?(mkmelin+mozilla)
Comment on attachment 811729 [details] [diff] [review]
atomThread.patch

Review of attachment 811729 [details] [diff] [review]:
-----------------------------------------------------------------

Nice feature! I'd like to test it when nits below are fixed

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +25,5 @@
>    enclosures: [],
>    // TO DO: this needs to be localized.
>    title: "(no subject)",
>    author: "anonymous",
> +  inreplyto: "",

camel case plase, inReplyTo

@@ +42,5 @@
>      '    %CONTENT%\n' +
>      '  </body>\n' +
>      '</html>\n',
>  
> +  get MOZKEYS_PAD()

just inline this function, it's small and only used once

@@ +91,5 @@
>      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
>      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
>      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
>  
> +    return this.normalizeMessageID(messageID);

I don't like how this adds the <> to the messageID. The messageId is the id, the arrows are not part of it, and you can call normalizeMessageID when you need the arrows.

@@ +340,5 @@
> +    // If there is an inreplyto value, create the headers.
> +    let inreplytoHdrsStr;
> +    if (this.inreplyto)
> +      inreplytoHdrsStr = "References: " + this.inreplyto + "\n" +
> +                         "In-Reply-To: " + this.inreplyto + "\n";

let inReplyToHdrsStr = (this.inReplyTo) ? the headers : "" here directly maybe?

The In-Reply-To header is always only one id, not all the ids in the thread. (The message it's a direct reply to.)
Attachment #811729 - Flags: review?(mkmelin+mozilla) → review-
Assignee

Comment 3

6 years ago
(In reply to Magnus Melin from comment #2)
> Comment on attachment 811729 [details] [diff] [review]
> atomThread.patch
> 
> Review of attachment 811729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice feature! I'd like to test it when nits below are fixed
> 
> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +25,5 @@
> >    enclosures: [],
> >    // TO DO: this needs to be localized.
> >    title: "(no subject)",
> >    author: "anonymous",
> > +  inreplyto: "",
> 
> camel case plase, inReplyTo
> 

sure.

> @@ +42,5 @@
> >      '    %CONTENT%\n' +
> >      '  </body>\n' +
> >      '</html>\n',
> >  
> > +  get MOZKEYS_PAD()
> 
> just inline this function, it's small and only used once
> 

fine.

> @@ +91,5 @@
> >      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
> >      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
> >      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
> >  
> > +    return this.normalizeMessageID(messageID);
> 
> I don't like how this adds the <> to the messageID. The messageId is the id,
> the arrows are not part of it, and you can call normalizeMessageID when you
> need the arrows.
> 
> @@ +340,5 @@
> > +    // If there is an inreplyto value, create the headers.
> > +    let inreplytoHdrsStr;
> > +    if (this.inreplyto)
> > +      inreplytoHdrsStr = "References: " + this.inreplyto + "\n" +
> > +                         "In-Reply-To: " + this.inreplyto + "\n";
> 
> let inReplyToHdrsStr = (this.inReplyTo) ? the headers : "" here directly
> maybe?
> 

no, normalizeMessageID() with <> is split out like this because it's also used in the parser, which must create the exact same messageId string as the messageID getter for this to work.  and angle brackets are part of the RFC5322 message id syntax.

> The In-Reply-To header is always only one id, not all the ids in the thread.
> (The message it's a direct reply to.)

no, not per https://tools.ietf.org/html/rfc5322.  a message may have 1 or more parents and In-Reply-To is to reflect that.  RFC 4685 provides for multiple <inreplyto> tags which give the parent messagIds.
Assignee

Comment 4

6 years ago
Posted patch atomThread.patch (obsolete) — Splinter Review
address nits.
Attachment #811729 - Attachment is obsolete: true
Attachment #813552 - Flags: review?(mkmelin+mozilla)
Comment on attachment 813552 [details] [diff] [review]
atomThread.patch

Review of attachment 813552 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin with the below fixed

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +85,5 @@
>      FeedUtils.log.trace("FeedItem.messageID: id - " + this.id);
>      FeedUtils.log.trace("FeedItem.messageID: mURL - " + this.mURL);
>      FeedUtils.log.trace("FeedItem.messageID: title - " + this.title);
>  
> +    return this.normalizeMessageID(messageID);

please don't normalize it in the getter. The angel brackets aren't semantically part of the message id. Just add them when needed
Attachment #813552 - Flags: review?(mkmelin+mozilla) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: support Atom Threading Extensions → support Atom Threading Extensions - RFC 4685
Status: NEW → ASSIGNED
Whiteboard: [tb31features]
Assignee

Comment 6

6 years ago
updated, for checkin.
Attachment #813552 - Attachment is obsolete: true
Attachment #815388 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0f6cf88829f2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 27.0
You need to log in before you can comment on or make changes to this bug.