Closed Bug 415043 Opened 16 years ago Closed 16 years ago

dragging many bookmarks duplicates them and mix up selection (but on folder change they are correct)


(Firefox :: Bookmarks & History, defect)

Not set



Firefox 3 beta3


(Reporter: mak, Assigned: mak)




(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
While working on selection i found this:

- 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
Flags: blocking-firefox3?
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]

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]

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
Blocks: 332047
Attached patch updated patch (obsolete) — Splinter Review
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
Attachment #300870 - Flags: review?(mano)
Comment on attachment 300870 [details] [diff] [review]
updated patch

clearing review by mano's request, coming soon with a new patch
Attachment #300870 - Flags: review?(mano)
Attached patch Updated patch (obsolete) — Splinter Review
added comments on return value

Mano: is this ok or should i add something more?
Attachment #300870 - Attachment is obsolete: true
Attachment #300957 - Flags: review?(mano)
Attachment #300957 - Attachment is obsolete: true
Attachment #300957 - Flags: review?(mano)
Attached patch Updated patchSplinter Review
throw if viewIndex is invalid
Attachment #300964 - Flags: review?(mano)
Comment on attachment 300964 [details] [diff] [review]
Updated patch

Attachment #300964 - Flags: review?(mano)
Attachment #300964 - Flags: review+
Attachment #300964 - Flags: approval1.9b3?
Attachment #300964 - Flags: approval1.9b3? → approval1.9b3+
Keywords: checkin-needed
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
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
Flags: blocking-firefox3?
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.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.