1081108 - Implement reorder in Bookmarks.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

We must implement reorder in Bookmarks.jsm so we can finally replace usage of setItemIndex.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

We're dropping it from this iteration for now and will pick it up again later.

Comment 2

[Marco Bonardo [:mak]]

Attachment: MozReview Request: bz://1081108/mak

Comment 3

[Marco Bonardo [:mak]]

/r/1523 - Bug 1081108 - Implement reorder in Bookmarks.jsm. r=Mano

Pull down this commit:

hg pull review -r 813c0839f7c141447e352f3ed3f6e22fc0aa250a

Comment 4

[Asaf Romano (gone)]

https://reviewboard.mozilla.org/r/1523/#review1019

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +        throw new Error("No folder found for the provided GUID.");

This sounds as if the guid isn't found. The error message should reflect that the pointed item isn't a folder.

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +    // Reorder the children array according to the wanted order, provided

s/wanted/specified

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +      return (i != -1 && j != -1 && i < j) || (i != -1 && j == -1) ? -1 : 1;

So, given a folder that looks like this pre-sort:
[a, b, d, c] and a [a, b] reorder call, does this ensure that c will still follow d?

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +    // Update the position now.  If any unknown GUID should have been

Either "Should any unknown guid have been inserted meanwhile" or "If any unknown guid have been inserted meanwhile"

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +    // To do the update in one step, we build a VALUES (guid, position)

a single step/query

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +                                           i, child.type,

If you use child.index here (rather than i), you can use a for-of loop.

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +         JOIN sorting b ON b.p <= a.p 

trailing space

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> +    // table. We use count() in the sorting table to avoid skipping values

We then

::: toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
(Diff revision 1)
> +    },

Remove ,

::: toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
(Diff revision 1)
> +  }

sorted = [for (bm of bookmarks] yield PlacesUtils.bookmarks.insert(b)]

Comment 5

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/1523/#review1021

> If you use child.index here (rather than i), you can use a for-of loop.

child.index is NOT updated... I could update it, with another loop, but I didn't so far cause we are not returning them.

> So, given a folder that looks like this pre-sort:
> [a, b, d, c] and a [a, b] reorder call, does this ensure that c will still follow d?

nope, ordering of not passed-in guids is unspecified, retaining it would be more complex (I'm not even sure if it is possible with this implmentation)

Comment 6

[Marco Bonardo [:mak]]

I can probably try to retain the original order by setting position to -position rather than -1. I have some tentative queries that seem to be working. let me see.

Comment 7

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/1523/#review1025

> This sounds as if the guid isn't found. The error message should reflect that the pointed item isn't a folder.

It could also not be found
(!parent || parent.type != this.TYPE_FOLDER) so the message is more generic on purpose.

> child.index is NOT updated... I could update it, with another loop, but I didn't so far cause we are not returning them.

Moreover child.index is the old index and we need both old and new index for the notification.

Comment 8

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/1523/#review1023

Comment 9

[Marco Bonardo [:mak]]

/r/1523 - Bug 1081108 - Implement reorder in Bookmarks.jsm. r=Mano

Pull down this commit:

hg pull review -r 06226488a19a041b50fa49aff220c460c9c3d2c6

Comment 10

[Asaf Romano (gone)]

https://reviewboard.mozilla.org/r/1521/#review1211

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1081108 - Implement reorder in Bookmarks.jsm. r=Mano