There's the nsIOutlinerBuilderObserver now. I think it could be just listener.

per 176370

I just want to save this patch here (not ready for landing on the trunk yet).

That's cool, but the interface should probably also expose performAction[onCell|onRow..] etc, since those are controller-like methods too. I stand by nsITreeController as an interface name ;-)

We can remove these methods from nsITreeView later. It will be also possible to get rid of nsITreeBuilderObserver.

Comment on attachment 204433 [details] [diff] [review] updated patch >+ attribute nsITreeController controller; >+ There's already an attribute called controllers for XUL elements, unrelated to this controller. Perhaps treeController would be better. > nsITreeContentView.idl \ >+ nsITreeController.idl \ Missing spaces? >+ /** >+ * Methods used by the drag feedback code to determine if a drag is allowable at >+ * the current location. To get the behavior where drops are only allowed on >+ * items, such as the mailNews folder pane, always return false when >+ * the orientation is not DROP_ON. >+ */ >+ boolean canDropOnLeaves(); >+ >+ boolean canDrop(in long row, in long orientation); Can you add a comment to both methods please? Looks like the comment above applies to canDrop. Also, add a bit that says "Return true if a drop is allowed on a particular row." > PRBool isContainer = PR_FALSE; > mView->IsContainer (*aRow, &isContainer); >- if (isContainer) { >+ if (isContainer || canDropOnLeaves) { Move the IsContainer call so that it doesn't get called if canDropOnLeaves is true

Comment on attachment 204480 [details] [diff] [review] patch v0.1 Looks OK. You probably need to get a review from a toolkit person though.

Comment on attachment 204480 [details] [diff] [review] patch v0.1 Some thoughts: Instead of using canDropOnLeaves(), if the drop position is in the 25%-75% zone, and canDrop(row, DROP_ON) returns FALSE, then try a 50%/50% split? Is it worthwhile making nsITreeView inherit from nsITreeController? Then if there's no controller you just use the view instead rather than all those if/else tests. The downside, if you didn't combine this with my first suggestion, is that it would require adding canDropOnLeaves to each view. Is it not better to store the controller on the element or the box object, like the view?

(In reply to comment #9) > (From update of attachment 204480 [details] [diff] [review] [edit]) > Some thoughts: > > Instead of using canDropOnLeaves(), if the drop position is in the 25%-75% > zone, and canDrop(row, DROP_ON) returns FALSE, then try a 50%/50% split? Sure it would be technically better, but I'm afraid I would have to check and fix all trees in the repository to return false if the item is a leaf. We would also break backward compatibility. > > Is it worthwhile making nsITreeView inherit from nsITreeController? Then if > there's no controller you just use the view instead rather than all those > if/else tests. The downside, if you didn't combine this with my first > suggestion, is that it would require adding canDropOnLeaves to each view. I'd like to remove all those redundant methods from nsITreeView later. > > Is it not better to store the controller on the element or the box object, like > the view? > It's actually stored on the element, isn't it? See also bug 317904 thanks for the initial sr

Well the controller could be owned by the view.

(In reply to comment #10) >>Instead of using canDropOnLeaves(), if the drop position is in the 25%-75% >>zone, and canDrop(row, DROP_ON) returns FALSE, then try a 50%/50% split? >Sure it would be technically better, but I'm afraid I would have to check and >fix all trees in the repository to return false if the item is a leaf. We would >also break backward compatibility. Since you want to remove the methods from nsITreeView later it doesn't make much difference, does it? As far as I can tell, the only view that really cares is the XUL tree builder, nobody else bothers with drag and drop. Combined with the approach below you could immediately remove the redundant nsITreeView methods. (In reply to comment #11) >Well the controller could be owned by the view. Actually I was thinking that the box/frame could own the view, and the XUL tree builder would implement nsITreeController and either EnsureView or SetView would QI it (but probably only if the tree didn't already have a controller).

(In reply to comment #12) >(In reply to comment #11) >>Well the controller could be owned by the view. >Actually I was thinking that the box/frame could own the view I mean controller here obviously :-[

I didn't want to own the controller in tree body frame. The view is owned by the frame, but there are additional hacks to persist it (box->Get/SetPropertyAsSupports) when the frame doesn't exist or it goes away. Regarding redundant nsITreeView methods, I'd like to remove them in next step, once this lands on the trunk.

I think my problem is that I'm not happy with you separating and enhancing the tree controller in one patch. If you can't live without canDropOnLeaves before you kill drop/action processing from views, then how about this approach: <field name="treeController"> // default to forwarding (almost) everything to the view ({ tree: this, canDropOnLeaves: function() { return false; }, canDrop: function(row, orientation) { return this.tree.view.canDrop(row, orientation); }, drop: function(row, orientation) { return this.tree.view.drop(row, orientation); }, performAction: function(action) { return this.tree.view.performAction(action); }, performActionOnRow: function(action, row) { return this.tree.view.performActionOnRow(action, row); }, performActionOnCell: function(action, row, col) { return this.tree.view.performActionOnCell(action, row, col); } }) </field>

Ok, I'll remove those methods from nsITreeView, would you give me r+ then ?

Assuming you'd update the existing drag-n-drop consumers too, then yes. As XPConnect will automatically create the C++ getter and setter for the field, is there any reason you use a custom treeController getter and setter for your field, rather than naming your field treeController in the first place?

Comment on attachment 225155 [details] [diff] [review] patch v0.2 forgot to include browser/

Comment on attachment 225155 [details] [diff] [review] patch v0.2 A couple of points: Several extensions, particularly irc, will get upset if you remove support for older browsers. I assume that the reason the folder pane and addressbook tree don't accept dropping on leaves is that they don't report any of their rows as being leaves?

(In reply to comment #20) >I assume that the reason the folder pane and addressbook tree don't accept >dropping on leaves is that they don't report any of their rows as being leaves? [If you allow dropping on leaves it saves querying for containership]

I don't think anyone is going to work on this given the plan to remove the XUL "tree" widget (bug 1446335).