Transaction for duplicating any places item

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

unspecified
mozilla35
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8478989 - Flags: review?(mak77)
Posted patch patch (obsolete) — Splinter Review
Attachment #8478989 - Attachment is obsolete: true
Attachment #8478989 - Flags: review?(mak77)
Attachment #8478991 - Flags: review?(mak77)
Note to self: before landing this, I need to fix the MoveItem caller in browser/.
Let's add this to the current iteration. Mano, can you please estimate this? Marco, can you please add it to the backlog?
Iteration: --- → 34.3
Flags: needinfo?(mmucci)
Flags: needinfo?(mano)
Flags: firefox-backlog+
Status: NEW → ASSIGNED
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?
Flags: needinfo?(mmucci) → qe-verify?
This cannot be verified on its own, so qa-. 8 points I think, or 5.
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(mano)
Points: --- → 8
Posted patch patchSplinter Review
* Fix some issues with livemarks and improve the tests covering them.
* Fix the MoveItem caller in browser/
Attachment #8478991 - Attachment is obsolete: true
Attachment #8478991 - Flags: review?(mak77)
Attachment #8479616 - Flags: review?(mak77)
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).
Iteration: 34.3 → 35.1
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 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
Attachment #8479616 - Flags: review?(mak77) → review+
(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 :)
Iteration: 35.1 → 35.2
Depends on: 1067978
Looks like this was the patch responsible for bug 1067978. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/176edad2279e
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
https://hg.mozilla.org/mozilla-central/rev/fbf464a2971c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.