387035 - Insert index (DnD)/orientation (cutpaste) is being incorrectly calculated in left pane of BM

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[Christine Yen]

Steps to reproduce:

1. In the Bookmarks Manager, arrange items so there are a few bookmarks, a folder, and a few more bookmarks below the folder.

2. Drag and drop one of the lower bookmarks to the left pane until the drop indicator is a line underneath the first folder, aiming for directly under the first folder.

Expected results: the bookmark appears in the right pane directly below the desired folder.

Actual results: the bookmark appears at the second position on the right, at the same index the folder appeared in the left pane.

Comment 1

[Christine Yen]

Forgot to say -- this error also appears when copying/pasting bookmarks by clicking on a folder in the left pane, when the selected folder is closed.

Comment 2

[Christine Yen]

Attachment: patch

Behavior with this patch applied:

Paste with a folder in the left panel selected, folder either open or closed: the item will be pasted to the bottom of the selected (and thus displayed-in-the-right-panel) folder.

Drag onto a folder in the left panel, either open or closed: the item will be dropped at the bottom of the target folder.

Drag between folders in the left panel, cursor above drop indicator: the item will be dropped directly below the first folder (visible in the right panel).

Drag between folders in the left panel, cursor below drop indicator: the item will be dropped directly above the second folder (visible in the right panel).

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 271172 [details] [diff] [review]
patch

Christine,

About your patch for https://bugzilla.mozilla.org/show_bug.cgi?id=387035

Can you explain why you need to check both of these?

+              // see if the container is the left pane, and excludes items
+              var excludeItems = false;
+              if (PlacesUtils.nodeIsFolder(container))
+                excludeItems = container.queryOptions.excludeItems;
+              else if (PlacesUtils.nodeIsQuery(container))
+                excludeItems = asQuery(container).queryOptions.excludeItems;

Also, what about adding a helper function it to PlacesUtils, since you call it twice?

var excludeItems = PlacesUtils.nodeExcludesItems(container);

Then do:

var lsi = PlacesUtils.nodeExcludesItems(container) ? lastSelected.bookmarkIndex : PlacesUtils.getIndexOfNode(lastSelected);

Comment 4

[Christine Yen]

Attachment: patch v2

Thanks for all the feedback on the previous attachment - a helper function is added now to utils.js, and cleaned up tree.xml.

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 271301 [details] [diff] [review]
patch v2

oops, looks like you re-attached the same patch.

Comment 6

[Christine Yen]

Attachment: patch

should be the right one this time...

Comment 7

[Asaf Romano (gone)]

I don't like the nodeExcludesItems helper, the places API is pretty straight forward here.

Comment 8

[Christine Yen]

Attachment: revision

(In reply to comment #7)
> I don't like the nodeExcludesItems helper, the places API is pretty straight
> forward here.

Takes out the helper, avoids unnecessary check when it should be clear that the node is a folder/container.

Comment 9

[Asaf Romano (gone)]

what is the nodeIsFolder check for?

Comment 10

[Christine Yen]

Attachment: minus isFolder check

(In reply to comment #9)
> what is the nodeIsFolder check for?
> 

Unless it becomes magically possible to paste into a view where the root is not a folder but a query, nothing. I don't think I realized, either, that the root of a treeview is always a folder - I was worried about resRoot.queryOptions throwing an exception.

Comment 11

[Asaf Romano (gone)]

Er? It actually isn't always a folder (e.g. history sidebar), but you could set excludeItems on a query as well...

Comment 12

[Asaf Romano (gone)]

I replied to your comment, the patch looks correct though (except it really isn't valuable to point to specific dialogs in the shared files).

Comment 13

[Christine Yen]

(In reply to comment #11)
> Er? It actually isn't always a folder (e.g. history sidebar), but you could set
> excludeItems on a query as well...
> 

You're right - I said 'magically possible' because of the line in insertionPoint (tree.xml - http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/tree.xml#442) that prevents pasting into the history sidebar.

Comment 15

[(not reading, please use [e-mail] instead)]

just hit this again, ugh.

hoping to drive in christine patch for m8.

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 272515 [details] [diff] [review]
minus isFolder check

re-assign review to dietrich.

this bug has been bit rotting in my review queue for a while.

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> Paste with a folder in the left panel selected, folder either open or closed:
> the item will be pasted to the bottom of the selected (and thus
> displayed-in-the-right-panel) folder.
> 
> Drag onto a folder in the left panel, either open or closed: the item will be
> dropped at the bottom of the target folder.

these WFM in contemporary builds.

> Drag between folders in the left panel, cursor above drop indicator: the item
> will be dropped directly below the first folder (visible in the right panel).
> 
> Drag between folders in the left panel, cursor below drop indicator: the item
> will be dropped directly above the second folder (visible in the right panel).
> 

these, i do not understand the steps. dragging an a folder in the left panel between other folders in the left panel? first and second folders where? on mac, i cannot get the cursor to be anywhere other than directly over the drop indicator line.

resolving WFM. please re-open if you can reproduce any of these problems.

Comment 18

[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