210196 - Fix use of bookmarks root / top root as a target

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Pierre Chanial]

this patch correctly sets the target to the bookmarks root if this one is
selected or if no row are selected.
It also cleanup such as the DND stuff, including a glitch in nsXULTreeBuilder:
the param |before| in canDropBeforeAfter is a boolean.

Comment 1

[Pierre Chanial]

Attachment: patch v1.0

Comment 2

[Pierre Chanial]

setting target to 1.4.

Comment 3

[jag (Peter Annema)]

Comment on attachment 126187 [details] [diff] [review]
patch v1.0

>? xpfe/components/bookmarks/clone.diff
>Index: content/xul/templates/src/nsXULTreeBuilder.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/templates/src/nsXULTreeBuilder.cpp,v
>retrieving revision 1.64
>diff -u -r1.64 nsXULTreeBuilder.cpp
>--- content/xul/templates/src/nsXULTreeBuilder.cpp	13 Jun 2003 20:08:39 -0000	1.64
>+++ content/xul/templates/src/nsXULTreeBuilder.cpp	21 Jun 2003 12:55:25 -0000
>@@ -2083,7 +2083,7 @@
>                 if (orient == nsITreeView::inDropOn)
>                     observer->CanDropOn(row, &canDrop);
>                 else
>-                    observer->CanDropBeforeAfter(row, orient, &canDrop);
>+                    observer->CanDropBeforeAfter(row, orient == nsITreeView::inDropBefore ? PR_TRUE : PR_FALSE, &canDrop);

|a == b ? true : false| is the same as |a == b|, prefer the latter.

>Index: xpfe/components/bookmarks/resources/bookmarks.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/resources/bookmarks.js,v
>retrieving revision 1.120
>diff -u -r1.120 bookmarks.js
>--- xpfe/components/bookmarks/resources/bookmarks.js	8 Jun 2003 16:21:08 -0000	1.120
>+++ xpfe/components/bookmarks/resources/bookmarks.js	21 Jun 2003 12:55:38 -0000
>@@ -862,7 +862,7 @@
>       return item0 != "NC:NewSearchFolder"       && 
>              (type0 == "Folder" || type0 == "PersonalToolbarFolder");
>     case "cmd_bm_movebookmark":
>-      return length > 0 && !aSelection.containsRF;
>+      return length > 0 && aSelection.containsMutable;

[The above is just an example, the question goes for all these code changes]
So what happens when the selection contains an immutable item? Does the action
as a whole get refused (like the old code), or will it get completed for the
mutable items, but not the immutable ones? Is the latter the better behaviour?

Comment 4

[Pierre Chanial]

> |a == b ? true : false| is the same as |a == b|, prefer the latter.

oops, I copied it blindly from
/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 3696
I'll correct both

> [The above is just an example, the question goes for all these code changes]
> So what happens when the selection contains an immutable item? Does the action
> as a whole get refused (like the old code), or will it get completed for the
> mutable items, but not the immutable ones? Is the latter the better behaviour?
the transaction will be performed, discarding the immutable items. I have no
opinion on that. I checked that NS4.x will cancel the transaction if there's an
immutable item. Just tell me if you want to keep this behavior, the fix is
really easy.

Comment 5

[jag (Peter Annema)]

I think I would prefer the current (and apparently NS4) behaviour. Though it's
up to Jan, it's his component. One thing your approach has going for it is that
you'll actually see which items are immutable by the fact that they didn't get
moved. If you know to look for that, that is. Otherwise it may come as a
surprise (maybe much later).

Comment 6

[Jan Varga [:janv]]

>the transaction will be performed, discarding the immutable items.

I thought that we just disable this command if selection contains immutable items.

Comment 7

[Jan Varga [:janv]]

Jag explained to me what's the problem is.
Sure, we should keep the current behaviour.

Comment 8

[jag (Peter Annema)]

Comment on attachment 126187 [details] [diff] [review]
patch v1.0

Okay, Pierre, could you do the patch? Thanks.

Comment 9

[Pierre Chanial]

Attachment: stripped down version

Jan: before this patch, immutable items as defined in checkSelection were
discarded from the selection and their existence did not prevent the whole
transaction from being performed.
The second point is that the root folder was not considered as an immutable
item and was treated as a particular case when there was no reason for it.

Anyways, it was just a cleanup. I'll attach a patch that won't deal with it to
maximize the chance to get it in 1.4.
Clean up will occur in Phoenix.
Please r/sr as quick as possible.

Comment 10

[jag (Peter Annema)]

Comment on attachment 126608 [details] [diff] [review]
stripped down version

sr=jag

Comment 11

[Pierre Chanial]

-> fixed

Comment 12

[Pierre Chanial]

Comment on attachment 126608 [details] [diff] [review]
stripped down version

asking approval for 1.4 branch.
The patch corrects a trivial glitch in the caller of the CanDropBeforeAfter
observers and fixes the hacked second row selection when the bookmark manager
is opened. This patch sets the bookmark root as the target (for paste, new
bookmark etc...) when it is itself selected and when no row is selected.
Risk is low and imo it's a visible bug.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 126608 [details] [diff] [review]
stripped down version

Please check this into 1.4 ASAP.

Comment 14

[Pierre Chanial]

I won't have an internet connection til saturday.
I am leaving in a few hours and won't have time to pull the branch.
Jan, could you take care of this patch, please??
Thanks in advance.

Comment 15

[Jan Varga [:janv]]

Comment on attachment 126608 [details] [diff] [review]
stripped down version

checked into the branch