Closed Bug 474709 Opened 11 years ago Closed 10 years ago

Subscribing to a feed (triggered by an external application) while no RSS account is present, does nothing

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: whimboo, Assigned: mcsmurf)

References

Details

Attachments

(1 file, 3 obsolete files)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090102 Shredder/3.0b2pre

If you choose to use Thunderbird as your RSS reader from within Firefox and the user subscribes to a new RSS feed, Thunderbird is started but nothing happens. It only works if a RSS account was already created.

Instead of bringing up no UI the user should be asked, if he wants to create a new RSS account.

Steps:
1. Make sure no RSS account is listed in your profile
2. Open Firefox and open http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml
3. Select Thunderbird from the drop down via "Choose Application"
4. Click on "Subscribe Now"

With step 4 Thunderbird will be opened but doesn't show anything. Users will be irritated why it doesn't work.
You'd think this would have already been filed, since there's been a comment in the code forever saying something like "// should create an account if we don't have one; just fail for now," but I don't see it if it is filed.
Component: RSS → Feed Reader
Product: Thunderbird → MailNews Core
Duplicate of this bug: 499449
Flags: blocking-seamonkey2.0b2?
Flags: blocking-seamonkey2.0b2? → blocking-seamonkey2.0b2+
Attached patch Patch (obsolete) — Splinter Review
Patch seems to work fine, still want to test a few testcases though. Unfortunately that way I had to duplicate some logic of the account manager here, but I saw no better way as the account wizard code requires the data for account creation stored in a special format and the code would also need to call a few JS functions (see FinishAccount() in AccountWizard.js).
This patch also fixes the URL that the feed preview page passes to MailNews, now it prefixes the URL with "feed:" so that MailNews actually detects the feed (currently this does not work when MailNews is not open).
Assignee: nobody → bugzilla
Attached patch Patch (obsolete) — Splinter Review
Also changes the TB l10n files.
Note that I had to move the detection of MailNews/TB window below the account creation part as the code first needs to create the account before it opens a mail window as otherwise the mail window would open the Account Wizard because it does not see any accounts yet (but the RSS account and the Local Folders already exist shortly after the mail window has opened).
Attachment #396090 - Attachment is obsolete: true
Attachment #396099 - Flags: review?(bugzilla)
Comment on attachment 396099 [details] [diff] [review]
Patch

>+      var server = acctMgr.createIncomingServer("nobody", "Feeds", "rss");
>+      var bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                             .getService(Components.interfaces.nsIStringBundleService)
>+                             .createBundle("chrome://messenger-newsblog/locale/newsblog.properties");
>+
[...]
>+      server.prettyName = bundle.GetStringFromName("feeds-accountname");

replace this code with
server.prettyName = GetNewsBlogStringBundle().GetStringFromName("feeds-accountname");
(In reply to comment #4)
> This patch also fixes the URL that the feed preview page passes to MailNews,
> now it prefixes the URL with "feed:" so that MailNews actually detects the feed

You probably want to either do that conditionally, or do it in your FeedConverter.js - I didn't try the patch, but I assume that since Fx always passes feed: URIs to external apps, and Tb's commandline handler only treats it as a feed subscription attempt if it's a feed: URI, that you'll then be turning all of ours into feed:feed://example.org and feed:feed:https://example.org.
Comment on attachment 396099 [details] [diff] [review]
Patch

When Thunderbird is already open, it would not take that code path. Anyway, it's probably better to fix this in the suite version of FeedConverter.js. The case you described could for example occur (with this patch) when using SeaMonkey as feed reader within Firefox.
Attachment #396099 - Flags: review?(bugzilla)
(In reply to comment #8)
> (From update of attachment 396099 [details] [diff] [review])
> When Thunderbird is already open, it would not take that code path. Anyway,
> it's probably better to fix this in the suite version of FeedConverter.js. The
> case you described could for example occur (with this patch) when using
> SeaMonkey as feed reader within Firefox.

Actually, I'm not sure, maybe can occur in TB, too. Will create a new patch for sure.
Attached patch New patch (obsolete) — Splinter Review
Ok, this one changes FeedConverter.js in suite/ to simply prefix the URL with feed:. Normally the code would (according to the spec) need to replace http:// with feed:// and for other URLs use feed: as prefix, but the mailnews feed handler can handle both cases just fine so I used the simpler approach for all URLs.
Attachment #396099 - Attachment is obsolete: true
Attachment #396156 - Flags: review?(bugzilla)
Comment on attachment 396156 [details] [diff] [review]
New patch

>+      var accountManager = Components.classes["@mozilla.org/messenger/account-manager;1"]
>+                           .getService(Components.interfaces.nsIMsgAccountManager);

nit: align the dots, or do something like:

var accountManager =
  Components.classes["@mozilla.org/messenger/account-manager;1"]
            .getService(Components.interfaces.nsIMsgAccountManager);


>+      for (var i=0; i< allServers.Count() && !aFolder; i++)

nit: space either side of = and space before < please.

>+        var currentServer = allServers.GetElementAt(i).QueryInterface(Components.interfaces.nsIMsgIncomingServer);

Please replace this by

var currentServer = allServers.QueryElementAt(i, Components.interfaces.nsIMsgIncomingServer);

its shorter and clearer.

r=Standard8 with those fixed.
Attachment #396156 - Flags: review?(bugzilla) → review+
Attached patch PatchSplinter Review
Fixed review comments.
Attachment #396156 - Attachment is obsolete: true
Attachment #396810 - Flags: superreview?(neil)
Attachment #396810 - Flags: review+
Comment on attachment 396810 [details] [diff] [review]
Patch

Got sr+ from Standard8 via IRC.
Attachment #396810 - Flags: superreview?(neil)
Pushed http://hg.mozilla.org/comm-central/rev/220ad101680c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hardware: x86 → All
Target Milestone: --- → Thunderbird 3.0b4
You need to log in before you can comment on or make changes to this bug.