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

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
Firefox 3 beta3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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
Flags: blocking-firefox3?
Attachment #300620 - Flags: review?(mano)
(Assignee)

Comment 1

10 years ago
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-
(Assignee)

Comment 3

10 years ago
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
(Assignee)

Updated

10 years ago
Blocks: 332047
(Assignee)

Comment 4

10 years ago
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
Attachment #300870 - Flags: review?(mano)
(Assignee)

Comment 5

10 years ago
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)
(Assignee)

Comment 6

10 years ago
Created attachment 300957 [details] [diff] [review]
Updated patch

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)
(Assignee)

Updated

10 years ago
Attachment #300957 - Attachment is obsolete: true
Attachment #300957 - Flags: review?(mano)
(Assignee)

Comment 7

10 years ago
Created attachment 300964 [details] [diff] [review]
Updated patch

throw if viewIndex is invalid
(Assignee)

Updated

10 years ago
Attachment #300964 - Flags: review?(mano)
Comment on attachment 300964 [details] [diff] [review]
Updated patch

r=mano.
Attachment #300964 - Flags: review?(mano)
Attachment #300964 - Flags: review+
Attachment #300964 - Flags: approval1.9b3?

Updated

10 years ago
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
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
(Assignee)

Updated

10 years ago
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.

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.