Port code of bug 225158 "Thunderbird should act as an RSS/Atom newsreader, too" for Seamonkey.

(In reply to comment #0) Mozilla Suite is my favourite application and it will be really great to have RSS support in it.

I do have a working patch, but spotted a couple of cosmetic things which I need to check out.

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 :-)

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.

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

Changes since v0.2 * This time I've included a missing file diff for the locale files

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.

These are additional changes for mailWindowOverlay.js to go in conjunction with patch v0.2a

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.

*** Bug 289660 has been marked as a duplicate of this bug. ***

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).

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 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.

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=

Requesting sr= even

cc'ing mscott to make sure he's ok with thunderbird and the suite sharing the newsblog extension.

I've not had any news from mscott on this, what are our options?

So, if I might make a quick inquiry: What's the current status of this feature. Anyone willing to offer an inkling?

(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.

(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.

asking for blocking 1.0b, as there is a reviewed patch long waiting to be sr'ed.

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 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)

(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.

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.

(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.

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 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 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 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 ;-)

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 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.

Changes since v1.0: * Removed changes to client.mk as they're not needed as pointed out above.

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 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 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.

Changes since v1.0: * Move feeds into its open mini-module as suggested by benjamin

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.

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?

(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 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.

Ian, Are you still working on this ?

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.

xref bug 450543 (for thunderbird to use the toolkit feed parsing)

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.

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...

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.

(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 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 ;-)

(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...

(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.

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.

(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 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 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.

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 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.

No, it is plain, it's just me going senile...

(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

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

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...

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.

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.

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.

(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 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 on attachment 345802 [details] [diff] [review] Reorder style sheet patch v0.4c (Checkin: Comment 69) http://hg.mozilla.org/comm-central/rev/3800bcea163a

(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.

This patch: * Adds missing entries in unix/packages * Adds missing entry in windows/packages * Removes unneeded entry in Makefile.in

(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.

(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 on attachment 346565 [details] [diff] [review] Packages patch v0.5 (Checkin: Comment 74) http://hg.mozilla.org/comm-central/rev/74a1fb12f1bf

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081107 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4) V.Fixed