Closed
Bug 332047
Opened 19 years ago
Closed 17 years ago
Should select correct items (after delete, insert, paste, drop)
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta4
People
(Reporter: bugs, Assigned: mak)
References
Details
(Keywords: regression)
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•18 years ago
|
Assignee: bugs → nobody
Updated•17 years ago
|
Assignee: nobody → swon
Updated•17 years ago
|
Assignee: stevewon → christineyen+bugs
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
Attachment #274235 -
Flags: review?(sspitzer)
Comment 3•17 years ago
|
||
also removed a dump() I'd left in there...
Attachment #274235 -
Attachment is obsolete: true
Attachment #274247 -
Flags: review?(sspitzer)
Attachment #274235 -
Flags: review?(sspitzer)
Updated•17 years ago
|
Attachment #274247 -
Attachment is obsolete: true
Attachment #274247 -
Flags: review?(sspitzer)
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
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)
Attachment #274490 -
Flags: review?(sspitzer)
Updated•17 years ago
|
Status: NEW → ASSIGNED
Target Milestone: Firefox 2 beta1 → Firefox 3 M8
Updated•17 years ago
|
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Updated•17 years ago
|
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Updated•17 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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...
Assignee: christineyen+bugs → mak77
Attachment #274490 -
Attachment is obsolete: true
Attachment #292768 -
Flags: review?(dietrich)
Attachment #274490 -
Flags: review?(sspitzer)
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 :)
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
Attachment #292768 -
Attachment is obsolete: true
Attachment #292959 -
Flags: review?(dietrich)
Attachment #292768 -
Flags: review?(dietrich)
Assignee | ||
Comment 9•17 years ago
|
||
asking blocking since this is a regression from firefox 2
Flags: blocking-firefox3?
Keywords: regression
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Priority: -- → P2
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.
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...
Assignee | ||
Updated•17 years ago
|
Summary: Should select inserted items (paste, drop) → Should select correct items (after delete, insert, paste, drop)
Assignee | ||
Comment 12•17 years ago
|
||
this could probably fix also bug 410512 (i cannot reproduce that bug with this patch checked-in)
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.
Attachment #292959 -
Flags: review?(dietrich) → review-
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?)
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).
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)
Attachment #292959 -
Attachment is obsolete: true
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.
Attachment #297342 -
Attachment is obsolete: true
Updated•17 years ago
|
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Assignee | ||
Comment 18•17 years ago
|
||
Assignee | ||
Comment 19•17 years ago
|
||
for drag & drop we will need to fix Bug 415043 before controlling selection, adding dependancy
Depends on: 415043
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)
Assignee | ||
Updated•17 years ago
|
Attachment #300656 -
Attachment is obsolete: true
Assignee | ||
Comment 21•17 years ago
|
||
drag & drop + selection on bottom depends on Bug 415173
Depends on: 415173
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)
Attachment #300866 -
Attachment is obsolete: true
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.
Attachment #301266 -
Attachment is obsolete: true
Attachment #301497 -
Flags: review?(dietrich)
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.
Attachment #301497 -
Flags: review?(dietrich) → review-
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?
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.
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 :)
Attachment #301497 -
Attachment is obsolete: true
Attachment #301641 -
Flags: review?(mano)
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.
Attachment #301641 -
Flags: review?(mano) → review+
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
Attachment #301641 -
Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•