Whilst trying to implement some new Drag & Drop functionality in address book, I found some problems with the drag and drop code in trees. Currently, when you drag an address book item over an address book, as the mouse reaches the address book then canDrop is queried with the current action state. However if you then keep the mouse still and change the drag action by pressing a modifier key canDrop isn't re-queried. I need this as in some cases within address book a copy should be allowed, but a move isn't. With the functionality as it is, it will cope, however the graphics aren't updated appropriately to inform the user as to what is actually happening. The problem is that in nsTreeBodyFrame::HandleEvent (http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2016) it filters out changes where the mouse hasn't moved, but doesn't take into account the drag action. I have a patch in progress for this and will attach it later.

This patch adds a value to Slots so that the drag action of the last call to HandleEvent is tracked and then is used in the comparison for changes to see if just the drag action has changed. If you wish to test it, at the moment I think the best way is find a drag & drop case for a tree where you can insert some text output into canDrop and then start to involke a drag to the point where canDrop is queried. Keep the mouse where it is and use the modifier keys to change the drag action - you should see the text output coming out as well.

I'm not qualified to review this code, at least if you want a review sometime this month; I'd try someone like Neil, perhaps...

Comment on attachment 220525 [details] [diff] [review] Keep track of the last drag action and add it to the list of checks for changes (In reply to comment #2) > I'm not qualified to review this code, at least if you want a review sometime > this month; I'd try someone like Neil, perhaps... > Thanks for letting me know. Trying Neil...

Comment on attachment 220525 [details] [diff] [review] Keep track of the last drag action and add it to the list of checks for changes >+ * Mark Banner <mark@stnadard8.demon.co.uk> :-P > nsCOMPtr<nsIDragSession> dragSession; Hey, is this line unused? ;-) > PRInt16 lastDropOrient = mSlots->mDropOrient; > PRInt16 lastScrollLines = mSlots->mScrollLines; > >+ // Find out the current drag action >+ PRUint32 currentDragAction = mSlots->mLastDragAction; >+ if (mSlots->mDragSession) >+ mSlots->mDragSession->GetDragAction(&currentDragAction); I think you're going about this the wrong way around. As the two lines of context demonstrate, your temporary variable should be the last value, and the Slots member should be named mDragAction. > PRInt16 mDropOrient; > >+ // The last drag action that was received for this slot >+ PRUint32 mLastDragAction; >+ > // Number of lines to be scrolled. > PRInt16 mScrollLines; Putting a 32 between two 16s is not good. Here's the layout on a PPC: NN NN NN NN mDropRow NN NN XX XX mDropOrient NN NN NN NN mDragAction NN NN XX XX mScrollLines NN NN NN NN mDragSession As you can see, you just required 8 extra bytes instead of 4. Try this: NN NN NN NN mDropRow NN NN mDropOrient NN NN mScrollLines NN NN NN NN mDragAction NN NN NN NN mDragSession

Updated patch to address Neil's comments.

Comment on attachment 220685 [details] [diff] [review] Keep track of the last drag action and add it to the list of checks for changes v2 Looks good now.

Patch checked into trunk -> fixed.

Comment on attachment 220685 [details] [diff] [review] Keep track of the last drag action and add it to the list of checks for changes v2 Requesting branch approval for this patch. I'd like to get it in as it provides the required functionality for the first address book drag and drop patch on bug 35837. Its been on trunk for about a week now and no one's raised any regressions as far as I can see. I'm hoping to get both patches on that bug in for Thunderbird 2.0/SeaMonkey 1.1 but this bug will block either of them going in.

Fix checked into branch.