232619 - can't paste a bookmark in correct location

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Boying Lu]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; zh-CN; rv:1.4.1) Gecko/20040102

The bookmark you added appearred in the same folder (level) with personal
toolbar folder, while not in personal toolbar folder

You can select any folder not just "Personal toolbar folder"

This bug can reproduced in mozilla1.6



Reproducible: Always
Steps to Reproduce:
 Open the Manage Bookmarks Window.
1. start mozilla
2. open "Bookmark manager" window from the menu "Bookmarks" --> "Manage
Bookmarks..." 
3. Edit|Cut or Edit|Copy any bookmark.
4. Select the 'Personal Toolbar Folder'.
5. Select Edit|Paste.

or
1. Select Bookmarks|Manage Bookmarks.
2. Select personal toolbar folder.
3. Select File|New Folder. Click 'OK in the dialog that appears.



Actual Results:  
The bookmark you added appearred in the same folder (level) with personal
toolbar folder, while not in personal toolbar folder

Expected Results:  
The pasted bookmark should under the "Personal Toolbar Folder" folder.

Comment 1

[Boying Lu]

Attachment: This patch is made under mozilla1.4

Comment 2

[Hermann Schwab]

I didn´t check it, but wouldn´t this be a nice improvement on 1.4.2?

Comment 3

[Hermann Schwab]

confirming bug on Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040130

Comment 4

[Boying Lu]

How about this pacth. Are it correct and can be checkin the trunk?

Comment 5

[Kyle Yuan]

Boying, you better to post a new patch against the trunk and have Neil review it.

Comment 6

[Boying Lu]

Attachment: This patch is made against the trunk

Comment 7

[timeless]

Comment on attachment 140716 [details] [diff] [review]
This patch is made against the trunk

>Index: bookmarksTree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/resources/bookmarksTree.xml,v
>retrieving revision 1.10
>diff -u -r1.10 bookmarksTree.xml
>--- bookmarksTree.xml	15 Jan 2004 02:57:15 -0000	1.10
>+++ bookmarksTree.xml	6 Feb 2004 04:53:53 -0000
>@@ -524,6 +524,18 @@
>               // this model is left to |preUpdateTreeSelection|, called when
>               // an insert transaction is executed, and |updateTreeSelection|
>               // called here. 
>+
>+              if (aCommand == "cmd_bm_newfolder"
>+                   || aCommand == "cmd_bm_newbookmark"
>+                   || aCommand == "cmd_bm_paste") {
1.		^ should align w/ 5.
2. we tend to use trailing ops. 3. you should try to align things... (aCommand
w/ aCommand w/ aCommand)
>+                if (selection.isContainer[0])
>+                  target = this.mOuter.getTreeTarget(selection.item[0],
>+                          selection.parent[0], BookmarksUtils.DROP_ON);
>+               }
5.		 v should align w/ 1.
>+               else if (aCommand == "cmd_bm_newseparator")
4. please explain why you're special casing new sep.
>+                 target = this.mOuter.getTreeTarget(selection.item[0],
>+                          selection.parent[0], BookmarksUtils.DROP_AFTER);
>+                                                                                
>               BookmarksController.doCommand(aCommand, selection, target);
>               this.mOuter.updateTreeSelection();
>               break;

Comment 8

[chris hofmann]

neil, can you review?

Comment 9

[Pierre Chanial]

for the 1.4 branch, I think it's safe to check that in, if Neil agrees with the
new behaviour.

But not in the trunk. The target should be determined in
BookmarksMenu.getBTTarget().

Comment 10

[[e-mail]]

Comment on attachment 140716 [details] [diff] [review]
This patch is made against the trunk

I think the right fix is to make onSelectionChanged smarter so that its target
is computed as a drop on for folders but still a drop before for items.

Comment 11

[Pierre Chanial]

> The target should be determined in BookmarksMenu.getBTTarget().
err... for trees, the target should be determined from the method getTreeTarget

Comment 12

[Boying Lu]

Attachment: I align some codes

According to netscape 7.0, a separator is always put after the selected
bookmark (or folder). So the following codes:
 else if (aCommand == "cmd_bm_newseparator")
		 target = this.mOuter.getTreeTarget(selection.item[0],
			  selection.parent[0], BookmarksUtils.DROP_AFTER);
are used to handle this case.

I think this maybe right to put  the codes  in	      
onSelectionChanged() but     I don't know  which  command
is to be executed  in that function.

Comment 13

[chris hofmann]

it would be good to get this for 1.7b

Comment 14

[Boying Lu]

Comment on attachment 141508 [details] [diff] [review]
I align some codes

Neil,
  Can you give r?

Comment 15

[[e-mail]]

Attachment: Alternate idea

This makes it so that new items are appended to an open container.

Comment 16

[Jan Varga [:janv]]

Comment on attachment 142898 [details] [diff] [review]
Alternate idea

I think this patch is good and simple solution in the current situation.
r=varga

Comment 17

[Alec Flett]

Comment on attachment 142898 [details] [diff] [review]
Alternate idea

I really hate huge ? : statements - I think they're confusing and hard to
maintain (i.e. error prone)

is there any way we can split this into an if/else for readability, or at least
break out the ?: into a temporary?

sr=alecf on the logic..

Comment 18

[[e-mail]]

Checked in the 4.x compatilbility fix - now paste, new will not insert at the
same level as an open container (note that there is still an issue that 4.x used
to drop/paste/insert at the start of the container, whereas Mozilla appends).

Comment 19

[Boying Lu]

I have a question about the "Alternate idea" patch: 
 In netscape 4.x, when I paste a "separator" on a folder, the separator will be
pasted after that folder. But after I add this patch, the "separator" will be
pasted in the folder. i.e. the "separator" becomes a subitem of the folder.
  Is that right?
 
BTW my patch("I align some codes") considers this case.

Comment 20

[[e-mail]]

I'm using Netscape Communicator 4.79 [en] (Win95; U)

If I highlight a closed folder, the bookmark/separator gets pasted/created after
the folder.

If I highlight an open folder then it goes in the folder.

The only difference is that Mozilla appends it to the folder whereas
Communicator inserts it as the first child of the folder.