The default bug view has changed. See this FAQ.

Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entitities| to SeaMonkey

RESOLVED FIXED in seamonkey2.15

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Trunk
seamonkey2.15
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.9 wontfix, seamonkey2.10 wontfix, seamonkey2.11 wontfix, seamonkey2.12 wontfix, seamonkey2.15 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

27.95 KB, patch
ewong
: review+
Details | Diff | Splinter Review
820 bytes, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
No longer blocks: 732027
Blocks: 732027
(Reporter)

Updated

5 years ago
status-seamonkey2.10: affected → wontfix
status-seamonkey2.12: --- → affected
tracking-seamonkey2.10: ? → ---
tracking-seamonkey2.11: --- → ?
(Assignee)

Comment 1

5 years ago
Created attachment 641401 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #641401 - Flags: review?(neil)

Comment 2

5 years ago
Comment on attachment 641401 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)

>+  get _copyableAnnotations() [
>+    this.DESCRIPTION_ANNO,
>+    this.LOAD_IN_SIDEBAR_ANNO,
>+    PlacesUtils.POST_DATA_ANNO,
>+    PlacesUtils.READ_ONLY_ANNO,
>+  ],
This creates a new array each time, doesn't it? I'd prefer _copyableAnnotations: [ .. ], instead.

>+   _getURIItemCopyTransaction:
What's the different between a URI item and a bookmark item? Since this ends up creating a bookmark, I'd rather it was called _getBookmarkItemCopyTransaction.

>+     if (aData.dateAdded) {
>+       transactions.push(
>+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
>+       );
>+     }
[Consistency nit: Extra {}s weren't added anywhere else. See below for example.]

[Existing "bug" noticed due to code churn?]
>         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
>+          if (node.livemark && node.annos)
>+            transactions.push(
>+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+            );
>           else
>+            transactions.push(
>+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+            );
>         }
_getFolderCopyTransaction special-cases being called on a livemark, so I don't see the need to special-case it again here.
What's the difference between a folder with two annotations and a livemark?
Attachment #641401 - Flags: feedback?(mak77)
(In reply to neil@parkwaycc.co.uk from comment #2)
> >+  get _copyableAnnotations() [
> >+    this.DESCRIPTION_ANNO,
> >+    this.LOAD_IN_SIDEBAR_ANNO,
> >+    PlacesUtils.POST_DATA_ANNO,
> >+    PlacesUtils.READ_ONLY_ANNO,
> >+  ],
> This creates a new array each time, doesn't it? I'd prefer
> _copyableAnnotations: [ .. ], instead.

the getter contents are unmodifiable, while your suggestion is not, though we may define a const array in the module scope and return it here.

> >+   _getURIItemCopyTransaction:
> What's the different between a URI item and a bookmark item? Since this ends
> up creating a bookmark, I'd rather it was called
> _getBookmarkItemCopyTransaction.

the point is that it may copy "either a bookmark or an history item" TO a bookmark folder (the destination is always a bookmark folder, since others are readonly, the source may be anything), as the javadoc says. URIItem just means "bookmark or history item".

> >+     if (aData.dateAdded) {
> >+       transactions.push(
> >+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
> >+       );
> >+     }
> [Consistency nit: Extra {}s weren't added anywhere else. See below for
> example.]

not sure what you mean, but this is multiline so should be braced (according to the guide also onelines, but we're not strict on that). Ideally we should fix the other places, but... whatever.

> [Existing "bug" noticed due to code churn?]
> >         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
> >+          if (node.livemark && node.annos)
> >+            transactions.push(
> >+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
> >+            );
> >           else
> >+            transactions.push(
> >+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
> >+            );
> >         }
> _getFolderCopyTransaction special-cases being called on a livemark, so I
> don't see the need to special-case it again here.
> What's the difference between a folder with two annotations and a livemark?

good catch, indeed this if/else can be removed.

Updated

5 years ago
Attachment #641401 - Flags: feedback?(mak77)

Comment 4

5 years ago
(In reply to Marco Bonardo from comment #3)
> (In reply to comment #2)
> > >+  get _copyableAnnotations() [
> > >+    this.DESCRIPTION_ANNO,
> > >+    this.LOAD_IN_SIDEBAR_ANNO,
> > >+    PlacesUtils.POST_DATA_ANNO,
> > >+    PlacesUtils.READ_ONLY_ANNO,
> > >+  ],
> > This creates a new array each time, doesn't it? I'd prefer
> > _copyableAnnotations: [ .. ], instead.
> the getter contents are unmodifiable, while your suggestion is not, though
> we may define a const array in the module scope and return it here.
I thought the _ meant that the array was private to the module anyway...

> the point is that it may copy "either a bookmark or an history item" TO a
> bookmark folder (the destination is always a bookmark folder, since others
> are readonly, the source may be anything), as the javadoc says. URIItem just
> means "bookmark or history item".
Thanks for explaining it so well.

Comment 5

5 years ago
(In reply to comment #2)
> (From update of attachment 641401 [details] [diff] [review])
> >+  get _copyableAnnotations() [
> >+    this.DESCRIPTION_ANNO,
> >+    this.LOAD_IN_SIDEBAR_ANNO,
> >+    PlacesUtils.POST_DATA_ANNO,
> >+    PlacesUtils.READ_ONLY_ANNO,
> >+  ],
> This creates a new array each time, doesn't it?
It refers to this, which you can't easily do otherwise. Sigh...

Comment 6

5 years ago
Comment on attachment 641401 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v1)

OK, so as previously discussed:
>+     if (aData.dateAdded) {
>+       transactions.push(
>+         new PlacesEditItemDateAddedTransaction(null, aData.dateAdded)
>+       );
>+     }
Remove the {}s

>+     if (aData.lastModified) {
>+       transactions.push(
>+         new PlacesEditItemLastModifiedTransaction(null, aData.lastModified)
>+       );
>+     }
Remove the {}s

>         if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
>+          if (node.livemark && node.annos)
>+            transactions.push(
>+              PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+            );
>           else
index);
>+            transactions.push(
>+              PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+            );
>         }
>         else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
  transactions.push(
    PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
  );
else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)

Bonus points for making _copyableAnnotations a lazy getter.
Attachment #641401 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #4)
> (In reply to Marco Bonardo from comment #3)
> > (In reply to comment #2)
> > > >+  get _copyableAnnotations() [
> > > >+    this.DESCRIPTION_ANNO,
> > > >+    this.LOAD_IN_SIDEBAR_ANNO,
> > > >+    PlacesUtils.POST_DATA_ANNO,
> > > >+    PlacesUtils.READ_ONLY_ANNO,
> > > >+  ],
> > > This creates a new array each time, doesn't it? I'd prefer
> > > _copyableAnnotations: [ .. ], instead.
> > the getter contents are unmodifiable, while your suggestion is not, though
> > we may define a const array in the module scope and return it here.
> I thought the _ meant that the array was private to the module anyway...

yes, though that's a suggestion that unfortunately doesn't enforce anything on add-ons... not that I think we are active clever in other parts...
(Assignee)

Comment 8

5 years ago
Created attachment 648951 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v2)
Attachment #641401 - Attachment is obsolete: true
Attachment #648951 - Flags: review?(neil)

Comment 9

5 years ago
Comment on attachment 648951 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v2)

>+  XPCOMUtils.defineLazyGetter(PlacesUIUtils, "_copyableAnnotations", function() {
>+    return [this.DESCRIPTION_ANNO,
>+            this.LOAD_IN_SIDEBAR_ANNO,
>+            PlacesUtils.POST_DATA_ANNO,
>+            PlacesUtils.READ_ONLY_ANNO]
>+  }); 
[Nit: trailing whitespace on that last line]
This needs to go near the end of the file with the other lazy getters.

>+        if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
>+          transactions.push(
>+            PlacesUIUtils._getLivemarkCopyTransaction(node, aContainer, index)
>+          );
Unfortunately you deleted the wrong line, you were supposed to delete this line and keep the _getFolderCopyTransaction line.
Attachment #648951 - Flags: review?(neil) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 649160 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)
Attachment #648951 - Attachment is obsolete: true
Attachment #649160 - Flags: review?(neil)
Comment on attachment 649160 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)

>   _getFolderCopyTransaction:
>   function PUIU__getFolderCopyTransaction(aData, aContainer, aIndex) {
Something here must be wrong because I can't seem to copy a folder but I don't see any exceptions in the Error Console...
Comment on attachment 649160 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v3)

>+        if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER)
>+          transactions.push(
>+            PlacesUIUtils._getFolderCopyTransaction(node, aContainer, index)
>+          );
>+        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
>+          transaction.push(new PlacesCreateSeparatorTransaction(-1, index));
>+        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
>+          transaction.push(
>+            PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index)
>+          );
These all need to say transactions.push but only the first one is right.
Also, _getBookmarkItemCopyTransaction needs to get renamed back to _getURIItemCopyTransaction (sorry to confuse you there).

+      if (aContainer == PlacesUtils.tagsFolderId) { // Copying a tag folder.
+        let transactions = [];
+        if (aData.children) {
+          aData.children.forEach(function(aChild) {
+            transactions.push(
+              new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
+                                          [aData.title])          
+            );
+          });
         }
+        return this.ptm.aggregateTransactions("addTags", transactions);
+      }
 
+      if (aData.livemark && aData.annos)  //Copying a livemark.
+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
     }
 
-    // tag folders use tag transactions
-    if (aContainer == PlacesUtils.tagsFolderId) {
-      var txns = [];
-      if (aData.children) {
-        aData.children.forEach(function(aChild) {
-          txns.push(
-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
-                                        [aData.title])          
-          );
-        }, this);
-      }
-      return this.ptm.aggregateTransactions("addTags", txns);
There's something seriously wrong here, this shouldn't have been reindented. Also the last line doesn't seem to have been updated.
Attachment #649160 - Flags: review?(neil) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 651250 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v4)
Attachment #649160 - Attachment is obsolete: true
Attachment #651250 - Flags: review?(neil)
Comment on attachment 651250 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v4)

Did you miss this because my quoting went wrong last time?
>+      if (aContainer == PlacesUtils.tagsFolderId) { // Copying a tag folder.
>+        let transactions = [];
>+        if (aData.children) {
>+          aData.children.forEach(function(aChild) {
>+            transactions.push(
>+              new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
>+                                          [aData.title])          
>+            );
>+          });
>         }
>-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
>-          txn = new PlacesCreateSeparatorTransaction(-1, index);
>-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
>-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
>+        return this.ptm.aggregateTransactions("addTags", transactions);
>+      }
> 
>-        if (txn)
>-          childItemsTransactions.push(txn);
>-        else
>-          throw("Unexpected item under a bookmarks folder");
>-      }
>-      return childItemsTransactions;
>+      if (aData.livemark && aData.annos)  //Copying a livemark.
>+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
>     }
> 
>-    // tag folders use tag transactions
>-    if (aContainer == PlacesUtils.tagsFolderId) {
>-      var txns = [];
>-      if (aData.children) {
>-        aData.children.forEach(function(aChild) {
>-          txns.push(
>-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
>-                                        [aData.title])          
>-          );
>-        }, this);
>-      }
>-      return this.ptm.aggregateTransactions("addTags", txns);
Somehow the tag folders and livemark copying code got indented and added into the previous block of code, which is incorrect. Also, the last line that I quoted above neeeds to be updated as per bug 629620.
Attachment #651250 - Flags: review?(neil) → review-
(Assignee)

Comment 15

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #14)
> Comment on attachment 651250 [details] [diff] [review]
> Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since
> they are new entities| to SeaMonkey (v4)
> 
> Did you miss this because my quoting went wrong last time?

Mainly because the {} placement is confusing me.  I'm  having some
difficulties in checking which code belongs to which block.
(Assignee)

Comment 16

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #14)

> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
> >-          txn = new PlacesCreateSeparatorTransaction(-1, index);
> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
> >-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
> >+        return this.ptm.aggregateTransactions("addTags", transactions);
> >+      }
> > 
> >-        if (txn)
> >-          childItemsTransactions.push(txn);
> >-        else
> >-          throw("Unexpected item under a bookmarks folder");
> >-      }
> >-      return childItemsTransactions;
> >+      if (aData.livemark && aData.annos)  //Copying a livemark.
> >+        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
> >     }
> > 
> >-    // tag folders use tag transactions
> >-    if (aContainer == PlacesUtils.tagsFolderId) {
> >-      var txns = [];
> >-      if (aData.children) {
> >-        aData.children.forEach(function(aChild) {
> >-          txns.push(
> >-            new PlacesTagURITransaction(PlacesUtils._uri(aChild.uri),
> >-                                        [aData.title])          
> >-          );
> >-        }, this);
> >-      }
> >-      return this.ptm.aggregateTransactions("addTags", txns);
> Somehow the tag folders and livemark copying code got indented and added
> into the previous block of code, which is incorrect. Also, the last line
> that I quoted above neeeds to be updated as per bug 629620.

Do you mean :

  return this.ptm.aggregateTransactions("addTags", transactions); ?

If so, isn't it in the above code? i.e:

> >-        else if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE)
> >-          txn = PlacesUIUtils._getBookmarkItemCopyTransaction(node, -1, index);
> >+        return this.ptm.aggregateTransactions("addTags", transactions);
> >+      }

or am I misunderstanding?
(Assignee)

Comment 17

5 years ago
Created attachment 655262 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)
Attachment #651250 - Attachment is obsolete: true
Attachment #655262 - Flags: review?(neil)
Comment on attachment 655262 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)

This is looking much better although I think I may have spotted a typo.
Comment on attachment 655262 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v5)

>+      return new PlacesAggregatedTransactions("addTags", transactions);
Typo: PlacesAggregatedTransaction
r=me with that fixed.

>     }
>+    if (aData.livemark && aData.annos)  //Copying a livemark.
Nit: could put a blank line before this if.
Attachment #655262 - Flags: review?(neil) → review+
(Assignee)

Comment 20

5 years ago
Created attachment 656844 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entities| to SeaMonkey (v6)
Attachment #655262 - Attachment is obsolete: true
Attachment #656844 - Flags: review+
Comment on attachment 656844 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entities| to SeaMonkey (v6)

Whoops, only just noticed this:

>+    if (aData.livemark && aData.annos)  //Copying a livemark.
>       return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
>     }
Mismatched } - I assume you meant to write
if (aData.livemark && aData.annos) { // Copying a livemark.
Or you could delete the } but you would still want to fix up the spaces.
(Assignee)

Comment 22

5 years ago
Created attachment 661450 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v7)
Attachment #656844 - Attachment is obsolete: true
Attachment #661450 - Flags: review+

Updated

5 years ago
Depends on: 794279

Updated

5 years ago
Blocks: 794279
No longer depends on: 794279
(Assignee)

Comment 23

5 years ago
Created attachment 667364 [details] [diff] [review]
Port |Bug 629620 - Copied bookmarks shouldn't inherit all anotations, since they are new entities| to SeaMonkey (v8)

Added space after //.
Attachment #661450 - Attachment is obsolete: true
Attachment #667364 - Flags: review+
(Assignee)

Comment 24

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/8a26f31333b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I think this bug fix/test has to do with the recent test problem on SeaMonkey build bot. See http://tbpl.drapostles.org/?tree=SeaMonkey Win debug XPCShelltest debug:
"TEST-INFO | e:\builds\slave\test\build\xpcshell\tests\suite\common\places\tests\unit\test_PUIU_makeTransaction.js | running test ...

command timed out: 1200 seconds without output, attempting to kill
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1"
ewong: Do you have time to look at this test problem? If not, we should back out your patch and/or fix the test. This patch isn't critical for the next release as far as I see this?
ewong: To help you a bit (not sure if you own Windows), the test gets stuck here:

 waitForBookmarkNotification("onItemAdded", function(aData) (line 122)

The tests before this line in this test run fine.
Created attachment 668954 [details] [diff] [review]
Fix broken test

ewong: You forgot to increment the loop counter (copy&paste error?) :-)
Attachment #668954 - Flags: review?(neil)

Updated

5 years ago
Attachment #668954 - Attachment description: Patch → Fix broken test

Updated

5 years ago
Attachment #668954 - Flags: review?(neil) → review+
Comment on attachment 668954 [details] [diff] [review]
Fix broken test

Checked in: https://hg.mozilla.org/comm-central/rev/0d68cf5ac488
Settings flags to make tracking of fixed bugs easier.
status-seamonkey2.15: --- → fixed
Target Milestone: --- → seamonkey2.15
status-seamonkey2.12: affected → wontfix

Updated

4 years ago
status-seamonkey2.11: affected → wontfix
tracking-seamonkey2.11: ? → ---
You need to log in before you can comment on or make changes to this bug.