From df2ebf6d72f322370f4629b9307b06fb804e612d Mon Sep 17 00:00:00 2001 From: Asaf Romano Date: Tue, 25 Nov 2014 13:33:28 +0200 Subject: [PATCH] Bug 1096837 - Allow PlacesUtils.bookmarks.update({ parentGuid, index: DEFAULT_INDEX }). r=mak --- toolkit/components/places/Bookmarks.jsm | 20 ++++-- .../tests/bookmarks/test_bookmarks_update.js | 65 +++++++++++++++++++ 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 7d3f6bda2f28..9db1dd4f5fd3 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -94,7 +94,7 @@ let Bookmarks = Object.freeze({ TYPE_SEPARATOR: 3, /** - * Default index used to append a bookmark-item at the end of a folder. + * Default index used to append a bookmark-item at the end of a folder. * This should stay consistent with nsINavBookmarksService.idl */ DEFAULT_INDEX: -1, @@ -238,7 +238,7 @@ let Bookmarks = Object.freeze({ let updateInfo = validateBookmarkObject(info, { guid: { required: true } , index: { requiredIf: b => b.hasOwnProperty("parentGuid") - , validIf: b => b.index >= 0 } + , validIf: b => b.index >= 0 || b.index == this.DEFAULT_INDEX } , parentGuid: { requiredIf: b => b.hasOwnProperty("index") } }); @@ -307,9 +307,15 @@ let Bookmarks = Object.freeze({ // the same container. Thus we know it exists. if (!parent) parent = yield fetchBookmark({ guid: item.parentGuid }); - // Set index in the appending case. - if (updateInfo.index > parent._childCount) - updateInfo.index = parent._childCount; + + if (updateInfo.index >= parent._childCount || + updateInfo.index == this.DEFAULT_INDEX) { + updateInfo.index = parent._childCount; + + // Fix the index when moving within the same container. + if (parent.guid == item.parentGuid) + updateInfo.index--; + } } let updatedItem = yield updateBookmark(updateInfo, item, parent); @@ -1158,7 +1164,7 @@ function rowsToItemsArray(rows) { } return item; - }); + }); } /** @@ -1267,7 +1273,7 @@ function validateBookmarkObject(input, behavior={}) { } } if (required.size > 0) - throw new Error(`The following properties were expected: ${[...required].join(", ")}`); + throw new Error(`The following properties were expected: ${[...required].join(", ")}`); return normalizedInput; } diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js index 456a50cd1d78..d74dbc79cc41 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js @@ -402,6 +402,71 @@ add_task(function* update_move() { Assert.equal(descendant.index, 1); }); +add_task(function* update_move_append() { + let folder_a = + yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_FOLDER }); + checkBookmarkObject(folder_a); + let folder_b = + yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_FOLDER }); + checkBookmarkObject(folder_b); + + /* folder_a: [sep_1, sep_2, sep_3], folder_b: [] */ + let sep_1 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid, + type: PlacesUtils.bookmarks.TYPE_SEPARATOR }); + checkBookmarkObject(sep_1); + let sep_2 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid, + type: PlacesUtils.bookmarks.TYPE_SEPARATOR }); + checkBookmarkObject(sep_2); + let sep_3 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid, + type: PlacesUtils.bookmarks.TYPE_SEPARATOR }); + checkBookmarkObject(sep_3); + + function ensurePosition(info, parentGuid, index) { + checkBookmarkObject(info); + Assert.equal(info.parentGuid, parentGuid); + Assert.equal(info.index, index); + } + + // folder_a: [sep_2, sep_3, sep_1], folder_b: [] + sep_1.index = PlacesUtils.bookmarks.DEFAULT_INDEX; + // Note sep_1 includes parentGuid even though we're not moving the item to + // another folder + sep_1 = yield PlacesUtils.bookmarks.update(sep_1); + ensurePosition(sep_1, folder_a.guid, 2); + sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid); + ensurePosition(sep_2, folder_a.guid, 0); + sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid); + ensurePosition(sep_3, folder_a.guid, 1); + sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid); + ensurePosition(sep_1, folder_a.guid, 2); + + // folder_a: [sep_2, sep_1], folder_b: [sep_3] + sep_3.index = PlacesUtils.bookmarks.DEFAULT_INDEX; + sep_3.parentGuid = folder_b.guid; + sep_3 = yield PlacesUtils.bookmarks.update(sep_3); + ensurePosition(sep_3, folder_b.guid, 0); + sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid); + ensurePosition(sep_2, folder_a.guid, 0); + sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid); + ensurePosition(sep_1, folder_a.guid, 1); + sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid); + ensurePosition(sep_3, folder_b.guid, 0); + + // folder_a: [sep_1], folder_b: [sep_3, sep_2] + sep_2.index = Number.MAX_SAFE_INTEGER; + sep_2.parentGuid = folder_b.guid; + sep_2 = yield PlacesUtils.bookmarks.update(sep_2); + ensurePosition(sep_2, folder_b.guid, 1); + sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid); + ensurePosition(sep_1, folder_a.guid, 0); + sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid); + ensurePosition(sep_3, folder_b.guid, 0); + sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid); + ensurePosition(sep_2, folder_b.guid, 1); +}); + function run_test() { run_next_test(); }