When using the URL referenced the feed gets cut into two pieces making the feed invalid

RESOLVED FIXED in Thunderbird 24.0

Status

MailNews Core
Feed Reader
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: M Prindle, Assigned: alta88)

Tracking

unspecified
Thunderbird 24.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

3.70 KB, patch
alta88
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

When I add this url "http://houston.craigslist.org/search/sss?query=google+nexus+(N1|one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss" into my RSS feed TB will check the feed once, but when I close and reopen TB the feed is cut into two entries.  The first entry is "http://houston.craigslist.org/search/sss?query=google+nexus+(N1" and the 2nd entry is "one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss".  It appears the | in the feed is breaking the url in TB.  On craigslist the | is use to OR a statement together.

The feeds are structured like this
Craigslist (Folder)
 - Feed 1
 - Feed 2
 - Feed 3

Also, as a part of this bug once the feed is broken in to two pieces I can no longer delete the feed.  I have to go to "C:\Users\username\AppData\Roaming\Thunderbird\Profiles\ve5l8jty.user\Mail\News & Blogs" and delete everything in the folder.  I'm sure there's a way to figure out what file to delete, but I'm unable to figure it out.

Reproducible: Always

Steps to Reproduce:
1.Subscribe to the feed referenced above
2.Close TB
3.Open TB and the feed is broke
Actual Results:  
1.Feed subscribes and updates once
2. After open & close the feed is broke

Expected Results:  
1.Subscribe the feed
2.Open TB and the feed updates
(Assignee)

Comment 1

5 years ago
yes, Tb uses | as a delimiter for multiple feeds subscribed to a folder.
Status: UNCONFIRMED → NEW
Component: Folder and Message Lists → Feed Reader
Ever confirmed: true
OS: Windows 7 → All
Product: Thunderbird → MailNews Core
QA Contact: folders-message-lists → feed.reader
Hardware: x86 → All
(Assignee)

Comment 2

4 years ago
Created attachment 751366 [details] [diff] [review]
patch
Assignee: nobody → alta88
Attachment #751366 - Flags: review?(mkmelin+mozilla)

Comment 3

4 years ago
Comment on attachment 751366 [details] [diff] [review]
patch

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

::: mailnews/extensions/newsblog/content/utils.js
@@ +270,5 @@
>      let feedurls = aFolder.getStringProperty("feedUrl");
> +    if (feedurls) {
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> +                                  "\x01http", "g");
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",

we don't allow file:// feeds, at least yet. 
so you should be able to simply split on "|http", no? that should also be forward-compatible (for downgrade scenarios)

@@ +395,5 @@
> +                                        "\x01file://", "g");
> +      curFeedUrls = curFeedUrls.split("\x01");
> +    }
> +    else
> +      curFeedUrls = [];

if one if-else has braces, they all should is the style rule.

(though if yo split on "|http" you don't need an if-else at all)
(Assignee)

Comment 4

4 years ago
(In reply to Magnus Melin from comment #3)
> Comment on attachment 751366 [details] [diff] [review]
> patch
> 
> Review of attachment 751366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/utils.js
> @@ +270,5 @@
> >      let feedurls = aFolder.getStringProperty("feedUrl");
> > +    if (feedurls) {
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> > +                                  "\x01http", "g");
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",
> 
> we don't allow file:// feeds, at least yet. 
> so you should be able to simply split on "|http", no? that should also be
> forward-compatible (for downgrade scenarios)
> 

yes, but it is possible to override the isValidScheme function, which i do locally and it's pretty important for testing, so i'd like to have this handled up front.


> @@ +395,5 @@
> > +                                        "\x01file://", "g");
> > +      curFeedUrls = curFeedUrls.split("\x01");
> > +    }
> > +    else
> > +      curFeedUrls = [];
> 
> if one if-else has braces, they all should is the style rule.
> 
> (though if yo split on "|http" you don't need an if-else at all)

ok.  (that's how i had it initially but needed to include file://.)

Comment 5

4 years ago
Comment on attachment 751366 [details] [diff] [review]
patch

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

::: mailnews/extensions/newsblog/content/utils.js
@@ +389,5 @@
>  
>      let curFeedUrls = aFolder.getStringProperty("feedUrl");
> +    if (curFeedUrls) {
> +      curFeedUrls = curFeedUrls.replace(this.kFeedUrlDelimiter + "http",
> +                                        "\x01http", "g");

Maybe do this for http:// and https:// cases separately? For edge cases.
Attachment #751366 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 753281 [details] [diff] [review]
updated


yes, better to be stricter.  converted to a function as well.
Attachment #751366 - Attachment is obsolete: true
Attachment #753281 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/de4183a854a0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
(Reporter)

Comment 8

4 years ago
Thanks for getting this bug fixed.  I can now go back to using the RSS reader in TB.
You need to log in before you can comment on or make changes to this bug.