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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: 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

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 2

[Asaf Romano (gone)]

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).

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: 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 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

clearing review by mano's request, coming soon with a new patch

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Updated patch

added comments on return value

Mano: is this ok or should i add something more?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Updated patch

throw if viewIndex is invalid

Comment 8

[Asaf Romano (gone)]

Comment on attachment 300964 [details] [diff] [review]
Updated patch

r=mano.

Comment 9

[Reed Loden [:reed]]

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

Comment 10

[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