Created attachment 300620 [details] [diff] [review] patch While working on selection i found this: STR: - open the Library and go to a folder in the bookmarks menu - suppose you have bookmarks a,b,c,d,e,f,g,h,i - select b,c and drag them between f and g - you end up with a,c,e,f,b,c,g,h,i and selection is messed up - if you change folder and come back the results are correct this is mostly an itemMoved problem this should be blocking, makes reordering bookmarks an hell i've tested this patch and it's working fine with bookmarks and folders (open and closed). notice that the problem is visible in library but could be also in bookmarks sidebar
Attachment #300620 - Flags: review?(mano)
note: there is also a problem if you drag items in the white space under the tree, that regressed with Bug 412988 check-in, index becomes index -1, but it's already correct here, so the item is showed one position upper.
Comment on attachment 300620 [details] [diff] [review] patch itemRemoved doesn't always change the selection (think of a tree opened in a background window with some other selection in the same folder).
Attachment #300620 - Flags: review?(mano) → review-
Comment on attachment 300620 [details] [diff] [review] patch to make things clearer the main problem here is not the selection itself (minor issue) but that the code that recalculates the items to be removed is wrong, so you end up with visible (but not existant really) dupes. after a chat with mano i'm going to make a new patch partially duplicating itemRemoved code to make a correct removal without touching up the selection
Attachment #300620 - Attachment is obsolete: true
Created attachment 300870 [details] [diff] [review] updated patch instead of duplicating code i preferred to create a new utility function, that should be more mantainable (in future if you would have to fix itemRemoved you should also have to fix itemMoved). what this patch do: - add an internal method _fixViewIndexOnRemove to treeView.js, this contains the code that was previously used in itemRemoved, and is now used in itemMoved too (sanitized) - fix itemMoved and itemRemoved to use the new method (cleaned up no more used variables) notice: this fixed mixed-up selection too! i've tested dragging and removing in different views in Library and sidebar and i've not found any problem for now
Comment on attachment 300870 [details] [diff] [review] updated patch clearing review by mano's request, coming soon with a new patch
Created attachment 300957 [details] [diff] [review] Updated patch added comments on return value Mano: is this ok or should i add something more?
Comment on attachment 300964 [details] [diff] [review] Updated patch r=mano.
Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView.js new revision: 1.35; previous revision: 1.34 done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
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.