Closed Bug 1096837 Opened 6 years ago Closed 6 years ago

Allow PlacesUtils.bookmarks.update({ parentGuid, index: DEFAULT_INDEX })

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
37.1

People

(Reporter: mano, Assigned: mano)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Move-to-the-end is a common use case that's currently unsupported by PlacesUtils.bookmarks.update (unless you pass something like MAX_INT for index).

This is necessary for reimplementing PlacesTransactions.Move using the update API.
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Attached patch patchSplinter Review
I also fixed a bug in update: moving an item to a "too high" index within the same folder was treated as appending.

Note that while the function "internals" support update( index: defualt ) without specifying parentGuid, the input validation disallows that. We may want to revise this decision later.
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #8527592 - Flags: review?(mak77)
Comment on attachment 8527592 [details] [diff] [review]
patch

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

::: toolkit/components/places/Bookmarks.jsm
@@ +306,5 @@
> +        if (!parent || parent.guid == item.parentGuid) {  // Same parent.
> +          // If at this point we don't have a parent yet, we are moving into
> +          // the same container.  Thus we know it exists.
> +          if (!parent)
> +            parent = yield fetchBookmark({ guid: item.parentGuid });

what about 

// current comment
if (!parent)
  parent =
if (updateInfo.index >= parent._childCount ||
    updateInfo.index == this.DEFAULT_INDEX) {
   updateInfo.index = parent._childCount;
}
// Fix the index when moving into the same container.
if (parent.guid == item.parentGuid)
   updateInfo.index--;

::: toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
@@ +422,5 @@
> +  let sep_3 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid,
> +                                                   type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
> +  checkBookmarkObject(sep_3);
> +
> +  function ensurePlace(info, parentGuid, index) {

s/ensurePlace/ensurePosition/ ...let's not overload the "place" word again :)

@@ +454,5 @@
> +  sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid);
> +  ensurePlace(sep_3, folder_b.guid, 0);
> +
> +  // folder_a: [sep_1], folder_b: [sep_3, sep_2]
> +  sep_2.index = PlacesUtils.bookmarks.DEFAULT_INDEX;

maybe for the third test you could use a very large index?
Attachment #8527592 - Flags: review?(mak77) → review+
Iteration: 36.3 → 37.1
https://hg.mozilla.org/mozilla-central/rev/a65baedb8ede
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.