Last Comment Bug 455590 - Allow new dnd api with tree views
: Allow new dnd api with tree views
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Neil Deakin (away until Oct 3)
:
:
Mentors:
Depends on: 500247 503966
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-16 14:19 PDT by Neil Deakin (away until Oct 3)
Modified: 2010-01-07 22:16 PST (History)
8 users (show)
asqueella: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
pass the dataTransfer to the treeview (28.54 KB, patch)
2009-06-05 02:18 PDT, Neil Deakin (away until Oct 3)
no flags Details | Diff | Splinter Review
testcase (2.53 KB, application/vnd.mozilla.xul+xml)
2009-06-05 02:41 PDT, Neil Deakin (away until Oct 3)
no flags Details
Changed this to use a drag event instead of the data transfer (11.22 KB, patch)
2009-06-09 10:43 PDT, Neil Deakin (away until Oct 3)
neil: review+
Details | Diff | Splinter Review
use datatransfer but move code to nsContentUtils (31.84 KB, patch)
2009-06-18 08:50 PDT, Neil Deakin (away until Oct 3)
neil: review+
bugs: superreview+
Details | Diff | Splinter Review

Description Neil Deakin (away until Oct 3) 2008-09-16 14:19:38 PDT
nsITreeView has two methods, canDrop and drop, used to specify where items may be dropped. These expect you to check the global drag service api. Instead, we should find a way to use the new api. Perhaps we could just pass the event to these methods as well.
Comment 1 Neil Deakin (away until Oct 3) 2009-06-05 02:18:24 PDT
Created attachment 381738 [details] [diff] [review]
pass the dataTransfer to the treeview

This makes it so we can check the data using the new api, and makes direct use of the dragservice fully obsolete.

Can't make an automated test of this, but a manual test should be easy enough.
Comment 2 Neil Deakin (away until Oct 3) 2009-06-05 02:41:58 PDT
Created attachment 381740 [details]
testcase

This test can form a manual test. Just drag the "Drag Me" box onto the tree.
Comment 3 neil@parkwaycc.co.uk 2009-06-05 05:26:42 PDT
Comment on attachment 381738 [details] [diff] [review]
pass the dataTransfer to the treeview

>+  // the dataTransfer field of the event caches the DataTransfer associated
>+  // with the drag. It is initialized when an attempt is made to retrieve it
>+  // rather that when the event is created to avoid duplicating the data when
>+  // no listener ever uses it.

>+// static
>+nsresult
>+nsDOMDragEvent::SetDataTransferInEvent(nsDragEvent* aDragEvent)
This code bears no relation to nsDOMDragEvent, except by way of being one of its callers... if you're going to move it around, why not move it to nsDragEvent itself (or failing that, nsContentUtils)? (That would also make it easier to call from nsTreeBodyFrame.)

>+#include "nsIDOMDataTransfer.h"
You don't actually use the definition, and you already have the declaration.

>+class nsIDOMDataTransfer;
Already declared via nsITreeView.h

>+        NS_ASSERTION(aEvent->eventStructType == NS_DRAG_EVENT, "wrong event type");
>+        nsDragEvent* dragEvent = static_cast<nsDragEvent*>(aEvent);
>+        nsDOMDragEvent::SetDataTransferInEvent(dragEvent);
I guess this counts as a use... seems a shame to create a data transfer just because you drag over a tree that doesn't care. Also, should the return be error-checked? (Maybe not, but you check in the DOMDragEvent code.)
Comment 4 Neil Deakin (away until Oct 3) 2009-06-09 10:43:24 PDT
Created attachment 382335 [details] [diff] [review]
Changed this to use a drag event instead of the data transfer
Comment 5 neil@parkwaycc.co.uk 2009-06-11 12:32:20 PDT
Comment on attachment 382335 [details] [diff] [review]
Changed this to use a drag event instead of the data transfer

This is going to break people building Thunderbird/SeaMonkey against mozilla-central (rather than mozilla-1.9.1); it might be worth writing a patch to #ifndef the extra arguments for the comm-central callers, or at least giving someone else time to write one before you land this.

>+        nsCOMPtr<nsIPrivateDOMEvent> privevent = do_QueryInterface(domEvent);
>+        privevent->SetTrusted(true);
[I don't quite see the point of this, given that the event never gets dispatched.]
Comment 6 Olli Pettay [:smaug] (reviewing overload) 2009-06-11 13:07:54 PDT
Creating artificial event for api calls is strange. It would mean that
for the same nsDragEvent there might be two different nsDOMDragEvents.
I think I prefer the first approach. IMO it makes more sense to use
nsIDOMDataTransfer in the API. Don't move the code to nsDragEvent, but to
nsContentUtils.
Would it be possible to "fill" the data transfer object lazyly? So that dragging
over a tree which doesn't care wouldn't cause a real data transfer to be created.
Though, how expensive is creating a data transfer?
Comment 7 Karl Tomlinson (:karlt) 2009-06-11 13:38:43 PDT
(In reply to comment #6)
> Though, how expensive is creating a data transfer?

Creating a data transfer while dragging from another app reads numDropItems from an nsIDragSession.  With GTK/X11 (don't know about others), this often requires (and due to bug 497496, always involves, even when not required) fetching information from the other app.

It would be nice to avoid waiting on the other app unnecessarily.
(Though maybe that's something that can/should be sorted out in the nsDOMDataTransfer construction?)
Comment 8 Neil Deakin (away until Oct 3) 2009-06-11 13:43:19 PDT
(In reply to comment #7)
> It would be nice to avoid waiting on the other app unnecessarily.
> (Though maybe that's something that can/should be sorted out in the
> nsDOMDataTransfer construction?)

We could change nsDOMDataTransfer to only cache the external data when one of the methods that uses it is called.
Comment 9 Neil Deakin (away until Oct 3) 2009-06-18 08:50:34 PDT
Created attachment 383943 [details] [diff] [review]
use datatransfer but move code to nsContentUtils

Creating a datatransfer can be expensive for external drags, but it only happens once per drag and then the data is cached already.
Comment 10 Olli Pettay [:smaug] (reviewing overload) 2009-06-23 06:22:18 PDT
Comment on attachment 383943 [details] [diff] [review]
use datatransfer but move code to nsContentUtils

>+nsContentUtils::SetDataTransferInEvent(nsDragEvent* aDragEvent)
>+{
>+  if (aDragEvent->dataTransfer)
>+    return NS_OK;
>+
>+  // For draggesture and dragstart events, the data transfer object is
>+  // created before the event fires, so it should already be set. For other
>+  // drag events, get the object from the drag session.
>+  NS_ASSERTION(aDragEvent->message != NS_DRAGDROP_GESTURE &&
>+               aDragEvent->message != NS_DRAGDROP_START,
>+               "draggesture event created without a dataTransfer");

Could you add some check which makes sure that this is called only with
trusted events.
Comment 11 Neil Deakin (away until Oct 3) 2009-06-24 10:14:27 PDT
http://hg.mozilla.org/mozilla-central/rev/5fca16b4c17f
Comment 12 Nickolay_Ponomarev 2009-07-02 02:03:51 PDT
Perhaps worth filing a bug to create a way to test DND automatically?
Comment 13 Sylvain Pasche 2009-07-13 14:46:34 PDT
This caused bug 503966
Comment 14 Eric Shepherd [:sheppy] 2009-07-29 11:04:42 PDT
In which version of Gecko will this ship?
Comment 15 Neil Deakin (away until Oct 3) 2009-07-29 11:09:16 PDT
(In reply to comment #14)
> In which version of Gecko will this ship?

1.9.2

The change is just an extra argument to nsITreeView::canDrop and nsITreeView::drop
Comment 16 Eric Shepherd [:sheppy] 2009-09-02 11:08:41 PDT
This is now documented in:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITreeView

Note You need to log in before you can comment on or make changes to this bug.