1058566 - Transaction for duplicating any places item

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

Attachment: patch

Comment 1

[Asaf Romano (gone)]

oops, wrong file.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Asaf Romano (gone)]

Note to self: before landing this, I need to fix the MoveItem caller in browser/.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Let's add this to the current iteration. Mano, can you please estimate this? Marco, can you please add it to the backlog?

Comment 5

[Marco Mucci [:MarcoM]]

Thanks Tim.  I've added Bug 1058566 to the iteration and marked it as blocking Bug 984903 on the iteration priority sheet.

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Let's add this to the current iteration. Mano, can you please estimate this?
> Marco, can you please add it to the backlog?

Comment 6

[Asaf Romano (gone)]

This cannot be verified on its own, so qa-. 8 points I think, or 5.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

* Fix some issues with livemarks and improve the tests covering them.
* Fix the MoveItem caller in browser/

Comment 8

[Asaf Romano (gone)]

So there's this issue with annotations:

My patch doesn't re-introduce the functionality change from bug 629620: copy only selected annotation (in an opt-in manner). That is, the Duplicate transaction copies over /all/ annotations.

The underlying issue for that _bug_ is the use of "inverted" key-value concepts... Rather than storing some bookmarks as the value of a SmartBookmark preference of some sort, a SmartBookmark annotation is set on some bookmarks, and then things get messy if these annotations are copied over.

Not willing to refactor more than we have to since that's already much more than enough, I would happily copy over a blacklist solution. The thing is, Marco deliberately decided to go with a whitelist solution (see bug 629620 comment 2), for reasons I'm not sure I agree with. So I'd like to discuss those with him once he's back. Implementing either solutions (whiltelist or blacklist, that is) is straightforward, so I'm not worried (my test would need some changes in the case of a whitelist).

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't think we currently have very compelling arguments for whitelist vs blacklist, some annotations might be per page while others might be for a very specific bookmark. I still don't get the current situation where we have a bookmarks tree with dusplicated rather than a tag cloud out of a single bookmarks queue, but I guess I'm going off topic.

So, I'm even fine with a blacklist, no worries.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8479616 [details] [diff] [review]
patch

Review of attachment 8479616 [details] [diff] [review]:
-----------------------------------------------------------------

the test is missing some yields, and needs the blacklist, nothing major though.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +771,5 @@
> + *       root one.
> + * @return {Promise}
> + */
> +function* createItemsFromBookmarksTree(aBookmarksTree, aRestoring = false) {
> +  function filterLivemarkDetails(aItem) {

s/filter/extract/

@@ +789,5 @@
> +      } );
> +    }
> +    return [feedURI, siteURI];
> +  }
> +  function* createItem(aItem,

newline above this please

@@ +839,5 @@
> +        break;
> +      }
> +    }
> +    if ("annos" in aItem)
> +      PlacesUtils.setAnnotationsForItem(itemId, aItem.annos);

please remember to filter these with the blacklist.

@@ +851,5 @@
> +    return itemId;
> +  }
> +  return yield createItem(aBookmarksTree,
> +                    aBookmarksTree.parentGUID,
> +                    aBookmarksTree.index);

indentation

@@ +1200,5 @@
>      const bms = PlacesUtils.bookmarks;
>  
> +    let itemInfo = null;
> +    try {
> +      itemInfo = yield PlacesUtils.promiseBookmarksTree(aGUID);

<3

@@ +1270,5 @@
> + * Required Input Properties: guid, newParentGUID
> + * Optional Input Properties: newIndex.
> + */
> +PT.Duplicate = DefineTransaction(["GUID", "newParentGUID"],
> +                                 ["newIndex"]);

This bug is lacking a global overview about why we need a Duplicate transaction... could you please provide some insight?

Why is this Duplicate rather than Copy?

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +273,5 @@
> +  // restored by Redo.
> +  if (aIsRestoredItem) {
> +    Assert.deepEqual(aOriginal, aNew);
> +    if (isLivemarkTree(aNew))
> +      yield ensureLivemarkCreatedByAddLivemark(aNew.guid);

since it's yielding should ensureEqualBookmarksTrees be a star function?

@@ +281,5 @@
> +  for (let property of Object.keys(aOriginal)) {
> +    if (property == "children") {
> +      Assert.equal(aOriginal.children.length, aNew.children.length);
> +      for (let i = 0; i < aOriginal.children.length; i++) {
> +        ensureEqualBookmarksTrees(aOriginal.children[i],

and here should yield

@@ +311,5 @@
> +
> +function* ensureBookmarksTreeRestoredCorrectly(aOriginalBookmarksTree) {
> +  let restoredTree =
> +    yield PlacesUtils.promiseBookmarksTree(aOriginalBookmarksTree.GUID);
> +  ensureEqualBookmarksTrees(aOriginalBookmarksTree, restoredTree);

and should yield here as well

@@ +1291,5 @@
> +      PT.Duplicate({ GUID: aOriginalGUID, newParentGUID: rootGUID }));
> +
> +    let originalInfo = yield PlacesUtils.promiseBookmarksTree(aOriginalGUID);
> +    let duplicateInfo = yield PlacesUtils.promiseBookmarksTree(duplicateGUID);
> +    ensureEqualBookmarksTrees(originalInfo, duplicateInfo, false);

should yield

@@ +1297,5 @@
> +    function* redo() {
> +      yield PT.redo();
> +      yield PT.redo();
> +      ensureBookmarksTreeRestoredCorrectly(originalInfo);
> +      ensureBookmarksTreeRestoredCorrectly(duplicateInfo);

shouldn't these yield?

@@ +1303,5 @@
> +    function* undo() {
> +      yield PT.undo();
> +      // also undo the original item addition.
> +      yield PT.undo();
> +      ensureNonExistent(aOriginalGUID, duplicateGUID);

shouldn't this yield?

@@ +1333,5 @@
> +    yield duplicate_and_test(guid);
> +  }
> +
> +  // Test duplicating a folder having some contents.
> +  let richFolderGUID = yield PT.transact(function *() {

sure, we were indeed just missing the "rich folder" concept :D :D

Comment 11

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #10)
> Comment on attachment 8479616 [details] [diff] [review]
> patch
> 
> Review of attachment 8479616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the test is missing some yields, and needs the blacklist, nothing major
> though.

I may do the white/black list part in follow up (just may, if it seems trivial enough I'll just land it as part of this patch).

> @@ +1270,5 @@
> > + * Required Input Properties: guid, newParentGUID
> > + * Optional Input Properties: newIndex.
> > + */
> > +PT.Duplicate = DefineTransaction(["GUID", "newParentGUID"],
> > +                                 ["newIndex"]);
> 
> This bug is lacking a global overview about why we need a Duplicate
> transaction... could you please provide some insight?
> 

It's going to be used both on-drop and on /copy/-paste. API-wise it should have the same semantics as the Move transaction, except the original item is kept in place.

> Why is this Duplicate rather than Copy?

I tend to prefer the Duplicate terminology because otherwise, you copy, and then when you paste, you "copy" again :). I should probably admit that I'm just influenced by OS X terminology... Anyway, I've no strong feelings about this, and I guess Copy is better considering the white/black list unfortunate mess.
 
> @@ +1333,5 @@
> > +    yield duplicate_and_test(guid);
> > +  }
> > +
> > +  // Test duplicating a folder having some contents.
> > +  let richFolderGUID = yield PT.transact(function *() {
> 
> sure, we were indeed just missing the "rich folder" concept :D :D

I guess you prefer nicelyFilledFolder :)

Comment 12

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/ae6de471bcd7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Looks like this was the patch responsible for bug 1067978. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/176edad2279e

Comment 14

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=176edad2279e

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mano, I suspect the usual bug with timers on Windows, those have a 16ms precision so you can't really check lastModified. that's why we have is_time_ordered helper http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#598

Comment 16

[Asaf Romano (gone)]

Take 2:

https://hg.mozilla.org/integration/fx-team/rev/fbf464a2971c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbf464a2971c