64722 - DND library needs cleanup & support for dragging multiple items

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

patch coming soon...

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: diff for new nsTransferable impl & updated d&d library. (call site patches to follow)

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

The previous patch modifies the implementation of nsTransferable considerably, 
and updates the clients nsDragAndDrop and nsClipboard which use it. I'll post a 
description of my changes here shortly, along with updated call sites in the 
app. 

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: navigator call site patches

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: content area D&D call site patch

Comment 5

[Blake Ross]

coolness.  let me know if you need some review.  cc'ing alec

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

need to fix up messengercompose. more to come...

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: messenger compose attachment bucket d&d call site

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: for the interested reader, nsDragAndDrop.js

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: for the interested reader, nsTransferable.js

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

A brief explanation. I've discussed the implementation of this with jag on irc 
over the past month, and built the code here on feedback from him, and others 
who have used this library. In the past this library has managed dragged data by 
using simple js objects called flavourData. The name was overloaded however - 
there were two different types of these objects used in two different 
situations, and the name was not descriptive. This update attempts to address 
these issues by creating a series of simple types which provide clearer 
descriptions of what is going on in the code, and also expand the drag and drop 
library to offer support for the dragging of multiple items. (This was always 
possible through the direct API, just never implemented in this library). The 
simple types are as follows:

Flavour: a simple type representing a data flavour. The fields are its content 
type (mime type) and a string key that is used to look up its IID.

FlavourSet: a collection of Flavours

FlavourData: data for a particular flavour

TransferData: all the data for all the flavours for a particular draggable item.

TransferDataSet: all the data for all the flavours associated with multiple 
draggable items.

(detailed comments in the source)

The client wishing to use the drag and drop library must implement a drag and 
drop observer for each node that they wish to be a drag source/target. This node 
implements methods that will be called on drag initiation, drag over, drop, etc, 
as well as methods that tell the drag and drop library what kinds of data that 
it can respond to. This is mostly unchanged from previous versions, with the 
exception of a few parameters and the integration of the new types. 

Comment 11

[Fabian Guisset]

coolio this would fix bug 42080. Other related bugs to drag and drop in 
bookmarks are bug 42116 bug 43753 bug 49894 bug 53403.

Comment 12

[Peter "jag" Annema]

nsDragAndDrop.js:

Why not encapsule

|      if (!this.mDragSession)
|        this.mDragSession = this.mDragService.getCurrentSession();

the way you've encapsuled mDragService?

nsTransferable.js:

Looks good :-) r=jag

Comment 13

[Peter "jag" Annema]

navigator call site patches:

Index: navigatorDD.js

would you mind changing
  if (!foo) return
to
  if (!foo)
    return;
?

You left a dump in...
 var proxyIconDNDObserver = {
...
+      var urlString = urlBar.value + "\n" + window._content.document.title;

This has the risk of a user typing a new url, then dragging that, meaning you
get a wrong title. I think however most users, after being bitten once perhaps,
will understand what's going on. We could prevent this by setting an attribute
on the urlbar (titlematches="true" ???) and remove that oninput. Worth the
trouble?

Index: navigator.xul


@@ -263,7 +265,7 @@
             <menupopup>
               <menuitem class="menuitem-iconic bookmarkitem" uri="rdf:*"
                 value="rdf:http://home.netscape.com/NC-rdf#Name"
-                oncommand="OpenBookmarkURL(event.target,
document.getElementById('innermostBox').database)"/>
+                        oncommand="OpenBookmarkURL(event,
document.getElementById('innermostBox').database)"/>
             </menupopup>
           </rule>
         </template>

Can you check if that oncommand is "off by one space" for you?

Next comment, next patch :-)

Comment 14

[Peter "jag" Annema]

Index: contentAreaDD.js

+function retrieveURLFromData (aData, flavour)
 {
-  switch (aData.flavour) {
+  switch (flavour) {
   case "text/unicode":
     // this might not be a url, but we'll return it anyway
-    return aData.data.data.toString();
-    break;
+    return aData;
   case "text/x-moz-url":
-    var data = aData.data.data.toString();
+    var data = aData.toString();

Isn't this .toString() unnecessary now that you automatically convert all
nsISupportsWStrings to js strings?

Looks okay otherwise.

Next comment, next patch.

Comment 15

[Peter "jag" Annema]

Index: MsgComposeCommands.js

r=jag

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

A text search for dump();s doesn't show any in the navigatorDD patch. I'll
search the code before checking in.

I'd prefer to keep my if (short_condition) returns ;)

You're right on the toString() thing, I think. I'll try without it. 

Comment 17

[David Hyatt]

Fix checked in. 

Comment 18

[Terri Preston]

Old bug, no comments in a long time, marking verified