Last Comment Bug 255834 - port Thunderbird RSS/Atom reader to SeaMonkey
: port Thunderbird RSS/Atom reader to SeaMonkey
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal with 40 votes (vote)
: seamonkey2.0a2
Assigned To: Ian Neal
:
Mentors:
: 289660 (view as bug list)
Depends on: 368021 225158 240393 287284 287288 287289 287291 287292
Blocks: 286975 sm1.1 415372 463255 465258
  Show dependency treegraph
 
Reported: 2004-08-16 21:57 PDT by Adam Hauner
Modified: 2009-02-04 12:05 PST (History)
55 users (show)
asa: blocking1.8b-
kairo: blocking‑seamonkey1.0a-
csthomas: blocking‑seamonkey1.0b-
kairo: blocking‑seamonkey1.1b-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Provisional Patch v0.1 (27.64 KB, patch)
2005-03-20 12:11 PST, Ian Neal
no flags Details | Diff | Review
Classic server-newsblog.gif (878 bytes, image/gif)
2005-03-20 12:20 PST, Ian Neal
no flags Details
Modern server-newsblog.gif (891 bytes, image/gif)
2005-03-20 12:21 PST, Ian Neal
no flags Details
Tweaked provisional patch v0.1a (30.97 KB, patch)
2005-03-20 15:59 PST, Ian Neal
no flags Details | Diff | Review
Port and movement patch v0.2 (22.62 KB, patch)
2005-03-22 16:09 PST, Ian Neal
no flags Details | Diff | Review
Revised port and move patch v0.2a (24.68 KB, patch)
2005-03-23 10:55 PST, Ian Neal
neil: review-
Details | Diff | Review
Additional changes to go with 0.2a - patch v0.2am (1.82 KB, patch)
2005-04-02 13:39 PST, Ian Neal
no flags Details | Diff | Review
Updated patch v0.2b (42.01 KB, patch)
2005-04-26 09:37 PDT, Ian Neal
neil: review+
Details | Diff | Review
Revised Patch v0.2c includes css changes (44.85 KB, patch)
2005-05-08 07:27 PDT, Ian Neal
iann_bugzilla: review+
mozilla: superreview-
Details | Diff | Review
Forked newsblog patch v0.3 (235.44 KB, patch)
2005-10-11 13:34 PDT, Ian Neal
neil: review-
Details | Diff | Review
Part 1 - Switch on pulling of toolkit and building of feed service for branch v1.0 (8.22 KB, patch)
2006-08-10 16:15 PDT, Ian Neal
no flags Details | Diff | Review
Part 1 for branch - Start building of feed service v1.0a (7.03 KB, patch)
2006-08-10 17:36 PDT, Ian Neal
neil: review-
Details | Diff | Review
Part 1 for trunk - Start building of feed service v1.0 (5.03 KB, patch)
2006-08-13 16:10 PDT, Ian Neal
neil: review+
benjamin: superreview-
Details | Diff | Review
Part 1 for trunk - Start building of feed service as mini module v1.0a (7.35 KB, patch)
2006-08-17 12:12 PDT, Ian Neal
neil: review+
benjamin: superreview+
Details | Diff | Review
Strange looking options.xul dialog box (25.14 KB, image/png)
2006-08-20 15:50 PDT, Ian Neal
no flags Details
Move newsblog from mail to mailnews and hook into SeaMonkey patch v0.4 (68.88 KB, patch)
2008-10-28 11:04 PDT, Ian Neal
neil: review-
Details | Diff | Review
Revised patch to move newsblog from mail/ to mailnews/ v0.4a (119.80 KB, patch)
2008-10-30 20:11 PDT, Ian Neal
neil: review+
Details | Diff | Review
Move newsblog to mailnews/ from mail/ patch v0.4b (113.92 KB, patch)
2008-10-31 10:25 PDT, Ian Neal
iann_bugzilla: review+
Details | Diff | Review
Reorder style sheet patch v0.4c (Checkin: Comment 69) (115.66 KB, patch)
2008-10-31 15:50 PDT, Ian Neal
iann_bugzilla: review+
mozilla: superreview+
Details | Diff | Review
Packages patch v0.5 (Checkin: Comment 74) (1.83 KB, patch)
2008-11-05 16:37 PST, Ian Neal
standard8: review+
neil: superreview+
Details | Diff | Review

Description Adam Hauner 2004-08-16 21:57:21 PDT
Port code of bug 225158 "Thunderbird should act as an RSS/Atom newsreader, too"
for Seamonkey.
Comment 1 Onkar Shinde 2005-03-03 06:35:03 PST
(In reply to comment #0)

Mozilla Suite is my favourite application and it will be really great to have
RSS support in it.
Comment 2 Ian Neal 2005-03-17 06:54:28 PST
I do have a working patch, but spotted a couple of cosmetic things which I need
to check out.
Comment 3 Ian Neal 2005-03-20 12:11:44 PST
Created attachment 178061 [details] [diff] [review]
Provisional Patch v0.1

This patch adds RSS/Atom reader to mailnews by:
* Makes use of mail's newsblog extension

Also fixes the following issues in both Thunderbird and mailnews:
* Feeds not appearing in list until feed subscription is reopened
* Invalid feeds not being removed properly from the feed subscription

The icons for folder-search-rss.png in classic and modern were just copied from
qute. I will attach the new server-newsblog.gif files in a moment - I'm very
happy for someone to improve/replace all of them :-)
Comment 4 Ian Neal 2005-03-20 12:20:01 PST
Created attachment 178064 [details]
Classic server-newsblog.gif
Comment 5 Ian Neal 2005-03-20 12:21:22 PST
Created attachment 178065 [details]
Modern server-newsblog.gif
Comment 6 Ian Neal 2005-03-20 15:59:03 PST
Created attachment 178077 [details] [diff] [review]
Tweaked provisional patch v0.1a

Changes since v0.1a:
* Changed client.mk so it pulls mail/extensions/newsblog for suite
* Fixed AccountManager.js so that deleting an RSS Account and then creating a
new one without OKing Account Manager does not cause Account Name to be blank
until Account Manager is reopened.
* Fixed AccountWizard.js so that setupCopiesAndFoldersServer returns early for
rss accounts.
Comment 7 Ian Neal 2005-03-22 16:09:11 PST
Created attachment 178305 [details] [diff] [review]
Port and movement patch v0.2

Changes since v0.1a
* Spun off various non-porting fixes to dependent bugs.
* Relies on newsblog being moved from mail/extensions to mailnews/extensions in
addition changes shown in diff
Comment 8 Ian Neal 2005-03-23 10:55:04 PST
Created attachment 178384 [details] [diff] [review]
Revised port and move patch v0.2a

Changes since v0.2
* This time I've included a missing file diff for the locale files
Comment 9 Bob Nisonger 2005-03-30 08:29:44 PST
Hi, I've been messing around with this feature for bug #286975. In Thunderbird
version 0.9 (20041103) I'm having a problem getting the feature to work with the
Atom feed from NewScientist.com. I'm also wondering if one of the cosmetic
changes made will permit closure of the manage subscriptions dialog box after
entering a feed URL (in my version the dialog box must remain open until all
stories have been downloaded). Thanks.
Comment 10 Ian Neal 2005-04-02 13:39:02 PST
Created attachment 179419 [details] [diff] [review]
Additional changes to go with 0.2a - patch v0.2am

These are additional changes for mailWindowOverlay.js to go in conjunction with
patch v0.2a
Comment 11 Robert Kaiser (not working on stability any more) 2005-04-13 06:33:04 PDT
Nah, would be really nice to have, but if it doesn't make it, we won't hold the
1.0 Alpha release for it.
Comment 12 Ian Neal 2005-04-17 14:03:56 PDT
*** Bug 289660 has been marked as a duplicate of this bug. ***
Comment 13 neil@parkwaycc.co.uk 2005-04-25 07:17:09 PDT
Comment on attachment 178384 [details] [diff] [review]
Revised port and move patch v0.2a

>+  skin/classic/messenger-newsblog/newsBlogOverlay.css                   (messenger/newsblog/newsBlogOverlay.css)
Bah, if this is a required extension why can't this go in folderPane.css? :-/
note that there's no CSS in folderMenus.css to make Advanced Search use these
icons. Doing this will also avoid duplicating the trash rule.

>+  <RDF:Description about="urn:mozilla:skin:classic/1.0:messenger-newsblog"
>+                   chrome:skinVersion="1.5"/>
Shouldn't this be an #expand?

>+#subscriptionsDialog { 
>+  padding: 14px;
>+}
Bah, is this a window or a dialog? But 14px is way too much. And with that
resizer, there should not be any padding at all, at least not on the dialog.

>+#rssFeedInfoBox {
>+  border: 1px solid ThreeDShadow;
>+  -moz-border-radius: 0px;
>+  margin: 4px;
>+  padding: 0px;
>+}
>+
>+#rssFeedInfoBox textbox {
>+  background-color: transparent;
>+}
>+
>+#backgroundBox {
>+  background-color: #FFFFFF; 
>+  opacity: 0.5;
>+}
See what I said about the SMTP server list account manager panel (which I guess
doesn't have the inset style, sigh...).

>+treechildren::-moz-tree-image(folderNameCol, serverType-rss),
>+.folderMenuItem[ServerType="rss"] {
>+  list-style-image: url("chrome://messenger/skin/icons/message-news.gif");
>+}
>+
>+treechildren::-moz-tree-image(folderNameCol, isServer-true, serverType-rss),
>+.folderMenuItem[IsServer="true"][ServerType="rss"] {
>+  list-style-image: url("chrome://messenger-newsblog/skin/icons/server-newsblog.gif");
>+}
If you're going to use the standard news icons (and I don't see why not), you
might as well use the standard server icon too.

>+/* ..... Saved Search Folder, Hack to force the saved search artwork to show up for virtual folders in RSS accounts ..... */
>+
>+treechildren::-moz-tree-image(folderNameCol, serverType-rss, specialFolder-Virtual) {
>+  list-style-image: url("chrome://messenger-newsblog/skin/icons/folder-search-rss.png");
>+}
Normaly saved search folders don't have special artwork, do they? I wonder why
RSS virtual folders want some.

What happend to the thread pane styles? (And again they belong in
threadPane.css so that they'll show up in advanced search).
Comment 14 Ian Neal 2005-04-26 09:37:55 PDT
Created attachment 181873 [details] [diff] [review]
Updated patch v0.2b

Changes since v0.2a
* Uses #expand in contents.rdf files and preprocess them
* Deals with Reply to properly (as per TB)
* Adds Manage Subscriptions context menu
* Merges newsBlogOverlay.css into [folderMenus|folderPane|threadPane].css files

* Moved trash folder css entries to after rss ones so correct image is used for
rss trash folder
Comment 15 neil@parkwaycc.co.uk 2005-05-07 14:02:22 PDT
Comment on attachment 181873 [details] [diff] [review]
Updated patch v0.2b

>@@ -261,20 +261,25 @@ function ComposeMessage(type, format, fo
You might conflict with timeless :-/

>   ShowMenuItem("folderPaneContext-subscribe", (numSelected <= 1) && canSubscribeToFolder);
>   EnableMenuItem("folderPaneContext-subscribe", true);
> 
>+  // XXX: Hack for RSS servers...
>+  ShowMenuItem("folderPaneContext-rssSubscribe", (numSelected <= 1) && (serverType == "rss"));
>+  EnableMenuItem("folderPaneContext-rssSubscribe", true);
At some point it would be nice if the backend could tell give us subscribe
states for rss...

>-.folderMenuItem[SpecialFolder="Trash"] {
Probably worth moving the special folder rules en bloc to keep them together.

>+#subscriptionsDialog { 
>+  padding: 0px;
>+}
Shouldn't need this, surely?

>+#rssFeedInfoBox {
>+  border: 1px solid ThreeDShadow;
>+  -moz-border-radius: 0px;
>+  margin: 4px;
>+  padding: 0px;
>+}
We don't use ThreeDShadow in Modern... also more useless 0px...

>+#backgroundBox {
>+  background-color: #FFFFFF; 
>+  opacity: 0.5;
>+}
Um... please no. It's unreadable on my screen, at least in the Modern theme,
and doesn't work well in 256 colours, iirc.

But basically r=me.
Comment 16 Ian Neal 2005-05-08 07:27:00 PDT
Created attachment 182946 [details] [diff] [review]
Revised Patch v0.2c includes css changes

Changes since v0.2b
* Alter css as per Neil's comments
* Spun off the rss subscribe state to bug 293359

Carrying forward r=

As well as this patch is the moving of all files under
mail/extensions/newsblog
to
mailnews/extensions/newsblog

Requesting r=
Comment 17 Ian Neal 2005-05-08 07:27:54 PDT
Requesting sr= even
Comment 18 David :Bienvenu 2005-05-08 14:27:29 PDT
cc'ing mscott to make sure he's ok with thunderbird and the suite sharing the
newsblog extension.
Comment 19 Ian Neal 2005-06-27 07:27:01 PDT
I've not had any news from mscott on this, what are our options?
Comment 20 Jay 2005-07-25 11:22:01 PDT
So, if I might make a quick inquiry:

What's the current status of this feature. Anyone willing to offer an inkling?
Comment 21 mv_van_rantwijk 2005-10-08 04:31:35 PDT
(In reply to comment #18)
> cc'ing mscott to make sure he's ok with thunderbird and the suite sharing the
> newsblog extension.

Excuse me? Isn't Mozilla Thunderbird just another Open Source project? I hope
that everyone read this and blog about how stupid that comment is.
Comment 22 Vidar Haarr (not reading bugmail) 2005-10-08 04:41:06 PDT
(In reply to comment #21)
> Excuse me? Isn't Mozilla Thunderbird just another Open Source project? I hope
> that everyone read this and blog about how stupid that comment is.

You can take the Thunderbird source code and fork it into "mv_van_rantwijk Mail"
and do what you want, but as long as we're dealing with the code that produces
Mozilla.orgs binaries, the owner of this particular code (which is mscott) has
to say "OK" to changes.

If you think this is a limitation, and that everyone should be able to check
code directly into the binaries that Mozilla.org sign off and distribute, that's
an issue you should address to staff@mozilla.org or another official channel,
not the development bug database.
Comment 23 Hermann Schwab 2005-10-08 05:01:45 PDT
asking for blocking 1.0b, as there is a reviewed patch long waiting to be sr'ed.
Comment 24 David :Bienvenu 2005-10-08 10:04:15 PDT
Comment on attachment 182946 [details] [diff] [review]
Revised Patch v0.2c includes css changes

sorry, I thought we'd talked about this on IRC. Scott is not ok with
thunderbird sharing the rss files with Seamonkey, so they need to be forked for
SM. (this patch changes thunderbird, so I  assume this patch is a non-forked
version)
Comment 25 David :Bienvenu 2005-10-08 10:42:50 PDT
Comment on attachment 182946 [details] [diff] [review]
Revised Patch v0.2c includes css changes

sorry, I thought we'd talked about this on IRC. Scott is not ok with
thunderbird sharing the rss files with Seamonkey, so they need to be forked for
SM. (this patch changes thunderbird, so I  assume this patch is a non-forked
version)
Comment 26 Scott MacGregor 2005-10-08 14:02:02 PDT
(In reply to comment #25)
> (From update of attachment 182946 [details] [diff] [review] [edit])
> sorry, I thought we'd talked about this on IRC. Scott is not ok with
> thunderbird sharing the rss files with Seamonkey, so they need to be forked for
> SM. (this patch changes thunderbird, so I  assume this patch is a non-forked
> version)

Your more than welcome to pull mozilla\mail\extensions\newsblog and build it as
part of seamonkey, I'm just not interested in moving thunderbird chrome files
into mozilla\mailnews. One day I hope to have all of the thunderbird UI
organized in mozilla\mail (just like firefox has everything in mozilla\browser)
instead of being split across two directories like it is today with some of the
older chrome files which we still happen to get from mozilla\mailnews. Moving
our RSS support back to the old mailnews diretory is moving in the wrong direction.
Comment 27 David :Bienvenu 2005-10-08 14:20:55 PDT
yes, sorry, I meant to say that we didn't want to have thunderbird using
seamonkey files that it was not using before, which is what the patch
effectively did, by moving the rss files into the seamonkey directory from the
thunderbird directory, and changing thunderbird to use the seamonkey files. 
Seamonkey can use the thunderbird files as is, if it wants, or copy them; it
just can't move them - up to now, SM hasn't used any files from Thunderbird, and
I didn't want to presume that the Seamonkey owners would want to start doing that.
Comment 28 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2005-10-11 12:59:44 PDT
(In reply to comment #23)
> asking for blocking 1.0b, as there is a reviewed patch long waiting to be sr'ed.

We have multiple other large feature bugs in the same state that have been
blocking-'ed..... not blocking on features.
Comment 29 Ian Neal 2005-10-11 13:34:57 PDT
Created attachment 199192 [details] [diff] [review]
Forked newsblog patch v0.3

This patch:
* Forks the newsblog extension from mail directory
* Makes relevant RSS related changes to mailnews js files
* Makes relevant RSS related changes to mailnews css files
Comment 30 neil@parkwaycc.co.uk 2006-04-15 13:51:09 PDT
Comment on attachment 199192 [details] [diff] [review]
Forked newsblog patch v0.3

>+        var messageIDScheme = messageID.split(":")[0];
>+        if ((messageIDScheme == 'http' || messageIDScheme == 'https') &&
Use /^https?:/.test(messageID)

>+            "openComposeWindowForRSSArticle" in this)
I gues this would be false if the extension was disabled?

>+  // XXX: Hack for RSS servers...
>+  ShowMenuItem("folderPaneContext-rssSubscribe", (numSelected <= 1) && (serverType == "rss"));
>+  EnableMenuItem("folderPaneContext-rssSubscribe", true);
I assume this "works" when the extension is disabled?

>+  var rssiframe = contentWindow.document.getElementById('_mailrssiframe');
>+
>+  // if we are displaying an RSS article, we really want to scroll the nested iframe
>+  if (rssiframe)
>+    contentWindow = rssiframe.contentWindow;
This isn't the world's most reliable test, but I don't know what settings i need to trigger this, so the best I can suggest is to check for "_mailrssiframe" in contentWindow i.e. contentWindow._mailrssiframe should be the rss iframe content window.

>+/* ..... servers ..... */
Why did all this stuff move around? I can't tell what's new and what isn't.

I guess the rest of the patch is all new files, right? I'll do them later.
Comment 31 neil@parkwaycc.co.uk 2006-04-20 06:35:41 PDT
Comment on attachment 199192 [details] [diff] [review]
Forked newsblog patch v0.3

>+#ifndef MOZ_XUL_APP
Should be #ifdef MOZ_SUITE of course.

>+*  content/messenger-newsblog/Feed.js                           (content/Feed.js)
>+*  content/messenger-newsblog/FeedItem.js                       (content/FeedItem.js)
>+*  content/messenger-newsblog/feed-parser.js                    (content/feed-parser.js)
>+*  content/messenger-newsblog/file-utils.js                     (content/file-utils.js)
>+*  content/messenger-newsblog/utils.js                          (content/utils.js)
>+*  content/messenger-newsblog/feed-properties.xul               (content/feed-properties.xul)
>+*  content/messenger-newsblog/feed-properties.js                (content/feed-properties.js) 
>+*  content/messenger-newsblog/feed-subscriptions.xul            (content/feed-subscriptions.xul)
>+*  content/messenger-newsblog/feed-subscriptions.js             (content/feed-subscriptions.js)
Most of these don't need preprocessing. For the dialogs, just pick a size that doesn't look too bad on the Mac.

>+*  content/messenger-newsblog/contents.rdf                      (content/contents.rdf)
>+*  locale/en-US/messenger-newsblog/contents.rdf                 (locale/en-US/contents.rdf)
Nit: These could do with some #ifdef MOZ_XUL_APP love.

>+EXPORT_DIR	= $(DIST)/bin/defaults/isp
>+EXPORT_DIR_L10N	= $(DIST)/bin/defaults/isp/US
>+
>+libs::
>+	$(INSTALL) $(addprefix $(srcdir)/, rss.rdf) $(EXPORT_DIR)
>+	$(INSTALL) $(addprefix $(srcdir)/, rss.rdf) $(EXPORT_DIR_L10N)
Check this with KaiRo.

>+          <NC:nsIMsgIdentity>
>+          </NC:nsIMsgIdentity>
Nit: <NC:nsIMsgIdentity/>

>+  // set our custom quickMode attribute
>+  document.getElementById('useQuickMode').checked = gIncomingServer.getBoolAttribute("quickMode");
Doesn't setting prefstring="mail.server.%serverkey%.quickMode" etc. work?

>+++ mailnews/extensions/newsblog/content/debug-utils.js	2004-06-16 18:00:26.000000000 +0100
Hmm...

>+function enumerateInterfaces(obj)
>+{
>+  var interfaces = new Array();
>+  for (i in Components.interfaces)
>+  {
>+    try
>+    {
>+      obj.QueryInterface(Components.interfaces[i]);
>+      interfaces.push(i);
>+    }
>+    catch(e) {}
>+  }
>+  return interfaces;
>+}
Use instanceof instead of try/catch

>+++ mailnews/extensions/newsblog/content/edittree.xml	2004-06-16 18:00:26.000000000 +0100
Another unused file?

>+  mFeeds: new Array(),
Should be a new Object (or {}).

>+    var normalizedUrl = ioService.newURI(aUrl, null, null);
>+    normalizedUrl.host = normalizedUrl.host.toLowerCase();
>+    return normalizedUrl.spec;
newURI already lowercases the host (in schemes that have one). Just create the URI and return the spec.

>+  this.resource = aResource.QueryInterface(Components.interfaces.nsIRDFResource);
aResource is already an nsIRDFResource.

>+  author: null,
>+  items: new Array(),
Unused (items would never have worked anyway).

>+    name = name.replace(/[\n\r\t]+/g, " ");
/\s+/g

>+    this.parseItems = aParseItems == null ? true : aParseItems ? true : false;
WTF?? Everyone seems to pass true, but ? true : false; is pointless.

>+    var uri = Components.classes["@mozilla.org/network/standard-url;1"].
>+                        createInstance(Components.interfaces.nsIURI);
Don't do this.

>+    uri.spec = this.url;
>+    if (!(uri.schemeIs("http") || uri.schemeIs("https")))
>+      return this.onParseError(this); // simulate an invalid feed error
nsIIOService has an extractScheme method. Or use /^https?:/.test

>+    if (aFeed && aFeed.downloadCallback)
>+    {
>+      if (aFeed.downloadCallback)
Something's wrong here...

>+      quickMode = eval(quickMode);
I think == "true" would be safer.

>+    if (!this.itemsToStore ||  !this.itemsToStore.length)
Nit: doubled space after ||

>+    this.itemsToStoreIndex++
Nit: missing ;

>+    var uri = Components.classes["@mozilla.org/network/standard-url;1"].getService(Components.interfaces["nsIStandardURL"]);
>+    uri.init(1, 80, aVal, null, null);
>+    var uri = uri.QueryInterface(Components.interfaces.nsIURI);
Again, this should use the IO service.

>+      return this.xmlContentBase
Missing ;

>+    if (ds.hasArcOut(resource, FZ_VALID)) 
>+    {
>+      var currentValue = ds.GetTarget(resource, FZ_VALID, true);
>+      ds.Change(resource, FZ_VALID, currentValue, RDF_LITERAL_TRUE);
>+    }
>+    else 
>+      ds.Assert(resource, FZ_VALID, RDF_LITERAL_TRUE, true);
Interestingly other places get the current value and assert if it's null.

>+    title = title.replace(/[\t\r\n]+/g, " ");
\s+/g

>+    var enclosureURL  = ioService.newURI(this.mURL, null, null).QueryInterface(Components.interfaces.nsIURL);
Nit: doubled space before =

>+  var remoteToUTCOffset = 0;
>+  if (parts[13] && parts[13] != "Z") {
>+    var direction = (parts[14] == "+" ? 1 : -1);
>+    if (parts[15])
>+      remoteToUTCOffset += direction * parts[15] * HOURS_TO_MILLISECONDS;
>+    if (parts[16])
>+      remoteToUTCOffset += direction * parts[16] * MINUTES_TO_MILLISECONDS;
>+  }
>+  remoteToUTCOffset = remoteToUTCOffset * -1; // invert it
Why not use -= in the first place?

>+      var contentNode = itemNode.getElementsByTagName("content")[0];
>+      if (contentNode) 
These [0]s should almost always be .item(0)s. Also watch out for trailing spaces.

>+    return content ? content : null;
content || null;

Note to self: next file is feed-properties.js
Comment 32 neil@parkwaycc.co.uk 2006-04-26 16:19:46 PDT
Comment on attachment 199192 [details] [diff] [review]
Forked newsblog patch v0.3

>+  // trim leading and trailing white space from the url
>+  feedLocation = feedLocation.replace( /^\s+/, "").replace( /\s+$/, ""); 
Nit: trim white space from the code too :-P

>+  window.arguments[0].folderURI = document.getElementById('selectFolder').getAttribute("uri");; 
One ; suffices ;-)

>+function PickedMsgFolder(selection,pickerID)
Nit: space after comma. [Question: can you nest folders?]

>+      <column  flex="1"/>
More space trimming (I won't list any more)...

>+      <row>
>+        <separator class="thin"/>
>+      </row>
A separator doesn't need to be in a <row>.

>+              datasources="rdf:msgaccountmanager rdf:mailnewsfolders"
rdf:msgaccountmanager should be unnecessary.

>+              ref="...">
Illegal ref. Just omit the attribute.

>+const IPS = Components.interfaces.nsIPromptService;
Please rename this back to nsIPromptService

>+    var docshell = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                        .getInterface(Components.interfaces.nsIWebNavigation)
>+                        .QueryInterface(Components.interfaces.nsIDocShell);        
The .s don't seem to line up.

>+    hasNextSibling: function (aParentIndex, aIndex) 
>+    { 
>+      var item = this.getItemAtIndex(aIndex);
>+      if (item) 
>+      {
>+        // if the next node in the view has the same level as us, then we must have a next sibling...
>+        if (aIndex + 1 < gFeedSubscriptionsWindow.mFeedContainers.length )
>+          return this.getItemAtIndex(aIndex + 1).level == item.level;
>+      }
>+
>+      return false;
>+    },
This appears to be incorrectly coded, but I'm not sure what's happening here as I didn't try the code out. See if you can throw a complex tree structure at it?

>+    hasPreviousSibling: function (aIndex)
Unused?

>+      gFeedSubscriptionsWindow.mTree.treeBoxObject.rowCountChanged(aIndex, delta);
Except the aIndex row didn't actually change...

>+      // now restore selection
>+      seln.select(currentSelectionIndex);
which will make this selection hack unnecessary.

>+        window.alert(gFeedSubscriptionsWindow.mBundle.getFormattedString('newsblog-invalidFeed', [feed.url]));
>+      else if (aErrorCode == kNewsBlogRequestFailure) 
>+        window.alert(gFeedSubscriptionsWindow.mBundle.getFormattedString('newsblog-networkError', [feed.url]));
Most of the rest of the file already uses the prompt service but these seem to have slipped through.

>+  if (el.getAttribute('collapsed'))
>+    el.removeAttribute('collapsed');
el.collapsed = false; suffices (getAttribute was wrong anyway).

>+        window.alert(gFeedSubscriptionsWindow.mBundle.getFormattedString('newsblog-invalidFeed', [feed.url]));
>+      else if (aErrorCode == kNewsBlogRequestFailure) 
>+        window.alert(gFeedSubscriptionsWindow.mBundle.getFormattedString('newsblog-networkError', [feed.url]));
Most of the rest of the file already uses the prompt service but these seem to have slipped through.

>+    <hbox id="statusContainerBox" align="center">
>+      <label id="statusText" class="statusbarpanel-text" crop="right" flex="1"/> 
>+      <progressmeter id="progressMeter" collapsed="true" class="progressmeter-statusbar" style="margin-right: 5px;" mode="normal" value="0"/>
>+    </hbox>
This should be a proper status bar with a resizer.

>+                oncommand="gFeedSubscriptionsWindow.addFeed();"/>
This confused me, as there is a global addFeed function :-(

>+++ mailnews/extensions/newsblog/content/file-utils.js	2005-08-25 20:32:46.000000000 +0100
These utility functions are really too generalised for this use case.

>+function getNodeValue(node) 
>+{
>+  if (node && node.textContent)
>+    return node.textContent;
>+  else if (node && node.firstChild) 
>+  {
>+    var ret = "";
>+    for (var child = node.firstChild; child; child = child.nextSibling) 
>+    {
>+      var value = getNodeValue(child);
>+      if (value)
>+        ret += value;
>+    }
>+
>+    if (ret)
>+      return ret;
>+  }
>+
>+  return null;
>+}
This function won't find any more text content by invoking itself recursively. Just remove this and make the callers use .textContent.

>+    try{
>+      node = node.QueryInterface(Components.interfaces.nsIRDFLiteral);
>+      if (node)
>+        return node.Value;
>+    }catch(e){
>+      // if the RDF was bogus, do nothing. rethrow if it's some other problem
>+      if(!((e instanceof Components.interfaces.nsIXPCException) 
>+	    && (e.result==Components.results.NS_ERROR_NO_INTERFACE)))
>+        throw e;
>+    }	    
In fact the try/catch is bogus as this should be using instanceof.

>+function createSubscriptionsFile(file) 
>+{
>+  file = new LocalFile(file, MODE_WRONLY | MODE_CREATE);
>+  file.write('\
It might be easier to build a "default" file and copy it.

>+    var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+        .getService(Components.interfaces.nsIRDFService);
Another . that should be lined up.

>+subscribe-OPMLExportFileTitle=Thunderbird OPML Export
>+subscribe-OPMLExportFileName=MyThunderbirdFeeds.opml
Oops ;-)
Comment 33 Ian Neal 2006-08-10 16:15:19 PDT
Created attachment 233171 [details] [diff] [review]
Part 1 - Switch on pulling of toolkit and building of feed service for branch v1.0

I'm going to split this patch into different parts. Part 1, this patch (for branch):
* Alters client.mk so it pulls toolkit.
* Builds (and includes in packing) the feeds service.
Comment 34 Robert Kaiser (not working on stability any more) 2006-08-10 16:22:41 PDT
Comment on attachment 233171 [details] [diff] [review]
Part 1 - Switch on pulling of toolkit and building of feed service for branch v1.0

>Index: client.mk
>===================================================================
> MODULES_suite :=                                \
>-  $(MODULES_core)                               \
>+  $(MODULES_toolkit)                            \
>   mozilla/suite                                 \
>   $(NULL)
> 
> LOCALES_suite :=                                \
>-  $(LOCALES_core)                               \
>+  $(LOCALES_toolkit)                            \
>   $(NULL)

toolkit is already pulled by MODULES_core, and we don't care about locales (source L10n) on 1.8 branch. And on trunk, we apparently already have in place what you're proposing here. So this hunk/file is unneeded in your patch.
Comment 35 Ian Neal 2006-08-10 17:36:44 PDT
Created attachment 233185 [details] [diff] [review]
Part 1 for branch - Start building of feed service v1.0a

Changes since v1.0:
* Removed changes to client.mk as they're not needed as pointed out above.
Comment 36 Ian Neal 2006-08-13 16:10:39 PDT
Created attachment 233518 [details] [diff] [review]
Part 1 for trunk - Start building of feed service v1.0

This patch is the equivalent one for trunk:
* Starts building of toolkit feeds service.

Adding to packages of module is already covered by another bug.
Comment 37 neil@parkwaycc.co.uk 2006-08-13 16:14:09 PDT
Comment on attachment 233185 [details] [diff] [review]
Part 1 for branch - Start building of feed service v1.0a

>Index: suite/Makefile.in
>+		../toolkit/components/feeds \
We already have an #ifdef MOZ_SUITE in toolkit/components/Makefile.in

>+LIBRARY_NAME	= sfeeds
What does toolkit call its library (or doesn't it?)

>+LOCAL_INCLUDES	+= \
>+		-I$(topsrcdir)/toolkit/components/feeds/src \
>+		$(NULL)
>+
>+SHARED_LIBRARY_LIBS	= \
>+			../../toolkit/components/feeds/src/$(LIB_PREFIX)feed_s.$(LIB_SUFFIX) \
>+			$(NULL)
Presumably toolkit/componets/build uses these options, but without the ..s?

>Index: suite/components/nsSuiteFeedsModule.cpp
IMHO this belongs in suite/components/feeds/
Comment 38 Benjamin Smedberg [:bsmedberg] 2006-08-16 06:22:42 PDT
Comment on attachment 233518 [details] [diff] [review]
Part 1 for trunk - Start building of feed service v1.0

I would much prefer that you make the feeds code its own mini-component, and remove things from the build/ directory.
Comment 39 Ian Neal 2006-08-17 12:12:41 PDT
Created attachment 234266 [details] [diff] [review]
Part 1 for trunk - Start building of feed service as mini module v1.0a

Changes since v1.0:
* Move feeds into its open mini-module as suggested by benjamin
Comment 40 Ian Neal 2006-08-20 15:50:28 PDT
Created attachment 234712 [details]
Strange looking options.xul dialog box

Not sure why this happening but trying to use the firefox options.xul gives this strange layout. Listbox does not take whole width of dialog box and link does not align to the right either.
Comment 41 Philip Chee 2006-08-20 20:51:46 PDT
In the current BonEcho nightlies and in Minefield, the Choose feeds option has been moved from a subDialog to a separate prefpane. Shouldn't we follow Suite, er, suit?
Comment 42 Ian Neal 2006-08-21 13:02:57 PDT
(In reply to comment #41)
> In the current BonEcho nightlies and in Minefield, the Choose feeds option has
> been moved from a subDialog to a separate prefpane. Shouldn't we follow Suite,
> er, suit?
> 
There is still a Choose Feed Reader option from the page that is brought up when clicking on the feeds icon in the URL bar (BuildID 20060819)

Comment 43 Benjamin Smedberg [:bsmedberg] 2006-08-23 13:53:28 PDT
Comment on attachment 234266 [details] [diff] [review]
Part 1 for trunk - Start building of feed service as mini module v1.0a

Because seamonkey is built non-static, you will need to add the new component DLL to the packaging lists in xpinstall/packager. Firefox/tbird are non-static and I don't think those packaging lists will need any changes.
Comment 44 Serge Gautherie (:sgautherie) 2008-06-02 09:30:30 PDT
Ian,
Are you still working on this ?
Comment 45 Robert Kaiser (not working on stability any more) 2008-06-02 09:37:48 PDT
I think Justin (Callek) wants to look into this, but it's still more or less blocked by other issues, like feed preview being of a higher priority, reworks of the code for using the toolkit reader and ideally no RDF needing to be done, and last not least, alpha blocker being of higher priority right now.

Anyway, I guess ASSINGED to Ian might be the wrong status here, you're right.
Comment 46 Magnus Melin 2008-09-08 08:36:31 PDT
xref bug 450543 (for thunderbird to use the toolkit feed parsing)
Comment 47 Ian Neal 2008-10-28 11:04:14 PDT
Created attachment 345118 [details] [diff] [review]
Move newsblog from mail to mailnews and hook into SeaMonkey patch v0.4

This patch is mostly an unbitrotted version of 0.2c
* Moves newsblog from mail/extensions to mailnews/extensions.
* Enables SeaMonkey to build newsblog and use it.
* Adds relevant theming and entities for SeaMonkey - some are just placeholders.

No link to the browser as yet, will talk to Callek on that.
Some work on Info.plist.in to be done for OSX once I have spoken to stefanh.
Comment 48 Justin Wood (:Callek) 2008-10-28 12:07:09 PDT
IanN, does this port newsblog to the toolkit backend, or is that "TODO" still?

Otherwise, *great* and we can link the browser part of SeaMonkey in my "RSS Preview" page stuff...
Comment 49 Robert Kaiser (not working on stability any more) 2008-10-28 12:47:22 PDT
I think this doesn't actually port the code to anything else, but I've come to the conclusion (and everyone I've spoken to agreed with me on that so far) that it's probably best to move the current code to mailnews first, and only then go and improve it - AFAIK, both Thunderbird and SeaMonkey want those improvements, we can work on those together in followups.
Comment 50 Ian Neal 2008-10-28 14:29:26 PDT
(In reply to comment #48)
> IanN, does this port newsblog to the toolkit backend, or is that "TODO" still?
> 
No, just moves the newsblog with TB specific mail/ to the shared mailnews/

> Otherwise, *great* and we can link the browser part of SeaMonkey in my "RSS
> Preview" page stuff...
Yes, that is one of the next bits to do is link the "RSS Preview" page through to the Feed Reader in mailnews.
Comment 51 neil@parkwaycc.co.uk 2008-10-29 15:42:25 PDT
Comment on attachment 345118 [details] [diff] [review]
Move newsblog from mail to mailnews and hook into SeaMonkey patch v0.4

Great work here, these are basically minor nits but there are a number of them!

>+DIRS =
I'm not sure this is right, but neither am I sure whether you should use $(NULL) or simply omit the line completely. Ask ted perhaps?

>+      var messageID = hdr.messageId;
>+      var messageIDScheme = messageID ? messageID.split(":")[0] : "";
>+      if (messageIDScheme &&
>+          (messageIDScheme == 'http' || messageIDScheme == 'https') &&
>+          "openComposeWindowForRSSArticle" in this)
When is ("openComposeWindowForRSSArticle" in this) not going to be true?
Also, the convoluted messageId code looks like it could be simplified to something like /^https?:/.test(hdr.messageId) [untested]

>-  var canGetMessages =  (isServer && (serverType != "nntp") && (serverType !="none")) || isNewsgroup;
>+  var canGetMessages =  (isServer && (serverType != "nntp") && (serverType !="none")) || isNewsgroup || (serverType == "rss");
Can you actually update each feed individually? If not, I'd just make the context subscribe appear for the account.

>+  // XXX: Hack for RSS servers...
>+  ShowMenuItem("folderPaneContext-rssSubscribe", (numSelected <= 1) && (serverType == "rss"));
>+  EnableMenuItem("folderPaneContext-rssSubscribe", true);
Why not enable the "real" subscribe menuitem? You already have MsgSubscribe opening the correct dialog. (Assuming this works we can persuade TB too.)

>+  var server = (preselectedFolder) ? preselectedFolder.server : null;
>+  if (server && server.type == "rss")
Could use if (preselectedFolder && preselectedFolder.server.type = "rss")
Or if that's too long, just drop the ()s before the ?

>+  var rssiframe = contentWindow.document.getElementById('_mailrssiframe');
>+
>+  // If we are displaying an RSS article, we really want to scroll
>+  // the nested iframe.
>+  if (rssiframe)
>+    contentWindow = rssiframe.contentWindow;
[Bah, what a hack. I wonder why we can't display the article directly. I mean, it works for the start page...]

>+      if (feedHandler)
>+        feedHandler.subscribeToFeed(window.arguments[0], null, msgWindow);
>+      gStartFolderUri = null;
[I guess we can't make the new feed the start folder?]

>diff --git a/mail/locales/en-US/chrome/messenger-newsblog/am-newsblog.dtd b/suite/locales/en-US/chrome/mailnews/newsblog/am-newsblog.dtd
[I guess at some point we should look at moving these into /messenger/]

>+  // RSS / feed protocol shell integration is not working so return PR_TRUE
>+  // until it is fixed (bug 445823).
[mcsmurf might possibly be able to help you there?]

>+  if (aApps & nsIShellService::RSS)
>+    *aIsDefaultClient &= PR_TRUE;
That is a no-op, and it's better to #ifdef the lines out anyway.

The following obviously applies to Modern as much as it does for Classic (although I appreciate that you chose slightly different styles).

>+  skin/classic/messenger/icons/acct-rss.png                             (messenger/icons/acct-rss.png)
Why not stick with acct-subscribe.png?

>+  skin/classic/messenger/icons/server-rss.png                           (messenger/icons/server-rss.png)
We already have a feed icon. (If it isn't in common we can move it.)

>+  skin/classic/messenger-newsblog/icons/folder-search-rss.png           (messenger/newsblog/folder-search-rss.png)
Unused?

>+.folderMenuItem[ServerType="rss"],
> .folderMenuItem[ServerType="nntp"] {
>   list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
> }
> 
>+.folderMenuItem[IsServer="true"][ServerType="rss"][IsSecure="true"],
> .folderMenuItem[IsServer="true"][ServerType="nntp"][IsSecure="true"] {
>   list-style-image: url("chrome://messenger/skin/icons/server-news-lock.png");
> }
> 
>+.folderMenuItem[IsServer="true"][ServerType="rss"],
> .folderMenuItem[IsServer="true"][ServerType="nntp"] {
>   list-style-image: url("chrome://messenger/skin/icons/server-news.png");
> }
These rules get overridden later, and the server rules are wrong anyway (should be feed icon) (but see below) (same goes for the tree rules).

>#subscriptionsDialog {
>  padding: 0px;
>}
Isn't that the default padding?

> #rssFeedInfoBox textbox {
>   background-color: transparent;
> }
Unnecessary due to class="plain"

> .tabmail-tab[type="folder"][ServerType="rss"] .tab-icon-image ,
> treechildren::-moz-tree-image(folderNameCol, serverType-rss),
> .folderMenuItem[ServerType="rss"] {
>   list-style-image: url("chrome://messenger/skin/icons/message-news.png");
> }
Ah, this is the rule with the incorrect override (message-news is the wrong icon). You probably don't need to provide this file at all if you put the right rules in the main CSS in the first place ;-)
Comment 52 Magnus Melin 2008-10-29 23:20:40 PDT
(In reply to comment #51)
> Can you actually update each feed individually? If not, I'd just make the
> context subscribe appear for the account.

Yes, you can.

> >+  var rssiframe = contentWindow.document.getElementById('_mailrssiframe');
> >+
> >+  // If we are displaying an RSS article, we really want to scroll
> >+  // the nested iframe.
> >+  if (rssiframe)
> >+    contentWindow = rssiframe.contentWindow;
> [Bah, what a hack. I wonder why we can't display the article directly. I mean,
> it works for the start page...]

Bug 438429 is actually fixing that i think...
Comment 53 Ian Neal 2008-10-30 19:44:25 PDT
(In reply to comment #51)
> > #rssFeedInfoBox textbox {
> >   background-color: transparent;
> > }
> Unnecessary due to class="plain"
Taking it out of modern gives a computed style
background-color: rgb(199,208,217)

and for classic
background-color: rgb(236,233,233)

And looking at them visually, it does not look correct.
Comment 54 Ian Neal 2008-10-30 20:11:17 PDT
Created attachment 345656 [details] [diff] [review]
Revised patch to move newsblog from mail/ to mailnews/ v0.4a

Changes since v0.4 (for both TB and SM unless stated otherwise):
* Use test with hdr.messageId in mailCommands.js.
* Merged RSS subscribe into standard subscribe.
* Removed "Get Messages" context item for Trash folder in Feeds.
* Show "Get Messages" instead of "Get Messages For Account" on newsgroups/feeds for SM.
* Made changes to MsgSubscribe function as suggested by reviewer.
* Use DIRS = $(NULL).
* Remove now unused newsblog.dtd.
* Merged newsBlogOverlay.css into folderMenus.css / folderPane.css / threadPane.css and sorted out ordering of rules. Removed newsBlogOverlay.css.
* Moved functions openSubscriptionsDialog and openComposeWindowForRSSArticle from toolbar-icon.xul into new file newsblogOverlay.js and pull it into am-newsblog.xul and mailWindowOverlay.xul allowing duplicate code to be removed from am-newsblog.xul and toolbar-icon.xul file removed.
* Made use of genericattr="true" in am-newsblog.xul so that value of quickMode does not set/got manually allowing more code to be stripped out of am-newsblog.js.
* Changed two entries in newsblog.properties to say SeaMonkey instead of Thunderbird for SM.
* Now use acct-subscribe instead of acct-rss for SM.
* Use common feed icon instead of server-rss for SM.
* Sorted out padding round feed subscriptions dialog for SM.

We could use qute's folder-search-rss.png as a better icon for saved searches in Feeds but not made that change yet.
Comment 55 neil@parkwaycc.co.uk 2008-10-31 03:17:41 PDT
(In reply to comment #53)
> (In reply to comment #51)
> > > #rssFeedInfoBox textbox {
> > >   background-color: transparent;
> > > }
> > Unnecessary due to class="plain"
> Taking it out of modern gives a computed style
> background-color: rgb(199,208,217)
Ah yes, we need to fix bug 456595 for Modern.
Comment 56 neil@parkwaycc.co.uk 2008-10-31 04:26:52 PDT
Comment on attachment 345656 [details] [diff] [review]
Revised patch to move newsblog from mail/ to mailnews/ v0.4a

warning: 9 lines add whitespace errors
[presumably mostly reindented lines, but ask jst-review simulacrum]

>+    (isServer && (serverType != "nntp") && (serverType !="none")) ||
Just noticed that this could do with a space between != and "none"

>+  if ((numSelected <= 1) && canGetMessages)
>+    if ((serverType == "nntp" || serverType == "rss"))
>+      SetMenuItemLabel("folderPaneContext-getMessages",
>+                       gMessengerBundle.getString("getMessages"));
>+    else
>+      SetMenuItemLabel("folderPaneContext-getMessages",
>+                       gMessengerBundle.getString("getMessagesFor"));
This is wrong, it needs to be if (isServer) [getMessagesFor] else [getMessages]
r=me with this fixed.

> function onPreInit(account, accountValues)
> {
>   gIncomingServer = account.incomingServer;
> }
Heh, hardly anything left ;-)

>+  // RSS / feed protocol shell integration is not working (bug 445823).
>+  if (aApps & nsIShellService::RSS)
>+    *aIsDefaultClient &= TestForDefault(gFeedSettings, sizeof(gFeedSettings)/sizeof(SETTING));
I'm confused. Does it work or doesn't it?

> /* ::::: folder icons for menus ::::: */
> 
> .folderMenuItem {
>   list-style-image: url("chrome://messenger/skin/icons/folder-closed.png");
> }
> 
> .folderMenuItem[open="true"] {
> list-style-image: url("chrome://messenger/skin/icons/folder-open.png");
>+}
>+
>+/* ..... servers ..... */
Please put all the server entries [IsServer="true"] back where they were.

>+.folderMenuItem[ServerType="rss"],
>+.folderMenuItem[ServerType="nntp"] {
>+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
>+}
Keep this one here though, it's a folder icon, rather than a server icon.
Same thing goes for the tree icons, and for Modern, of course.

> #rssFeedInfoBox textbox {
>   background-color: transparent;
> }
I'm sure you don't need this in Classic any more, but bonus points for fixing Modern's textbox.css instead of copying this rule.
Comment 57 Robert Kaiser (not working on stability any more) 2008-10-31 04:38:06 PDT
Comment on attachment 345656 [details] [diff] [review]
Revised patch to move newsblog from mail/ to mailnews/ v0.4a

>--- a/mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
>+++ b/suite/locales/en-US/chrome/mailnews/newsblog/newsblog.properties
>@@ -4,18 +4,18 @@
>-subscribe-OPMLExportFileTitle=Thunderbird OPML Export
>-subscribe-OPMLExportFileName=MyThunderbirdFeeds.opml
>+subscribe-OPMLExportFileTitle=SeaMonkey OPML Export
>+subscribe-OPMLExportFileName=MySeaMonkeyFeeds.opml

This smells like a bug - when we use branding, we should actually make it parameterized. This might be followup fodder though.
Comment 58 Ian Neal 2008-10-31 10:25:36 PDT
Created attachment 345742 [details] [diff] [review]
Move newsblog to mailnews/ from mail/ patch v0.4b

Changes since v0.4a:
* Removed unneeded white spaces at the end of some lines.
* Put server related style rules towards end of folderMenus.css / folderPane.css.
* Fixed modern's textbox.css as per fix to bug 456595.
* Changed getMessages switching to happen only for servers.
* Removed #rssFeedInfoBox textbox entries from feed-subscription.css files.
* Revised comment for Windows Shell Service changes and commented out RSS check (RSS / feed protocol shell integration is to be completed in bug 453797).

Carrying forward r=
Comment 59 neil@parkwaycc.co.uk 2008-10-31 13:55:03 PDT
Comment on attachment 345742 [details] [diff] [review]
Move newsblog to mailnews/ from mail/ patch v0.4b

> textbox[readonly="true"] {
>   background-color: #C7D0D9;
> }
> 
> /* ::::: plain textbox ::::: */
> 
> textbox.plain {
>+  background-color: transparent;
D'oh, I said plain, but it's readonly that should be transparent.
Comment 60 neil@parkwaycc.co.uk 2008-10-31 13:56:29 PDT
No, it is plain, it's just me going senile...
Comment 61 neil@parkwaycc.co.uk 2008-10-31 13:58:35 PDT
(In reply to comment #56)
>(From update of attachment 345656 [details] [diff] [review])
>>+.folderMenuItem[ServerType="rss"],
>>+.folderMenuItem[ServerType="nntp"] {
>>+  list-style-image: url("chrome://messenger/skin/icons/folder-newsgroup.png");
>>+}
>Keep this one here though, it's a folder icon, rather than a server icon.
>Same thing goes for the tree icons, and for Modern, of course.
But I did mean keep it there, and not put it somewhere less obvious such as between Junk and Trash :-P
Comment 62 Ian Neal 2008-10-31 15:50:53 PDT
Created attachment 345802 [details] [diff] [review]
Reorder style sheet patch v0.4c (Checkin: Comment 69)

Changes since v0.4b:
* Moved non-server news/feed style rules to before special folder rules.

Carrying forward r=
Comment 63 David :Bienvenu 2008-11-03 07:30:06 PST
trying this patch now - first issue: the feed-subscriptions.css changes to the two mail themes don't apply cleanly. I'll figure out what's going on there and then run with the patch...
Comment 64 David :Bienvenu 2008-11-03 10:00:41 PST
I got the patch to apply using git-apply.

If I select the root rss account folder and click get messages, I get the following error on the console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

But it does seem to update the feeds. I'm not sure if that same error occurs w/o the patch; I'll try to check.

Unsubscribing didn't work, but that may also be true w/o the patch. I'll try that as well.
Comment 65 Philip Chee 2008-11-03 10:10:37 PST
This error occurs all over the place in Firefox trunk and in the SeaMonkey trunk browser component, so I don't think it's caused by this patch.
Comment 66 David :Bienvenu 2008-11-03 10:17:00 PST
that error occurs w/o the patch (though in this case it definitely has to do with clicking get msgs when an rss account is selected).

Unsubscribing also failed on the trunk, so this patch doesn't have anything to do with that either.

One difference this patch does cause is that any local folder, even those not in an rss account, have "subscribe..." in their context menu, and it brings up the rss subscribe ui. Is that intentional? In TB, that option is only present if it's a folder in an rss account.
Comment 67 neil@parkwaycc.co.uk 2008-11-03 16:05:49 PST
(In reply to comment #66)
> One difference this patch does cause is that any local folder, even those not
> in an rss account, have "subscribe..." in their context menu, and it brings up
> the rss subscribe ui. Is that intentional? In TB, that option is only present
> if it's a folder in an rss account.
This is a depend build bug. An obsolete copy of toolbar-icon.xul is creating an unexpected menuitem. Delete the line in $DIST/bin/chrome/messenger.manifest that refers to it.
Comment 68 David :Bienvenu 2008-11-03 17:52:03 PST
Comment on attachment 345802 [details] [diff] [review]
Reorder style sheet patch v0.4c (Checkin: Comment 69)

Neil's right, thx for the patch, Ian. I'm looking forward to collaboration on the rss stuff :-)
Comment 69 Ian Neal 2008-11-04 09:45:40 PST
Comment on attachment 345802 [details] [diff] [review]
Reorder style sheet patch v0.4c (Checkin: Comment 69)

http://hg.mozilla.org/comm-central/rev/3800bcea163a
Comment 70 Mark Banner (:standard8) 2008-11-05 14:17:56 PST
(In reply to comment #69)
> (From update of attachment 345802 [details] [diff] [review])
> http://hg.mozilla.org/comm-central/rev/3800bcea163a

The packages file changes are missing from Linux.

rss.rdf isn't in the packages file for Windows (or the Linux one ;-) ): http://mxr.mozilla.org/comm-central/search?find=%2F&string=rss.rdf

 MOZ_NONLOCALIZED_PKG_LIST = \
 	xpcom \
 	browser \
 	mail \
+	newsblog \
 	reporter \
 	$(NULL)

This is wrong, you only need to add newsblog if you're putting it in its own [newsblog] section in the packages file.
Comment 71 Ian Neal 2008-11-05 16:37:46 PST
Created attachment 346565 [details] [diff] [review]
Packages patch v0.5 (Checkin: Comment 74)

This patch:
* Adds missing entries in unix/packages
* Adds missing entry in windows/packages
* Removes unneeded entry in Makefile.in
Comment 72 Jens Hatlak (:InvisibleSmiley) 2008-11-06 02:46:59 PST
(In reply to comment #71)
> Created an attachment (id=346565) [details]
> Installer patch v0.5

I think there is more missing than that, unless the creation of ZIP builds and complete MARs depends on the installer: I installed yesterday's Windows ZIP nightly build from ftp://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-trunk and used AUS to upgrade today. In both cases the RSS option didn't show up in the New Account dialog.
Comment 73 Mark Banner (:standard8) 2008-11-06 02:55:27 PST
(In reply to comment #72)
> (In reply to comment #71)
> > Created an attachment (id=346565) [details] [details]
> > Installer patch v0.5
> 
> I think there is more missing than that, unless the creation of ZIP builds and
> complete MARs depends on the installer:

The patch was partially incorrectly named, the packages files control what goes into the zip and installer builds, and the contents of the zip and installer builds determined the MAR updates. I'd have just called it "packages patch v0.5". That doesn't really matter anyway, it will fix the problem.
Comment 74 Ian Neal 2008-11-06 14:08:21 PST
Comment on attachment 346565 [details] [diff] [review]
Packages patch v0.5 (Checkin: Comment 74)

http://hg.mozilla.org/comm-central/rev/74a1fb12f1bf
Comment 75 Serge Gautherie (:sgautherie) 2008-11-07 11:24:44 PST
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081107 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

V.Fixed

Note You need to log in before you can comment on or make changes to this bug.