709247 - Fix to sync feeds database on folderpane moves

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

Attachment: fix.

review is requested from iann and neil since you were both involved in Bug 471932.  please advise who, if not you, would be the right reviewer(s) to get this and Bug 705504 moving.

Comment 1

[Ian Neal]

Comment on attachment 580499 [details] [diff] [review]
fix.

>diff --git a/mailnews/extensions/newsblog/content/utils.js b/mailnews/extensions/newsblog/content/utils.js
>--- a/mailnews/extensions/newsblog/content/utils.js
>+++ b/mailnews/extensions/newsblog/content/utils.js
>@@ -211,11 +211,28 @@
>   }
>   if (msgDb && msgDb.dBFolderInfo) {
>     feedurls = msgDb.dBFolderInfo.getCharProperty("feedUrl");
>-    // Clean up the feedUrl string.
>+    // Clean up the feedUrl string.  Also sync the feeds database with the
>+    // feed url's real current folder.
>     feedurls.split(kFeedUrlDelimiter).forEach(
>       function(url) {
>-        if (url && feedUrlArray.indexOf(url) == -1)
>+        if (url && feedUrlArray.indexOf(url) == -1) {
>           feedUrlArray.push(url);
>+
>+          try {
Which part of the code is going to throw?

>+            var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                                .getService(Components.interfaces.nsIRDFService);
Isn't rdf already global in this file? (line 49 and 50)

>+            var ds = getSubscriptionsDS(aFolder.server);
Do we really need to keep getting ds, can it not be declared outside of the function and then used?

>+            var id = rdf.GetResource(url);
>+            var resource = rdf.GetResource(aFolder.URI);
>+            // Get the node for the current folder URI.
>+            var node = ds.GetTarget(id, FZ_DESTFOLDER, true);
>+            if (node)
>+              ds.Change(id, FZ_DESTFOLDER, node, resource);
>+            else
>+              addFeed(url, resource.name, resource);
We seem to be duplicating code between here and newsblog.js (see updateSubscriptionsDS - http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/js/newsblog.js#236), is there not a way we could share it?
r- for the moment

Comment 2

[alta88]

(In reply to Ian Neal from comment #1)
> Comment on attachment 580499 [details] [diff] [review]
> fix.
> 
> >diff --git a/mailnews/extensions/newsblog/content/utils.js b/mailnews/extensions/newsblog/content/utils.js
> >--- a/mailnews/extensions/newsblog/content/utils.js
> >+++ b/mailnews/extensions/newsblog/content/utils.js
> >@@ -211,11 +211,28 @@
> >   }
> >   if (msgDb && msgDb.dBFolderInfo) {
> >     feedurls = msgDb.dBFolderInfo.getCharProperty("feedUrl");
> >-    // Clean up the feedUrl string.
> >+    // Clean up the feedUrl string.  Also sync the feeds database with the
> >+    // feed url's real current folder.
> >     feedurls.split(kFeedUrlDelimiter).forEach(
> >       function(url) {
> >-        if (url && feedUrlArray.indexOf(url) == -1)
> >+        if (url && feedUrlArray.indexOf(url) == -1) {
> >           feedUrlArray.push(url);
> >+
> >+          try {
> Which part of the code is going to throw?
>

the intent is to guarantee it doesn't.  

> >+            var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
> >+                                .getService(Components.interfaces.nsIRDFService);
> Isn't rdf already global in this file? (line 49 and 50)
> 

true.

> >+            var ds = getSubscriptionsDS(aFolder.server);
> Do we really need to keep getting ds, can it not be declared outside of the
> function and then used?
> 

this is done well over a dozen times already in the codebase.  imo a real fix with a proper utils object with getters is out of scope for this bug.

> >+            var id = rdf.GetResource(url);
> >+            var resource = rdf.GetResource(aFolder.URI);
> >+            // Get the node for the current folder URI.
> >+            var node = ds.GetTarget(id, FZ_DESTFOLDER, true);
> >+            if (node)
> >+              ds.Change(id, FZ_DESTFOLDER, node, resource);
> >+            else
> >+              addFeed(url, resource.name, resource);
> We seem to be duplicating code between here and newsblog.js (see
> updateSubscriptionsDS -
> http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/js/
> newsblog.js#236), is there not a way we could share it?
> r- for the moment

yes, that's where i got it.  this code block is meant to run only once per folder and not again, and could be removed after a few versions.  imo, a handful of lines isn't worth it.

anyway, this section isn't important to fix the real problem.

Comment 3

[alta88]

Attachment: fix2.

Comment 4

[[e-mail]]

Comment on attachment 580499 [details] [diff] [review]
fix.

> NS_IMETHODIMP nsRssIncomingServer::FolderMoveCopyCompleted(bool aMove, nsIMsgFolder *aSrcFolder, nsIMsgFolder *aDestFolder)
> {
>-  return FolderChanged(aSrcFolder, PR_FALSE);
>+  return FolderChanged(aDestFolder, PR_FALSE);
So, the point of this is to run through all the feeds in the destination, updating the data source with their new folder, in much the same way as renaming a folder does. I wonder what the right thing to do in the case of a copy (aMove is false) is though.

I see your other patch only has this change and not the JS change, does that exist only to clean up after existing moved folders?

Note that you can probably invoke UpdateSubscriptionsDS from utils.js since it seems to exist on the nsNewsBlogFeedDownloader global variable.

Comment 5

[alta88]

(In reply to [e-mail] from comment #4)
> Comment on attachment 580499 [details] [diff] [review]
> fix.
> 
> > NS_IMETHODIMP nsRssIncomingServer::FolderMoveCopyCompleted(bool aMove, nsIMsgFolder *aSrcFolder, nsIMsgFolder *aDestFolder)
> > {
> >-  return FolderChanged(aSrcFolder, PR_FALSE);
> >+  return FolderChanged(aDestFolder, PR_FALSE);
> So, the point of this is to run through all the feeds in the destination,
> updating the data source with their new folder, in much the same way as
> renaming a folder does. I wonder what the right thing to do in the case of a
> copy (aMove is false) is though.
> 

yes.  in fact, a move followed by a rename would have been ok.  i thought about copy, but don't think it matters, as you can't copy anywhere in the same feed account, so the account's db isn't changed.  if you copy to another feed account, it will be like a move for that account's db.  a folder copy to a non rss account won't run.

> I see your other patch only has this change and not the JS change, does that
> exist only to clean up after existing moved folders?
> 

yes, for those poor unfortunates who finally got themselves organized, with an out of sync db, but nonetheless working due to the feedUrl business, which is very fragile (and contrary to a db design that might consider the concept of transactional integrity).

it can be added here or else i also have it in another patch that fixes the UI freeze on biffs.  as mentioned it will only run once per folder where the feedUrl hasn't yet been set by setStringProperty.

> Note that you can probably invoke UpdateSubscriptionsDS from utils.js since
> it seems to exist on the nsNewsBlogFeedDownloader global variable.

unfortunately that would cause recursion, and only part of it is needed.

Comment 8

[[e-mail]]

(In reply to alta88 from comment #5)
> (In reply to [e-mail] from comment #4)
> > Note that you can probably invoke UpdateSubscriptionsDS from utils.js since
> > it seems to exist on the nsNewsBlogFeedDownloader global variable.
> unfortunately that would cause recursion, and only part of it is needed.
How do you mean? updateSubsciptionsDS looks just like the code you're adding.
(Sorry for the typo and wrong source file name.)

Comment 9

[alta88]

per Bug 705504 checkin, updateSubsciptionsDS calls getFeedUrlsInFolder..

i propose this bug be solely about the 1 line fix patch.

Comment 10

[David :Bienvenu]

Comment on attachment 580820 [details] [diff] [review]
fix2.

can you use false instead of PR_FALSE?

Comment 11

[alta88]

Attachment: address comments.

this bug should be in the same branch as bug 705504, so requesting aurora.

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/a3b6a095274e

Comment 13

[alta88]

i guess this needs a checkin for aurora post approval..?

Comment 14

[Mark Banner (:standard8)]

Yes, but please keep the milestone as the trunk version it was fixed in - a status flag will indicate fixed on branches.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/e39e86f75d02