Closed Bug 709247 Opened 8 years ago Closed 8 years ago

Fix to sync feeds database on folderpane moves

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set

Tracking

(thunderbird11 fixed)

RESOLVED FIXED
Thunderbird 12.0
Tracking Status
thunderbird11 --- fixed

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(1 file, 2 obsolete files)

549 bytes, patch
alta88
: review+
Details | Diff | Splinter Review
Attached patch fix. (obsolete) — Splinter Review
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.
Attachment #580499 - Flags: superreview?(neil)
Attachment #580499 - Flags: review?(iann_bugzilla)
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
Attachment #580499 - Flags: review?(iann_bugzilla) → review-
(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.
Attached patch fix2. (obsolete) — Splinter Review
Attachment #580820 - Flags: review?(iann_bugzilla)
Attachment #580820 - Flags: review?(iann_bugzilla) → review?(dbienvenu)
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.
(In reply to neil@parkwaycc.co.uk 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.
Blocks: 711173
Duplicate of this bug: 311697
Duplicate of this bug: 257700
(In reply to alta88 from comment #5)
> (In reply to neil@parkwaycc.co.uk 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.)
per Bug 705504 checkin, updateSubsciptionsDS calls getFeedUrlsInFolder..

i propose this bug be solely about the 1 line fix patch.
Comment on attachment 580820 [details] [diff] [review]
fix2.

can you use false instead of PR_FALSE?
Attachment #580820 - Flags: review?(dbienvenu) → review+
this bug should be in the same branch as bug 705504, so requesting aurora.
Attachment #580499 - Attachment is obsolete: true
Attachment #580820 - Attachment is obsolete: true
Attachment #580499 - Flags: superreview?(neil)
Attachment #584744 - Flags: review+
Attachment #584744 - Flags: approval-comm-aurora?
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/a3b6a095274e
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Attachment #584744 - Flags: approval-comm-aurora? → approval-comm-aurora+
i guess this needs a checkin for aurora post approval..?
Keywords: checkin-needed
Target Milestone: Thunderbird 12.0 → Thunderbird 11.0
Yes, but please keep the milestone as the trunk version it was fixed in - a status flag will indicate fixed on branches.
Target Milestone: Thunderbird 11.0 → Thunderbird 12.0
You need to log in before you can comment on or make changes to this bug.