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

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Ian Neal]

Patch coming up to fix various issues with mailnews feed code.

Comment 1

[Ian Neal]

Attachment: Feed tidy patch v0.1

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.

Comment 2

[Phil Ringnalda (:philor)]

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

Comment 3

[Ian Neal]

Attachment: Hopefully correct patch for TB part v0.1

This should be a better version of the TB part of the patch.

Comment 4

[[e-mail]]

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.

Comment 5

[Ian Neal]

Attachment: Feed tidy using IsSpecialFolder patch v0.1a

Changes since v0.1:
* Use IsSpecialFolder for checking heritage of folder.

Carrying forward r= from Neil

Comment 6

[Ian Neal]

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)

Comment 7

[Wayne Mery (:wsmwk)]

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 8

[Karsten Düsterloh]

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.

Comment 9

[Ian Neal]

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

Comment 10

[Ian Neal]

Attachment: Feed tidy patch v0.1b

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.

Comment 11

[Karsten Düsterloh]

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

Comment 12

[Karsten Düsterloh]

Attachment: screeny of folder pane and subscribe dialog with vertual folders

Comment 13

[Ian Neal]

Attachment: Feed tidy, AM tweak and test update patch v0.1c

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.

Comment 14

[Karsten Düsterloh]

My apologies, this has bitrotted again. :(

Comment 15

[Ian Neal]

Attachment: Feed tidy patch v0.1e

Changes since v0.1c:
* unbitrotted - not sure what happened to 0.1d though.

Comment 16

[Karsten Düsterloh]

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.

Comment 17

[Ian Neal]

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

Comment 18

[Karsten Düsterloh]

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

Comment 19

[Ian Neal]

Attachment: Feed tidy patch v0.1f

Changes since v0.1e:
* As per reviewer's comments.

Comment 20

[[e-mail]]

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.

Comment 21

[Ian Neal]

pushed -> http://hg.mozilla.org/comm-central/rev/358ec16450f3

Only stuff left now is how to clear up orphaned feeds.

Comment 22

[Ian Neal]

Spun off orphan question into bug 483711