Ben Goodger (use ben at mozilla dot org for email) Reporter
	Description • 19 years ago

Steps to reproduce:
1. drag and drop an item

Results: 
The dropped item is not selected (this results in the selection being destroyed if the drop was to another position within the same folder)

Expected:

The dropped item is selected.

	Christine Yen
	Comment 2 • 17 years ago

Seems to work well, I'll outline some test cases in a bit for testing/reviewing this patch. I've seen a few crashes (one today, many yesterday on a much older version of the patch) in:

Thread 0 Crashed:
0   org.mozilla.firefox      	0x0080d725 nsTreeRange::Contains(int) + 9
1   org.mozilla.firefox      	0x006f4991 nsTreeSelection::IsSelected(int, int*) + 59
2   org.mozilla.firefox      	0x005cf78b nsTreeBodyFrame::PrefillPropertyArray(int, nsTreeColumn*) + 265
3   org.mozilla.firefox      	0x005d379e nsTreeBodyFrame::PaintRow(int, nsRect const&, nsPresContext*, nsIRenderingContext&, nsRect const&, nsPoint) + 60
4   org.mozilla.firefox      	0x005d40f9 nsTreeBodyFrame::PaintTreeBody(nsIRenderingContext&, nsRect const&, nsPoint) + 869
5   org.mozilla.firefox      	0x005d4268 PaintTreeBody(nsIFrame*, nsIRenderingContext*, nsRect const&, nsPoint) + 52

so... watch out for that as well. I've seen it once with something close to this patch, but can't reproduce it.

Anyway - the expected behavior of the selection in each case is described in the comment above restoreSelection. See Firefox 2's (relatively different) version of restoreSelection here: http://mxr.mozilla.org/mozilla1.8/source/browser/components/bookmarks/content/bookmarksTree.xml#434

	Christine Yen
	Comment 3 • 17 years ago

also removed a dump() I'd left in there...

	Christine Yen
	Comment 4 • 17 years ago

Test cases (for any link A, assume that link B is the item at the next tree index, by alphabetical order)

When testing, please note if a certain sequence of these test cases causes the selection to be restored incorrectly.

== REMOVAL ==

1. Remove Item A from the tree.
  Item B (the item which was in row B, but is now in row A) should be selected.

2. Remove items A and B from the tree.
  Item C (the item which was in row C, but is now in row A) should be selected.

3. Remove items A and C from the tree.
  Item B (the item which was in row B, but is now in row A) should be selected.

4. Delete the Item Z from the tree.
  Item Y should be selected.

5. Delete items V and Z from the tree.
  Item W (which is now occupying row V) should be selected.

== INSERTION ==

1. Select Item A, and insert a new separator/item/folder.
  The newly inserted Item B should be selected.

2. Select Items A and C, and insert a new item.
  The newly inserted Item D should be selected, and be inserted below Item C.

3. Copy Items A, B, and C. Don't paste them, but instead click Item D and try to insert a new item. (Similar to 
  The newly inserted item E should be the ONLY item selected. restoreSelection() should NOT try to highlight the same number of rows as the number of rows which were copied.

4. Copy Items A and B, select Item D, and paste from the clipboard.
  The newly pasted items A' and B', below Item D, should be selected.

== DRAG/DROP ==

1. Select Item A, and drag it up/down a few places by steps of 1.
  Item A should remain selected during the dragging process.

2. Select Item A, and drag it into the empty white area of the tree (make sure the window is larger than the contents of the tree.
  Item A is no longer selected, but is dropped at the end of the tree. (?)

3. Select Items A and B, and drag them up and down a few places by steps of 1.
  Items A and B should remain selected during the dragging process.

4. Select Items A and D, and drag them up or down a few places by steps of 1.
  Items A and D should be dropped next to each other, and selected.

5. Select Items A and D, and drag them in between Items B and C.
  Items A and D should now be next to each other, selected, and in between Items B and C.

6. Given a folder with items A, B, and C inside, followed by Items D and E outside the folder, drag Item B out of the folder and in between Items D and E.
  Item B should still be selected.

7. Drag Item A, from Folder A, and drop it onto Folder B.
  Since Item A is inserted at index = -1 inside Folder B, nothing is selected, and Item A is dropped at the bottom of Folder B.

8. Drag Item A, from inside Folder A to someplace inside Folder B.
  Item A is still selected.

9. Drag Items A and B, inside their respective Folders A and B, somewhere into the tree of the parent folder.
  Items A and B should be next to each other, on the same level as Folders A and B, and selected.

I feel like there are an infinite number of DnD test cases... see comments in bug 381751 for more

	Christine Yen
	Comment 5 • 17 years ago

INSERTION case 5:

5. Copy Items A and B, click on Folder C, and paste from the clipboard.
  The newly pasted items should not be selected (insertionpoint index == -1)

	Marco Bonardo [:mak] Assignee
	Comment 6 • 17 years ago

i've un-bitrot christine patch and fixed some minor bug, i've tested all the reported testcases in comment #4, except D&D 6,7,8,9, that require dragging between folders (since it does not still work). 
All testcases are working as expected.

there is still some minor bug like dragging the last element under itself selects everything, or dragging in the blank space does not select last element. But this could be a good starting point to move on with spinoff bugs.

Dietrich do you have time to test this and make a review if fine? Or someone else... 
Also if someone wants to test out the functionality it would be fine since there are a lot of testcases...

	Marco Bonardo [:mak] Assignee
	Comment 7 • 17 years ago

adding Mano to the cc since he is working on D&D and he will kill me for this :)

	Marco Bonardo [:mak] Assignee
	Comment 8 • 17 years ago

this fixes the following testcases (plus the other already fixed):

== DRAG/DROP ==

2. Select Item A, and drag it into the empty white area of the tree (make sure
the window is larger than the contents of the tree.
  Item A is selected at the end of the tree

10. Select Item Z, and drag it into the empty white area of the tree 
  Item Z is selected at the end of the tree

11. Select Item V, Z, and drag them into the empty white area of the tree 
  Items V,Z are selected at the end of the tree

12. Select Item V, Z, and drag them after the last node of the tree (so that the insertion line is shown after the node)
  Items V,Z are selected at the end of the tree

as before i cannot test drag into other folders since it does not work

	Marco Bonardo [:mak] Assignee
	Comment 9 • 17 years ago

asking blocking since this is a regression from firefox 2

	Dietrich Ayala (:dietrich)
	Comment 10 • 17 years ago

(In reply to comment #7)
> adding Mano to the cc since he is working on D&D and he will kill me for this
> :)
> 

marco (or mano) is this referring to specific bug? i'll go ahead and review this if it's not conflicting w/ other work.

	Marco Bonardo [:mak] Assignee
	Comment 11 • 17 years ago

mmh i know that Asaf was working with d&d on the same files, mainly with drag & drop items problems, but since some problems are caused by thread manager or core bugs i think that you could go on with review...

	Marco Bonardo [:mak] Assignee
	Comment 12 • 17 years ago

this could probably fix also bug 410512 (i cannot reproduce that bug with this patch checked-in)

	Asaf Romano (gone)
	Comment 13 • 17 years ago

Comment on attachment 292959 [details] [diff] [review]
un-bitrot, and fixed better

So, I don't think we should use the selection-restore code here (actually, with all my recent in-treeview selection work, I'm willing to drop it altogether). The insert-commands can probably call tree.selectItems with the returned itemId.

	Marco Bonardo [:mak] Assignee
	Comment 14 • 17 years ago

mano, instead of calling selectItems, could the treeview itself select new items on insert? i was thinking that we could put a lastInsertTime variable in the treeview, on iteminserted we check it and if null we set that to current time, if time elapsed > 0.5s (or less) we clear current selection and update lastInsertTime then select inserted item, if < 0.5s we add to current selection (so concurrent inserts are correctly selected). This way everything would be mantained in itemInserted not requiring other calls to set the correct selection. This should work for most insert/cut&paste (maybe also D&D?)

	Asaf Romano (gone)
	Comment 15 • 17 years ago

Unlike itemRemoved, there are are a lot of cases in which you don't want to change the selected item on-insert (esp. in smart queries).

	Marco Bonardo [:mak] Assignee
	Comment 16 • 17 years ago

mostly working

i still have problems with Drag&Drop, it appear that the tree is not rebuilt correctly (often after dragging i see duplicated items, closing and reopening the tree makes them disappear, is this a refreshVisibleSection bug?)

also i cannot insert in the sidebar because it is trying to insert into container itemId, while those containers are special folders (so getDropPoint should return the correct folderId and relative index insted of itemId and absolute index)

	Marco Bonardo [:mak] Assignee
	Comment 17 • 17 years ago

Comment on attachment 297342 [details] [diff] [review]
wip, using selectItems and insertionPoint

not working fine in bookmarks sidebar, due to recent getConcreteItemId work from Mano. Particular we can't rely on ip.itemId since it could be a Concrete one and not the real itemId in the tree (for folder shortcuts).

Since we don't know the actual new itemId in the newItem, newSeparator, newFolder functions, i'm adding a method to Bookmark Service to get an itemId knowing its FolderId and Position, we actually don't have such a method that could also be useful elsewhere, now that we have concreteIds for special folders.

this way i can get the itemId starting from insertion point, and select that with selectItems.

	Marco Bonardo [:mak] Assignee
	Comment 19 • 17 years ago

for drag & drop we will need to fix Bug 415043 before controlling selection, adding dependancy

	Marco Bonardo [:mak] Assignee
	Comment 20 • 17 years ago

this is quite ready for review, but i'd like to try fix dependant bug before, to be able to test selection also on dragging

what this patch will do:
- add a method getIdForItemAt to Bookmarks Service, this will return the itemId when you know the parent and the index of an item
- use the new method to select new and pasted nodes based on insertionPoint
- add a couple of tests for the new API in unit tests
- remove old save/restoreselection methods from tree.xml
- fix itemRemoved so that when you delete the last item in the view the new last item is selected (now nothing is selected if you remove the last item)

what this patch does not do:
- fix selection in D&D (should be done in Bug 415043)

	Marco Bonardo [:mak] Assignee
	Comment 21 • 17 years ago

drag & drop + selection on bottom depends on Bug 415173

	Marco Bonardo [:mak] Assignee
	Comment 22 • 17 years ago

- now backend GetIdForItemAt will return the last itemId if input index == -1
- added a private method getLastChildId for better readability
- with Bug 415521 we will select the correct item when inserting in sorted views
- fixed tests accordingly (removed old test request for a getItemAtIndex method)

	Marco Bonardo [:mak] Assignee
	Comment 23 • 17 years ago

unbitrotted patch

this (with ondrej's backout patch in bug 415173) fixes most things on insert / remove / paste

notice: drag & drop could still have some selection issue, it's difficult to test since there are many bugs still open there, i think that we will be able to open single spun-off bugs on drag&drop selection problems, this mostly removes the old borked selection save/restore code, that is totally unrelated to drag & drop moves.

	Asaf Romano (gone)
	Comment 24 • 17 years ago

Comment on attachment 301497 [details] [diff] [review]
updated patch

These changes are awesome.

Index: browser/components/places/content/controller.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
retrieving revision 1.198
diff -u -8 -p -r1.198 controller.js
--- browser/components/places/content/controller.js	1 Feb 2008 03:03:14 -0000	1.198
+++ browser/components/places/content/controller.js	5 Feb 2008 13:49:04 -0000
@@ -779,54 +779,68 @@ PlacesController.prototype = {
    * @param aType
    *        the type of the new item (bookmark/livemark/folder)
    */
   newItem: function PC_newItem(aType) {
     var ip = this._view.insertionPoint;
     if (!ip)
       throw Cr.NS_ERROR_NOT_AVAILABLE;
 
-    this._view.saveSelection(this._view.SAVE_SELECTION_INSERT);
-
     var performed = false;
     if (aType == "bookmark")
       performed = PlacesUtils.showAddBookmarkUI(null, null, null, ip);
     else if (aType == "livemark")
       performed = PlacesUtils.showAddLivemarkUI(null, null, null, null, ip);
     else // folder
       performed = PlacesUtils.showAddFolderUI(null, ip);
 
-    if (performed)
-      this._view.restoreSelection();
+    if (performed) {
+      // select the new inserted item

nit: either inserted or new, pick one :)

In all other places too.

@@ -1196,16 +1210,24 @@ PlacesController.prototype = {
 
     // Get transactions to paste any folders, separators or links that might
     // be on the clipboard, aggregate them and execute them. 
     var transactions = getTransactions([PlacesUtils.TYPE_X_MOZ_PLACE,
                                         PlacesUtils.TYPE_X_MOZ_URL, 
                                         PlacesUtils.TYPE_UNICODE]);
     var txn = PlacesUtils.ptm.aggregateTransactions("Paste", transactions);
     PlacesUtils.ptm.doTransaction(txn);
+
+    // select the pasted items, they should be consecutive
+    var insertedNodeIds = [];
+    for (var i = 0; i < transactions.length; ++i)
+      insertedNodeIds.push(PlacesUtils.bookmarks
+                                      .getIdForItemAt(ip.itemId, ip.index + i));
+    if (insertedNodeIds.length > 0)
+      this._view.selectItems(insertedNodeIds);

selectItem takes an array.

Index: browser/components/places/content/tree.xml
===================================================================

The methods should be removed from menu.xml and toolbar.xml as well.

Index: browser/components/places/content/treeView.js
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
retrieving revision 1.35
diff -u -8 -p -r1.35 treeView.js
--- browser/components/places/content/treeView.js	4 Feb 2008 01:22:39 -0000	1.35
+++ browser/components/places/content/treeView.js	5 Feb 2008 13:49:06 -0000
@@ -742,18 +742,24 @@ PlacesTreeView.prototype = {
           this.nodeForTreeIndex(min.value) == aItem)
         selectNext = true;
     }
 
     // remove the item and fix viewIndex values
     this._fixViewIndexOnRemove(aItem, aParent);
 
     // restore selection if the item was exclusively selected
-    if (selectNext && this._visibleElements.length > oldViewIndex)
+    if (!selectNext)
+      return;
+    // restore selection
+    if (this._visibleElements.length > oldViewIndex)
       selection.rangedSelect(oldViewIndex, oldViewIndex, true);
+    else if (this._visibleElements.length > 0)
+      selection.rangedSelect(this._visibleElements.length - 1,
+                             this._visibleElements.length - 1, true);
   },
 
   /**
    * Be careful, aOldIndex and aNewIndex specify the index in the
    * corresponding parent nodes, not the visible indexes.
    */
   itemMoved:
   function PTV_itemMoved(aItem, aOldParent, aOldIndex, aNewParent, aNewIndex) {
   
Please explaing these changes.

Index: toolkit/components/places/src/nsNavBookmarks.cpp
===================================================================

+nsresult
+nsNavBookmarks::GetLastChildId(PRInt64 aFolder, PRInt64* aItemId)
+{
+  mozIStorageConnection *dbConn = DBConn();
+  nsCOMPtr<mozIStorageStatement> statement;
+  PRBool hasMore;
+  nsresult rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
+      "SELECT id FROM moz_bookmarks WHERE parent = ?1 "
+      "ORDER BY position DESC LIMIT 1"), getter_AddRefs(statement));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = statement->BindInt64Parameter(0, aFolder);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = statement->ExecuteStep(&hasMore);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!hasMore) {
+    // Item doesn't exist, we set aItemId to zero and return with error
+    *aItemId = 0;
+    return NS_ERROR_INVALID_ARG;
+  }
+  *aItemId = statement->AsInt64(0);
+  return NS_OK;
+}
+

See comments below about the exception, they apply here too.

+NS_IMETHODIMP
+nsNavBookmarks::GetIdForItemAt(PRInt64 aFolder, PRInt32 aIndex, PRInt64* aItemId)
+{
+  nsresult rv;
+  // if index == -1 we want the last item into this container

s/into this container/within aFolder/ maybe?

+  if (aIndex == -1) {
+    rv = GetLastChildId(aFolder, aItemId);
+    NS_ENSURE_SUCCESS(rv, rv);

shouldn't this be |return GetLastChildId(aFolder, aItemId);|?

+  }
+  else {
+    mozIStorageConnection *dbConn = DBConn();
+    {
+      // get the item in aFolder with position aIndex
+      mozStorageStatementScoper scope(mDBGetChildAt);
+      PRBool hasMore;
+      rv = mDBGetChildAt->BindInt64Parameter(0, aFolder);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = mDBGetChildAt->BindInt32Parameter(1, aIndex);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = mDBGetChildAt->ExecuteStep(&hasMore);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      if (!hasMore) {
+        // Item doesn't exist, we set aItemId to zero and return with error
+        *aItemId = 0;

if we wouldn't throw, -1 would be more consistent with the result nodes API.

+        return NS_ERROR_INVALID_ARG;

this makes your checks in browser code kinda pointless.

I'm actually fine with throwing here, but then why bother:
1. checking the out-param in front-end cde.
2. set the out param value here.


Index: toolkit/components/places/public/nsINavBookmarksService.idl
===================================================================
@@ -330,16 +330,27 @@ interface nsINavBookmarksService : nsISu
    *  @param aFolder
    *         The folder to remove a child from
    *  @param aIndex
    *         The index of the child to remove
    */
   void removeChildAt(in long long aFolder, in long aIndex);
 
   /**
+   * Get the ItemId given the containing folder and the index.

mega-nit: itemId :)

+   * NOTE: If index == -1 will return the last item of the folder, if exists.
+   *  @param aFolder
+   *         The folder where to search
+   *  @param aIndex
+   *         The index of the item into the folder, -1 for the last item
+   *  @returns the id of the found item, 0 if not found

Please document the exception.

Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
===================================================================

It's time for a new file.

	Marco Bonardo [:mak] Assignee
	Comment 25 • 17 years ago

MAno: thank you for review, will fix soon, i still have some question:

> +
> +    // select the pasted items, they should be consecutive
> +    var insertedNodeIds = [];
> +    for (var i = 0; i < transactions.length; ++i)
> +      insertedNodeIds.push(PlacesUtils.bookmarks
> +                                      .getIdForItemAt(ip.itemId, ip.index +
> i));
> +    if (insertedNodeIds.length > 0)
> +      this._view.selectItems(insertedNodeIds);
> 
> selectItem takes an array.

insertedNodeIds is an array...


> I'm actually fine with throwing here, but then why bother:
> 1. checking the out-param in front-end cde.
> 2. set the out param value here.

what do you think is better? throw or return -1, try to continue and do checks in the frontend?


> Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
> ===================================================================

> It's time for a new file.

what do you mean here?

	Asaf Romano (gone)
	Comment 26 • 17 years ago

(In reply to comment #25)
> MAno: thank you for review, will fix soon, i still have some question:
> 

nit: Mano, or Asaf. Thanks.

> > +
> > +    // select the pasted items, they should be consecutive
> > +    var insertedNodeIds = [];
> > +    for (var i = 0; i < transactions.length; ++i)
> > +      insertedNodeIds.push(PlacesUtils.bookmarks
> > +                                      .getIdForItemAt(ip.itemId, ip.index +
> > i));
> > +    if (insertedNodeIds.length > 0)
> > +      this._view.selectItems(insertedNodeIds);
> > 
> > selectItem takes an array.
> 
> insertedNodeIds is an array...
> 
yeah, read that wrong, sorry.
 
> > I'm actually fine with throwing here, but then why bother:
> > 1. checking the out-param in front-end cde.
> > 2. set the out param value here.
> 
> what do you think is better? throw or return -1, try to continue and do checks
> in the frontend?

I prefer throwing, hopefully the front-end code cannot actually get bogus indexes, and if it does, i want to to about these case via the console.
 
> 
> > Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
> > ===================================================================
> 
> > It's time for a new file.
> 
> what do you mean here?
> 

test_332047.js! test_bookmarks.js is way too long for any addition at this point.

	Marco Bonardo [:mak] Assignee
	Comment 27 • 17 years ago

(In reply to comment #24)
> (From update of attachment 301497 [details] [diff] [review])
> Index: browser/components/places/content/controller.js
> ===================================================================
> -    if (performed)
> -      this._view.restoreSelection();
> +    if (performed) {
> +      // select the new inserted item
> 
> nit: either inserted or new, pick one :)

fixed

> Index: browser/components/places/content/tree.xml
> ===================================================================
> 
> The methods should be removed from menu.xml and toolbar.xml as well.

done

> Index: browser/components/places/content/treeView.js
> ===================================================================
> +    if (!selectNext)
> +      return;
> +    // restore selection
> +    if (this._visibleElements.length > oldViewIndex)
>        selection.rangedSelect(oldViewIndex, oldViewIndex, true);
> +    else if (this._visibleElements.length > 0)
> +      selection.rangedSelect(this._visibleElements.length - 1,
> +                             this._visibleElements.length - 1, true);
> 
> Please explaing these changes.

done

> Index: toolkit/components/places/src/nsNavBookmarks.cpp
> ===================================================================
> +  if (aIndex == -1) {
> +    rv = GetLastChildId(aFolder, aItemId);
> +    NS_ENSURE_SUCCESS(rv, rv);
> 
> shouldn't this be |return GetLastChildId(aFolder, aItemId);|?

yes, done

> +      if (!hasMore) {
> +        // Item doesn't exist, we set aItemId to zero and return with error
> +        *aItemId = 0;
> 
> if we wouldn't throw, -1 would be more consistent with the result nodes API.

we throw

> +        return NS_ERROR_INVALID_ARG;
> 
> this makes your checks in browser code kinda pointless.

removed checks

> Index: toolkit/components/places/public/nsINavBookmarksService.idl
> ===================================================================
> +   * Get the ItemId given the containing folder and the index.
> 
> mega-nit: itemId :)

mega-fixed

> +   * NOTE: If index == -1 will return the last item of the folder, if exists.
> +   *  @param aFolder
> +   *         The folder where to search
> +   *  @param aIndex
> +   *         The index of the item into the folder, -1 for the last item
> +   *  @returns the id of the found item, 0 if not found
> 
> Please document the exception.

done

> Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
> ===================================================================
> 
> It's time for a new file.

i've not created a new file because i've inserted getIdForItemAt into the actual  move test. This was already requested by an XXX todo comment, it's about 10 additional rows, 5 of which are comments, this is more a completion of move tests rather than a direct test of GetIdForItemAt (it gets however tested using it into move tests).

> MAno: thank you for review, will fix soon, i still have some question:
> 
>nit: Mano, or Asaf. Thanks.

patched my brain :)

	Asaf Romano (gone)
	Comment 28 • 17 years ago

Comment on attachment 301641 [details] [diff] [review]
updated patch

>Index: browser/components/places/content/treeView.js
>===================================================================
>     // restore selection if the item was exclusively selected
>-    if (selectNext && this._visibleElements.length > oldViewIndex)
>+    if (!selectNext)
>+      return;
>+    // restore selection
>+    if (this._visibleElements.length > oldViewIndex)
>       selection.rangedSelect(oldViewIndex, oldViewIndex, true);
>+    // if we removed the last child, we select the new last child if exists
>+    else if (this._visibleElements.length > 0)

please move this comment into the |else if| block

>+      selection.rangedSelect(this._visibleElements.length - 1,
>+                             this._visibleElements.length - 1, true);
>   },
> 
>   /**
>    * Be careful, aOldIndex and aNewIndex specify the index in the
>    * corresponding parent nodes, not the visible indexes.
>    */
>   itemMoved:
>   function PTV_itemMoved(aItem, aOldParent, aOldIndex, aNewParent, aNewIndex) {

>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================
>+nsresult
>+nsNavBookmarks::GetLastChildId(PRInt64 aFolder, PRInt64* aItemId)
>+{
>+  mozIStorageConnection *dbConn = DBConn();
>+  nsCOMPtr<mozIStorageStatement> statement;
>+  PRBool hasMore;
>+  nsresult rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT id FROM moz_bookmarks WHERE parent = ?1 "
>+      "ORDER BY position DESC LIMIT 1"), getter_AddRefs(statement));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = statement->BindInt64Parameter(0, aFolder);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = statement->ExecuteStep(&hasMore);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!hasMore) {
>+    // Item doesn't exist
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+  *aItemId = statement->AsInt64(0);
>+  return NS_OK;
>+}
>+

you don't like empty lines, do you? :) please add some.

>+NS_IMETHODIMP
>+nsNavBookmarks::GetIdForItemAt(PRInt64 aFolder, PRInt32 aIndex, PRInt64* aItemId)
>+{
>+  nsresult rv;
>+  // if index == -1 we want the last item within aFolder
>+  if (aIndex == -1)
>+    return GetLastChildId(aFolder, aItemId);
>+  else {
>+    mozIStorageConnection *dbConn = DBConn();

Per the code style conventions for this file, this should be:

>+  // if index == -1 we want the last item within aFolder
>+  if (aIndex == -1) {
>+    return GetLastChildId(aFolder, aItemId);
>+  } else {
>+    mozIStorageConnection *dbConn = DBConn();

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>@@ -330,16 +330,28 @@ interface nsINavBookmarksService : nsISu
>    *  @param aFolder
>    *         The folder to remove a child from
>    *  @param aIndex
>    *         The index of the child to remove
>    */
>   void removeChildAt(in long long aFolder, in long aIndex);
> 
>   /**
>+   * Get the itemId given the containing folder and the index.
>+   * NOTE: If index == -1 will return the last item of the folder, if exists.
>+   *  @param aFolder
>+   *         The folder where to search

The direct parent folder of the item.

>+   *  @param aIndex
>+   *         The index of the item into the folder, -1 for the last item

within aFolder, DEFAULT_INDEX for the last item. I don't think you need the extra NOTE.

>+   *  @returns the id of the found item
>+   *  @throws if the item does not exist
>+   */
>+  long long getIdForItemAt(in long long aFolder, in long aIndex);
>+
>+  /**
>    * Get a globally unique identifier for an item, meant to be used in
>    * sync scenarios.  Even if their contents are exactly the same
>    * (including an item in a different profile with the same ItemId),
>    * the GUID would be different.
>    *  @param   aItemId
>    *           The ID of the item to get the GUID for
>    *  @returns The GUID string
>    */
>Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
>===================================================================

>-  // try to get index of the folder from it's ex-parent
>-  // XXX expose getItemAtIndex(folder, idx) to test that the item was *removed* from the old parent?
>-  // XXX or expose FolderCount, and check that the old parent has one less kids?
>+  // try to get index of the folder from its ex-parent
>+  // check that it has been really removed from there
>+  do_check_neq(bmsvc.getIdForItemAt(testRoot, 0), workFolder);
>+  // check the last item from old-parent
>+  do_check_neq(bmsvc.getIdForItemAt(testRoot, -1), workFolder);
>+  // check the index of the item into new-parent
>+  do_check_eq(bmsvc.getIdForItemAt(homeFolder, 1), workFolder);
>+  // try to get index of the last item from new-parent

within the new parent folder

r=mano otherwise.

	Marco Bonardo [:mak] Assignee
	Comment 29 • 17 years ago

(In reply to comment #28)
> (From update of attachment 301641 [details] [diff] [review])
> >Index: browser/components/places/content/treeView.js
> >===================================================================
> >+    if (this._visibleElements.length > oldViewIndex)
> >       selection.rangedSelect(oldViewIndex, oldViewIndex, true);
> >+    // if we removed the last child, we select the new last child if exists
> >+    else if (this._visibleElements.length > 0)
> 
> please move this comment into the |else if| block

done

> >Index: toolkit/components/places/src/nsNavBookmarks.cpp
> >===================================================================
> >+nsresult
> >+nsNavBookmarks::GetLastChildId(PRInt64 aFolder, PRInt64* aItemId)
> >+{
> you don't like empty lines, do you? :) please add some.

added a bunch

> >+NS_IMETHODIMP
> >+nsNavBookmarks::GetIdForItemAt(PRInt64 aFolder, PRInt32 aIndex, PRInt64* aItemId)
> >+{
> >+  nsresult rv;
> >+  // if index == -1 we want the last item within aFolder
> >+  if (aIndex == -1)
> >+    return GetLastChildId(aFolder, aItemId);
> >+  else {
> >+    mozIStorageConnection *dbConn = DBConn();
> 
> Per the code style conventions for this file, this should be:

fixed

> >Index: toolkit/components/places/public/nsINavBookmarksService.idl
> >===================================================================
> >+   *  @param aIndex
> >+   *         The index of the item into the folder, -1 for the last item
> 
> within aFolder, DEFAULT_INDEX for the last item. I don't think you need the
> extra NOTE.

removed NOTE, fixed idl (also other places) to use DEFAULT_INDEX instead of -1 in comments

> >Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js
> >===================================================================
> >+  do_check_eq(bmsvc.getIdForItemAt(homeFolder, 1), workFolder);
> >+  // try to get index of the last item from new-parent
> 
> within the new parent folder

fixed most comments to use a more correct form

	Asaf Romano (gone)
	Comment 30 • 17 years ago

mozilla/browser/components/places/content/controller.js 1.199
mozilla/browser/components/places/content/menu.xml 1.99
mozilla/browser/components/places/content/toolbar.xml 1.119
mozilla/browser/components/places/content/tree.xml 1.93
mozilla/browser/components/places/content/treeView.js 1.37
mozilla/toolkit/components/places/public/nsINavBookmarksService.id 1.53
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.149
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.51
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.41

	Mike Lierman
	Comment 31 • 17 years ago

*** VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre

-Mike

	Tracy Walker [:tracy]
	Comment 32 • 17 years ago

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre)
Gecko/2008032106 Minefield/3.0b5pre
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre)
Gecko/2008032106 Minefield/3.0b5pre

adding in-litmus?  I want to use the various cases in comment #4 as a guideline to create more detailed individual test cases in litmus, rather than the broad scoped ones we have now.

	Gervase Markham [:gerv]
	Comment 33 • 15 years ago

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

	Tracy Walker [:tracy]
	Comment 34 • 11 years ago

removing in-litmus flag, it no longer exists