Closed Bug 471932 Opened 11 years ago Closed 11 years ago

Do some tidy up/fixing of feed subscription and related code

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

Attachments

(2 files, 6 obsolete files)

Patch coming up to fix various issues with mailnews feed code.
Attached patch Feed tidy patch v0.1 (obsolete) — Splinter Review
So far this patch does:
* Fixes drag and drop in the subscription dialog
* Corrects how item progress is displayed when adding a new feed
* Moves feed deletion code out of feed-subscription so that it can be shared
* Stops virtual folders being display in feed-subscription window
* Stops feeds which are in Trash from being updated
* Moving feeds out of the Trash folder (undeleting) now works and can be edited properly in feed-subscription
* Emptying Trash now properly deletes feeds and feeditems from their respective rdf files
* Changes where FolderDeleted is called from so should propagate down nested folders
* Switches from FolderListener to MsgFolderListener (which is the future apparently)

Done testing and everything appears to compile and work on Linux.
Attachment #355186 - Flags: review?(neil)
Attachment #355186 - Flags: review?(mnyromyr)
Attachment #355186 - Flags: superreview?(bienvenu)
Comment on attachment 355186 [details] [diff] [review]
Feed tidy patch v0.1

>+++ b/mail/base/content/msgMail3PaneWindow.js
>+  if (!resource || GetFolderAttribute(tree, resource, "IsServer") == 'true')

"GetFolderAttribute is not a function" (or whatever that error message actually is).
This should be a better version of the TB part of the patch.
Comment on attachment 355186 [details] [diff] [review]
Feed tidy patch v0.1

>+function IsChildOfTrash(tree, resource)
>+{
>+  if (!resource || GetFolderAttribute(tree, resource, "IsServer") == 'true')
>+    return false;
>+  if (GetFolderAttribute(tree, resource, "SpecialFolder") == 'Trash')
>+    return true;
>+  return IsChildOfTrash(tree, resource.parent);
>+}
You should really be calling IsSpecialFolder, but be sure that you're passing it an nsIMsgFolder as .parent doesn't exist for true resources; perhaps you got lucky and xpconnect handed you a wrapper that knew it was an nsIMsgFolder too.
Attachment #355186 - Flags: review?(neil) → review+
Attachment #355186 - Flags: superreview?(bienvenu)
Attachment #355186 - Flags: review?(mnyromyr)
Changes since v0.1:
* Use IsSpecialFolder for checking heritage of folder.

Carrying forward r= from Neil
Attachment #355186 - Attachment is obsolete: true
Attachment #355485 - Attachment is obsolete: true
Attachment #357432 - Flags: review+
Attachment #357432 - Flags: review?(mnyromyr)
sid0 said on IRC:
you'll also need to update a test, "test_nsIMsgFolderListenerLocal.js", as right now it assumes that you'll only get top level notifications and you'll need to add additional events to gExpectedEvents, about each subfolder (in whichever order the notifications are sent)
nice!


> * Emptying Trash now properly deletes feeds and feeditems from their respective
rdf files

hmm, is there also a prescription for orphaned feeds?  i.e fixup of already partial info in the RDF?
Comment on attachment 357432 [details] [diff] [review]
Feed tidy using IsSpecialFolder patch v0.1a

First of all, this patch didn't apply cleanly for me on today's SM trunk - I tried to fix it up manually, but may have failed:

applying /home/kd/moz/patches/fremde/471932patch0_1a.diff
patching file mailnews/extensions/newsblog/content/utils.js
Hunk #2 succeeded at 125 with fuzz 2 (offset 0 lines).
patching file mailnews/extensions/newsblog/js/newsblog.js
Hunk #1 succeeded at 76 with fuzz 2 (offset 0 lines).
Hunk #2 FAILED at 199
1 out of 2 hunks FAILED -- saving rejects to file mailnews/extensions/newsblog/js/newsblog.js.rej

I didn't check TB, all following comments are SM based.

- Drag'n'drop in the subscribe dialog now works.
- But double right click there acts like double left click, which is odd (and should be turned off without a context menu). 
- Minor: The delete key doesn't work in the subscribe dialog.
- The subscribe tree should have tree lines.
- Undelete seems to work correctly now.
- I still see virtual folders in the subscribe dialog, although they are iconed as normal folders.
And I got lot's of these when using the subscribe dialog:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "IsSpecialFolder is not defined" {file: "file:///home/kd/projekte/mozilla/mozilla.org/obj/sr/mozilla/dist/bin/components/newsblog.js" line: 85}]' when calling method: [nsINewsBlogFeedDownloader::downloadFeed]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************
- What do you mean by "Corrects how item progress is displayed when adding a new feed"? I don't see any difference.

>+++ b/mailnews/extensions/newsblog/content/feed-subscriptions.js
>+      var feed = this.storeFeed(feedProperties);
>+ 
>+      if (feed)

Drop empty line.

The rest looks okay otherwise.
Attachment #357432 - Flags: review?(mnyromyr) → review-
(In reply to comment #8)
> (From update of attachment 357432 [details] [diff] [review])
> First of all, this patch didn't apply cleanly for me on today's SM trunk - I
> tried to fix it up manually, but may have failed:
> 
> applying /home/kd/moz/patches/fremde/471932patch0_1a.diff
> patching file mailnews/extensions/newsblog/content/utils.js
> Hunk #2 succeeded at 125 with fuzz 2 (offset 0 lines).
> patching file mailnews/extensions/newsblog/js/newsblog.js
> Hunk #1 succeeded at 76 with fuzz 2 (offset 0 lines).
> Hunk #2 FAILED at 199
> 1 out of 2 hunks FAILED -- saving rejects to file
> mailnews/extensions/newsblog/js/newsblog.js.rej
Not sure what is happening here as I get:
applying bug471932patch0_1a.diff
patching file mailnews/extensions/newsblog/js/newsblog.js
Hunk #2 succeeded at 208 with fuzz 2 (offset 1 line).
Now at: bug471932patch0_1a.diff
> 
> - But double right click there acts like double left click, which is odd (and
> should be turned off without a context menu). 
Will look at this, never looked at what double clicking does.
> - Minor: The delete key doesn't work in the subscribe dialog.
Again, I will look at this.
> - The subscribe tree should have tree lines.
I will into what they are and how to put them in.
> - Undelete seems to work correctly now.
> - I still see virtual folders in the subscribe dialog, although they are iconed
> as normal folders.
Virtual folders should be flagged as such, so if it does not have the icon then css does not think it is a virtual folder either.
> And I got lot's of these when using the subscribe dialog.
IsSpecialFolder is in commandglue.js and for some reason your build cannot see that function, I've not been able to reproduce the error.
> - What do you mean by "Corrects how item progress is displayed when adding a
> new feed"? I don't see any difference.
When you subscribe to a new feed (say of 48 items), the count would always stop at one below the maximum and the progress bar would not reach 100%. This is because the code started counting from zero rather than one.
Attached patch Feed tidy patch v0.1b (obsolete) — Splinter Review
Changes since v0.1b:
* Double click only works for left mouse button now.
* Added treelines attribute to subscription dialog.
* Delete key deletes feeds in subscription dialog.
* Some tidy up of onKeyPress code.

Still looking at test changes and orphaned feed question.
Attachment #357432 - Attachment is obsolete: true
Attachment #358619 - Flags: review?(mnyromyr)
Comment on attachment 358619 [details] [diff] [review]
Feed tidy patch v0.1b

- This patch applies cleanly now. :)
- I still get quite some of these errors, one for each feed subscribed:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "IsSpecialFolder is not defined" {file: "file:///home/kd/projekte/mozilla/mozilla.org/obj/sr/mozilla/dist/bin/components/newsblog.js" line: 85}]' when calling method: [nsINewsBlogFeedDownloader::downloadFeed]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

But the subscribe dialog is innocent, the problem is the automatic load of new feed messages (I set it to every minute): newsblogs.js is not loaded into the main window context, hence downloadFeed can't access commandglue's IsSpecialFolder. (It'd probably have a chance as aMsgWindow.IsSpecialFolder if aMsgWindow wasn't always null?)

- Virtual folders still show up in subscribe, so I'm not quite sure we're talking about the same thing here? (I'll attach a screenshot shortly.)
- Maybe it'd worth fixing bug 471151 as well while being in the vicinity? ;-)
Attachment #358619 - Flags: review?(mnyromyr) → review-
Changes since v0.1b:
* newsblog.js now has its own function for checking if the folder is in Trash.
* adjusts account manager pane as requested.
* adds flag check to makeFolderObject to stop Virtual folders which are subfolders showing in feed subscription dialog box.
* fixes up test so that it deals with additional folderDeleted notification.

Still got to look at what to do with orphaned feeds.
Attachment #358619 - Attachment is obsolete: true
Attachment #359146 - Flags: review?(mnyromyr)
My apologies, this has bitrotted again. :(
Attachment #359146 - Flags: review?(mnyromyr)
Attached patch Feed tidy patch v0.1e (obsolete) — Splinter Review
Changes since v0.1c:
* unbitrotted - not sure what happened to 0.1d though.
Attachment #359146 - Attachment is obsolete: true
Attachment #366420 - Flags: review?(mnyromyr)
Comment on attachment 366420 [details] [diff] [review]
Feed tidy patch v0.1e

>diff --git a/mailnews/extensions/newsblog/content/am-newsblog.xul b/mailnews/extensions/newsblog/content/am-newsblog.xul
>-      <label value="&accountName.label;" control="server.prettyName"
>+      <label value="&accountName.label;"
>+             control="server.prettyName"
>              accesskey="&accountName.accesskey;"/>

accesskey should always directly follow the attribute it relates to and control/aria-labelledby be last before any event handlers. (Same goes for some items below, too, where don't comment on.)

>diff --git a/mailnews/extensions/newsblog/content/feed-subscriptions.js b/mailnews/extensions/newsblog/content/feed-subscriptions.js
>+        folderEnumerator.getNext()
>+                        .QueryInterface(Components.interfaces.nsIMsgFolder);
>+      if (!folder.getFlag(Components.interfaces.nsMsgFolderFlags.Virtual))
>+        folderObject.children
>+                    .push(this.makeFolderObject(folder, aCurrentLevel + 1));

Using "if ... instanceof" instead of the QI would be even safer. (Same below.)

>+    if (aEvent.keyCode == aEvent.DOM_VK_ENTER ||
>+        aEvent.keyCode == aEvent.DOM_VK_RETURN)
>+      this.editFeed();
>+    if (aEvent.keyCode == aEvent.DOM_VK_DELETE)
>+      this.removeFeed();

It's somewhat unlikely to fulfill the second "if" condition if the first one did match already... ;-)

>     item = this.mView.getItemAtIndex(seln.currentIndex);
>-      
>+ 
>     this.updateFeedData(item);
>-        
>+ 

Just drop the empty lines completely here.

>     var feedsAdded = false;
>  
>     for (var index = 0; index < outlines.length; index++)
>+      feedsAdded = this.importOutline(outlines[index]) || feedsAdded;

Hm, you're calculating and setting feedsAdded even if unnecessary. 
  if (this.importOutline(outlines[index]) && !feedsAdded)
    feedsAdded = true;
should be slightly more efficient.

>+      var feedName;
>+      if (aOutline.hasAttribute("text"))
>+        feedName = aOutline.getAttribute("text");
>+      else if (aOutline.hasAttribute("title"))
>+        feedName = aOutline.getAttribute("title");
>+      else if (aOutline.hasAttribute("xmlUrl"))
>+        feedName = aOutline.getAttribute("xmlUrl");

Do you really intend to use the text/title attribute even if it's empty? That doesn't sound too useful. How about this:

var feedName = aOutline.getAttribute("text") || aOutline.getAttribute("title") || aOutline.getAttribute("xmlUrl");

>diff --git a/mailnews/extensions/newsblog/js/newsblog.js b/mailnews/extensions/newsblog/js/newsblog.js
>   showPanel: function (server)
>   {
>-    return server.type == "rss";
>+    return false;
>   },

Why this?


r=me with the above addressed/explained.
Attachment #366420 - Flags: review?(mnyromyr) → review+
(In reply to comment #16)
> (From update of attachment 366420 [details] [diff] [review])
> >diff --git a/mailnews/extensions/newsblog/js/newsblog.js b/mailnews/extensions/newsblog/js/newsblog.js
> >   showPanel: function (server)
> >   {
> >-    return server.type == "rss";
> >+    return false;
> >   },
> 
> Why this?

At the moment, the server/feed settings gets shown twice, on the main page and also the "Feed Settings" page. This change just makes it show under the main page.
> At the moment, the server/feed settings gets shown twice, on the main page and
> also the "Feed Settings" page. This change just makes it show under the main
> page.

Ah, okay.
Changes since v0.1e:
* As per reviewer's comments.
Attachment #366420 - Attachment is obsolete: true
Attachment #367517 - Flags: superreview?(neil)
Attachment #367517 - Flags: review+
Comment on attachment 367517 [details] [diff] [review]
Feed tidy patch v0.1f

>   const kVirtualFlag = Components.interfaces.nsMsgFolderFlags.Virtual;
>   var isVirtualFolder = folder ? folder.flags & kVirtualFlag : false;
>+  const kTrashFlag = Components.interfaces.nsMsgFolderFlags.Trash;
>+  var isChildOfTrash = IsSpecialFolder(folder, kTrashFlag, true);
> 
>   var isServer = folder.isServer;
>   var serverType = folder.server.type;
>   var specialFolder = getSpecialFolderString(folder);
>   var canSubscribeToFolder = (serverType == "nntp") ||
>                              (serverType == "imap") ||
>                              (serverType == "rss");
>   var isNewsgroup = !isServer && serverType == 'nntp';
>   var isMailFolder = !isServer && serverType != 'nntp';
>   var canGetMessages =
>     (isServer && (serverType != "nntp") && (serverType != "none")) ||
>     isNewsgroup ||
>-    ((serverType == "rss") && (specialFolder != 'Trash'));
>+    ((serverType == "rss") && !isChildOfTrash && !isVirtualFolder);
[It seems strange to me that they mix flag tests and specialFolder attribute]

>+  const kTrashFlag = Components.interfaces.nsMsgFolderFlags.Trash;
>+  var msgFolder =
>+    folderResource.QueryInterface(Components.interfaces.nsIMsgFolder);
>+  var isChildOfTrash = IsSpecialFolder(msgFolder, kTrashFlag, true);
>   var canGetMessages =
>     (isServer && (serverType != "nntp") && (serverType != "none")) ||
>     isNewsgroup ||
>-    ((serverType == "rss") && (specialFolder != 'Trash'));
>+    ((serverType == "rss") && !isChildOfTrash && !isVirtualFolder);
Don't bother changing specialFolder != 'Trash' to !isChildOfTrash.

>     // if the listener is tracking progress for storing each item, report it here...
>     if (item.feed.downloadCallback && item.feed.downloadCallback.onFeedItemStored)
>-      item.feed.downloadCallback.onFeedItemStored(item.feed, this.itemsToStoreIndex, this.itemsToStore.length);
>+      item.feed.downloadCallback.onFeedItemStored(item.feed, this.itemsToStoreIndex + 1, this.itemsToStore.length);
>  
>     this.itemsToStoreIndex++
Why not just move this above the callback? (You can then give it a ; ;-)

>+      <treechildren id="subscriptionChildren"
>+                    ondblclick="if (event.button == 0) gFeedSubscriptionsWindow.editFeed();"
Might be worth adding default="true" to the edit feed button? Also while you're there the window open call should use centerscreen instead of center.
Attachment #367517 - Flags: superreview?(neil) → superreview+
pushed -> http://hg.mozilla.org/comm-central/rev/358ec16450f3

Only stuff left now is how to clear up orphaned feeds.
Blocks: 483711
Spun off orphan question into bug 483711
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: RSS → Feed Reader
Product: Thunderbird → MailNews Core
Duplicate of this bug: 456264
Duplicate of this bug: 471151
You need to log in before you can comment on or make changes to this bug.