Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix to sync feeds database on folderpane moves

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 12.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird11 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

549 bytes, patch
alta88
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 580499 [details] [diff] [review]
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.
Attachment #580499 - Flags: superreview?(neil)
Attachment #580499 - Flags: review?(iann_bugzilla)

Comment 1

6 years ago
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-
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
Created attachment 580820 [details] [diff] [review]
fix2.
Attachment #580820 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

6 years ago
Attachment #580820 - Flags: review?(iann_bugzilla) → review?(dbienvenu)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 711173
(Assignee)

Updated

6 years ago
Duplicate of this bug: 311697
(Assignee)

Updated

6 years ago
Duplicate of this bug: 257700

Comment 8

6 years ago
(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.)
(Assignee)

Comment 9

6 years ago
per Bug 705504 checkin, updateSubsciptionsDS calls getFeedUrlsInFolder..

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

Comment 10

6 years ago
Comment on attachment 580820 [details] [diff] [review]
fix2.

can you use false instead of PR_FALSE?
Attachment #580820 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 584744 [details] [diff] [review]
address comments.


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?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/a3b6a095274e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Attachment #584744 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 13

6 years ago
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
Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/e39e86f75d02
status-thunderbird11: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.