1400846 - Bookmark ordering -- folders or BM

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Bruce]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170917100334

Steps to reproduce:

Bookmarks rearranging in Fx57 32bit Nightly

When moving a bookmark or folder from Level2 to Level1 or level3, or Level3 to Level2 or Level1 the moved BM or folder will jump up 1 row in the listing.

Reproducible: always

STR:
Level1
 . . . Level2
 . . . . . .  Level3

Create a folder at level3 and move it to Level1 or Level2 -- using the dark line placement indicator the moved BM or folder will be inserted 1 row higher than expected.


Actual results:

The moved BM or folder ends up 1 line higher than where the indicator is showing


Expected results:

The moved BM or folder should be inserted where the indicator is showing

Comment 1

[Marco Bonardo [:mak]]

Where are you doing the moving, in the bookmarks sidebar, in the library left pane, from the Librart left pane to the right pane (or viceversa)?

Where is the placement indicator shown?

I tried to move a folder I created at level 3 into folder 1 and folder 2, and had no problems doing that, so far.
A video may surely help, but also more specific steps about WHERE and HOW to move the bookmark may help.

Comment 2

[Bruce]

Click Bookmarks/Show All Bookmarks from the file menu

In the left column below this is where it occurs.

If I move any folder from a different level (directory wise) when I select the location (dark line indicator) the item is inserted 1 row above where I want.

I use the thick dark line for placement -- I believe this is the intent.

Image:
[img]https://i.imgur.com/STE21k2.jpg[/img]

Sorry -- I don't know how to make a video.

Comment 3

[Eddward]

Maybe I can reproduce the issue using Bookmarks toolbar.
STR:
1. Create a folder on the Bookmarks toolbar.
2. Create a few bookmarks (3) in this folder and 1 subfolder.
3. Create a bookmark in this new subfolder.
4. Move the bookmark from this subfolder to the folder on the Bookmarks toolbar using mouse and try to move it to the bottom, so the placement indicator will be below the last bookmark. 
5. Notice that the bookmark will end up above the last bookmark not below as expected.

Comment 4

[Alice0775 White]

[Tracking Requested - why for this release]: Bookmark ordering broken

Comment 5

[Marco Bonardo [:mak]]

why should we track this, it's not even clear if it's a 57 regression?
If it's related  to the async transactions work, that is not going to ride the train to 57, regardless.

Comment 6

[Alice0775 White]

[Tracking Requested - why for this release]:it's a 57 regression

regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f950e9aa797d68fc58ff395fcf05e656e73b7cbf&tochange=7d7e8ffee1356b9270b9dbee860a952df4a61ec9

Comment 7

[Mark Banner (:standard8)]

Just to confirm this is async transactions only, and they'll be turned off for 57. Putting this in my queue as I think I know what caused this.

Comment 8

[Eddward]

Yes, I can not reproduce the issue if I set browser.places.useAsyncTransactions to false. Currently it is true by default.

Comment 9

[Bruce]

I too can confirm it works correctly with browser.places.useAsyncTransactions; false and a restart on the browser.

Comment 10

[Mark Banner (:standard8)]

Attachment: Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

Review commit: https://reviewboard.mozilla.org/r/181268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181268/

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

https://reviewboard.mozilla.org/r/181268/#review187076

::: browser/components/places/content/controller.js:1653
(Diff revision 1)
> +          if (existingBookmark && parentGuid == existingBookmark.parentGuid) {
> +            let dragginUp = index < existingBookmark.index;
> +
> -          if (dragginUp) {
> +            if (dragginUp) {
> -            index += movedCount++;
> +              index += movedCount++;
> -          } else if (PlacesUIUtils.useAsyncTransactions) {
> +            } else if (PlacesUIUtils.useAsyncTransactions) {

So, even if it may be correct, I find this if (draggingup) else if (asynctransactions) very confusing.

I'd prefer an explicit 
if (asynctransactions) { ... } else { ... } even if that means duplicating the dragginUp thing inside each brace.
It will also be easier to remove in the future.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181268/diff/1-2/

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

https://reviewboard.mozilla.org/r/181268/#review187482

::: browser/components/places/content/controller.js:1664
(Diff revision 2)
> +                continue;
> +              }
> +            } else {
> +              // Sync Transactions.
> +
> +              // Adjust insertion index to prevent reversal of dragged items. When you

too many newlines.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8909769 [details]
Bug 1400846 - Fix ordering of bookmarks with Async Transactions when dropping into different folders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181268/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96757433b36d
Fix ordering of bookmarks with Async Transactions when dropping into different folders. r=mak

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/96757433b36d

Comment 17

[Bruce]

I have verified this bug is fixed with browser.places.useAsyncTransactions set true.

Thank you all for your work.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 19

[Mark Banner (:standard8)]

This only affects async places transactions which we're not shipping in 57.

Comment 20

[Hani Yacoub]

Build ID: 20171016220427
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.