423200 - nsNavBookmarks.cpp: can't moveItem() one index down?

Status: RESOLVED FIXED

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

Description

[Adam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12) Gecko/20080129 Iceweasel Firefox/2.0.0.4 (Debian-2.0.0.12-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4

I can't seem to use the bookmarks service moveItem() function to move an item exactly one index down in its folder. I believe this is due to a bad check found on line 1620 of nsNavBookmarks.cpp:

http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#1620

1620     if (oldParent == aNewParent && newIndex > oldIndex) {
1621       // when an item is being moved lower in the same folder, the new index
1622       // refers to the index before it was removed. Removal causes everything
1623       // to shift up.
1624       --newIndex;
1625     }
1626   }
1627 
1628   // this is like the previous check, except this covers if
1629   // the specified index was -1 (append), and the calculated
1630   // new index is the same as the existing index
1631   if (aNewParent == oldParent && newIndex == oldIndex) {
1632     // Nothing to do!
1633     return NS_OK;
1634   }

If the new index is exactly one greater than the original index of the item, newIndex is decremented and the very next check results in no action.

Reproducible: Always

Steps to Reproduce:
  var BMSVC = Components.classes["@mozilla.org/browser/nav-bookmarks-service;1"]
                      .getService(Components.interfaces.nsINavBookmarksService);
  var BM = BMSVC.bookmarksMenuFolder;
  var item = BMSVC.getIdForItemAt(BM, 0);
  var oIdx = BMSVC.getItemIndex(item);
  BMSVC.moveItem(item, BM, 1);
  if (BMSVC.getItemIndex(item) == oIdx)
    alert('fail');
  else
    alert('success');
Actual Results:  
alert shows 'fail'

Expected Results:  
alert shows 'success'

I believe the solution is to make the check on line 1620:

"if (oldParent == aNewParent && newIndex > oldIndex+1)"

This should result in the proper behavior while removing the bad edge case that is failing.

The workaround I'm currently using is to get the item at the index I want to move to and then moving it up one instead.

Comment 1

[Dietrich Ayala (:dietrich)]

terrific, this basic scenario is not in the automated tests:

http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/tests/bookmarks/test_bookmarks.js

the tests there are all using default index (-1), or are testing cycles (parent = self, parent = child of parent).

scenarios that need tests:
- 1 up
- 1 down
- valid number up
- valid number down
- invalid number up (eg: > childCount)
- invalid number down (eg: < -1)

Comment 2

[Marco Bonardo [:mak]]

Adam, if you do BMSVC.moveItem(item, BM, 2); do you find the bookmark in the position 1?

in the backend when moving down in the same folder we are subtracting 1 to take in count old item position, so to move down by 1 position actually you should move down by 2.
it's something odd and undocumented, i'd prefer this kind of calculation done in the frontend, since now if i have item at position 5 and i call
moveItem(item, parent, 2);
moveItem(item, parent, 5);
i'd expect the item is back to initial position, but it's at position 4.

So your fix will work for moving down by 1, but then if you move down by 2 you will end up again moving down by 1.

The correct fix should be document this into the IDL, or change the backend behaviour to aways put in the requested position, and fix its current users in the frontend (i'd prefer the latter)

Comment 3

[Adam]

Marco,

You are correct about this fault.

The problem with the current behavior is consistency as your example with an item at position 5 shows.

My fix was naive, simply removing the check at 1620 entirely should solve this. 

I'm not sure how much code already expects the current behavior.

http://mxr.mozilla.org/firefox/search?string=.moveItem(

In nsPlacesTransactionsService.js it is just reusing the transaction's values for old and new index ( http://mxr.mozilla.org/firefox/source/browser/components/places/src/nsPlacesTransactionsService.js#459 ) so it is not taking the current behavior into account.

If you open the bookmarks window and drag a bookmark up one position, then try to undo it, it does nothing. Drag it up two and undo, it moves it back down only one.

utils.js http://mxr.mozilla.org/firefox/source/browser/components/places/content/utils.js#348 makes use of moveItem as well, but it also doesn't seem to account for this behavior.

I'm not sure what other users there are, but my best guess is they won't account for this behavior (and won't need updating).

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

we are not going to change the api at this time, the moveItem is drag&drop friendly like it is, so to move down in the same container add +1 to final index.

- added comment to API to indicate the need to add +1 when moving down into the same container

- fixed an index adjust when moving to new container (we need to add 1 from the new index on, and not from newIndex +1 on, or we'll end up with duped position value)

- fixed transaction manager to behave correctly when moving back to original position (for moveItem into the same container)

- created a specific moveItem test file (and moved out bits from test_bookmarks.js to there)

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch

Fixed a typo in bookmarks.idl, we are moving an itemId not only a folder

-  void moveItem(in long long aFolder, in long long newParent, in long aIndex);
+  void moveItem(in long long aItemId, in long long newParent, in long aIndex);

Comment 6

[Marco Bonardo [:mak]]

bump to normal severity due to index error when inserting into a new container

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 313595 [details] [diff] [review]
patch

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.54
>diff -u -8 -p -r1.54 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	17 Mar 2008 23:25:36 -0000	1.54
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	4 Apr 2008 12:23:50 -0000
>@@ -297,18 +297,22 @@ interface nsINavBookmarksService : nsISu
>   /**
>    * Moves an item to a different container, preserving its contents.
>    *  @param aItemId
>    *         The id of the item to move
>    *  @param aNewParent
>    *         The id of the new parent
>    *  @param aIndex
>    *         The index under aNewParent, or DEFAULT_INDEX to append
>+   *
>+   * NOTE: When moving down in the same container we take in count the

s/in count/into account/

>+   * removal of the original item. If you want to move from index X to
>+   * index Y > X you must use moveItem(folder, folder, Y + 1)

s/folder/id/ for clarity

>    */
>-  void moveItem(in long long aFolder, in long long newParent, in long aIndex);
>+  void moveItem(in long long aItemId, in long long newParent, in long aIndex);

s/newParent/aNewParent/ for consistency, and because that's how it's doc'd above.

r=me with these fixed

Comment 8

[Marco Bonardo [:mak]]

Attachment: for check-in

fixed review comments

Comment 9

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v  <--  nsINavBookmarksService.idl
new revision: 1.55; previous revision: 1.54
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.157; previous revision: 1.156
done
Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.43; previous revision: 1.42
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_moveitem.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_moveitem.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_moveitem.js,v  <--  test_moveitem.js
initial revision: 1.1
done

Comment 10

[Dietrich Ayala (:dietrich)]

Forgot the front-end file:

Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.37; previous revision: 1.36
done

Comment 11

[Josh Aas]

backed out due to unit test orange

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch

Thank you Josh, sorry for that, the transaction manager change came at last, i was sure to have checked unit tests but probably was before that :|

So, found and fixed another problem in transaction manager (when moving to -1 we should save the correct new index, not -1), plus fixed a wrong index check in the trasaction manager unit test (i've followed the test and that item should end up in index 2, previous wrong transaction manager behaviour made the test pass, badly). 
Asking an additional review because of new changes, executed the full testsuite check, all PASS now.

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 314071 [details] [diff] [review]
patch

r=me, thanks

Comment 14

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.39; previous revision: 1.38
done
Checking in browser/components/places/tests/unit/test_placesTxn.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_placesTxn.js,v  <--  test_placesTxn.js
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v  <--  nsINavBookmarksService.idl
new revision: 1.57; previous revision: 1.56
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.159; previous revision: 1.158
done
Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.45; previous revision: 1.44
done

Comment 15

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv