326598 - Tree D&D is massively broken

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

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

There are about nine million ways to break D&D in tree views.

Comment 1

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

Attachment: partial work

Comment 2

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

Attachment: partial work

Comment 3

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

Attachment: more work

Comment 4

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

Comment on attachment 211354 [details] [diff] [review]
more work

>Index: browser/components/places/content/controller.js

>+const NHRVO = Ci.nsINavHistoryResultViewObserver;

Convenience/conciseness.

>+const NEWLINE = "\r\n";

Windows text-based applications need line endings to be in this format. I'm not sure if this hurts other platforms. If it does, this should be wrapped in #ifdef XP_WIN. 

>-function ViewConfig(dropTypes, dropOnTypes, excludeItems, expandQueries, firstDropIndex, filterTransactions) {
>+function ViewConfig(dropTypes, dropOnTypes, excludeItems, 
>+                    expandQueries, firstDropIndex, filterTransactions) {

Me fussy.

>-      var parent = nodes[i].parent;
>-      if (parent.childrenReadOnly)
>+      var parent = nodes[i].parent || this._activeView.getResult().root;
>+      if (this.nodeIsReadOnly(parent))

Use common utility function for this. 

>+  _canInsert: function PC__canInsert() {

This determines whether or not items can be inserted into a folder - either by a drag-drop operation, a paste or an action like new folder/separator/etc. 

>+  /**
>+   * Determines whether or not the paste command is enabled, based on the 
>+   * content of the clipboard and the selection within the active view.
>+   */
>+  _canPaste: function PC__canPaste() {
>+    if (this._canInsert()) 
>+      return this._hasClipboardData();
>+    return false;

This implementation becomes simple now it just calls through to _canInsert. 

>   nodeIsFolder: function PC_nodeIsFolder(node) {
>+    ASSERT(node, "null node");

Safety. Looks like I forgot to add ASSERTs to nodeIsQuery/ReadOnly though. Will fix that before checking in. 

>+  nodeIsReadOnly: function PC_nodeIsReadOnly(node) {
>+    if (this.nodeIsFolder(node))
>+      return this._bms.getFolderReadonly(asFolder(node).folderId);
>+    else if (this.nodeIsQuery(node))
>+      return asQuery(node).childrenReadOnly;
>+    return false;
>+  },

A generic helper. 

PC_onCommandUpdate:
>+    var canInsert = this._canInsert();
>-    this._setEnabled("placesCmd_sortby:name", viewIsFolder);
>+    this._setEnabled("placesCmd_sortby:name", viewIsFolder && canInsert);
..etc..

Enable commands that insert items into a folder, or reorder the contents of a folder, only if the folder can be manipulated in such ways. 


>   newFolder: function PC_newFolder() {
>     var view = this._activeView;
>     
>-    view.saveSelection(view.SAVE_SELECTION_REMOVE);
>+    view.saveSelection(view.SAVE_SELECTION_INSERT);

Fix a stupid bug from an earlier checkin.

>   wrapNode: function PC_wrapNode(node, type) {
>     switch (type) {
>     case TYPE_X_MOZ_PLACE_CONTAINER:
>     case TYPE_X_MOZ_PLACE: 
>     case TYPE_X_MOZ_PLACE_SEPARATOR:
>+      // Data is encoded like this:
>+      // bookmarks folder: <folderId>\n<>\n<parentId>\n<indexInParent>
>+      // uri:              0\n<uri>\n<parentId>\n<indexInParent>
>+      // separator:        0\n<>\n<parentId>\n<indexInParent>

Add some documentation...

>       var wrapped = "";
>       if (this.nodeIsFolder(node))
>-        wrapped += asFolder(node).folderId + "\n";
>+        wrapped += asFolder(node).folderId + NEWLINE;

... and use the new NL constant. 

>   unwrapNodes: function PC_unwrapNodes(blob, type) {
>-    var parts = blob.split("\r\n");
>+    // We use \n here because the transferable system converts \r\n to \n
>+    var parts = blob.split("\n");

This actually broke D&D etc when I checked this in yesterday. Turns out the clipboard/drag service converts \r\n to \n so we only need to split on \n when we unwrap. 

>   canDrop: function PCDH_canDrop(view, orientation) {
>-    var result = view.getResult();
>-    if (result.readOnly || !PlacesController.nodeIsFolder(result.root))
>+    var parent = view.getResult().root;
>+    if (PlacesController.nodeIsReadOnly(parent) || 
>+        !PlacesController.nodeIsFolder(parent))

.readOnly was not a valid property... so use the new utility helper instead. D&D was pretty broken as a result of this. 

> /**
>  * Move a Folder
>  */
> function PlacesMoveFolderTransaction(id, oldContainer, oldIndex, newContainer, newIndex) {
>+  ASSERT(!isNaN(id + oldContainer + oldIndex + newContainer + newIndex), "Parameter is NaN!");

You should never be able to move a folder if any of these values === undefined. 

>Index: browser/components/places/content/places.js
>     this._places.init(new ViewConfig([TYPE_X_MOZ_PLACE_CONTAINER],
>                                      ViewConfig.GENERIC_DROP_TYPES,
>-                                     true, false, 3, true));
>+                                     true, false, 4, true));

Now that we have a "Subscriptions" item we need to bump this index to 4. 

>Index: browser/components/places/content/tree.xml

>       <!-- AVI Method -->
>       <property name="insertionPoint">
...
>+          // If an item in the static region is selected, insert the new item 
>+          // before the first drop index. 
>+          var node = this.getResult().nodeForTreeIndex(max.value);
>+          var container = asContainer(this.getResult().root);
>+          var cc = container.childCount;
>+          for (var i = 0; i < cc; ++i) {
>+            if (container.getChild(i) == node && 
>+                i < this.firstDropIndex) {
>+              max.value = this.firstDropIndex;
>+              orientation = NHRVO.DROP_BEFORE;
>+              break;
>+            }
>+          } 
>+          return this._getInsertionPoint(max.value, orientation);

e.g. if "History" is selected and the user clicks "New Folder", we need to insert the new folder right after "Subscriptions" - or _before_ the first drop index. 

>-          var folder = container.QueryInterface(Ci.nsINavHistoryFolderResultNode);
>-          return new InsertionPoint(folder.folderId, index, orientation);
>+          return new InsertionPoint(asFolder(container).folderId, index, orientation);

Unnecessary QI.

>       <!-- nsDragAndDrop -->
>       <method name="onDragStart">
>         <parameter name="event"/>
>         <parameter name="xferData"/>
>         <parameter name="dragAction"/>
>         <body><![CDATA[ 
>+          // Drag and Drop does not work while a tree view is sorted.
>           if (this.getAttribute("sortActive") == "true")
>-            throw Components.results.NS_OK;
>+            throw Cr.NS_OK;
>+          
>+          // Items that are "static" - i.e. above the user-configurable area
>+          // of the view - can not be moved. 
>+          var nodes = this.getSelectionNodes();
>+          var bms = 
>+              Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+              getService(Ci.nsINavBookmarksService);
>+          for (var i = 0; i < nodes.length; ++i) {
>+            var node = nodes[i];
>+            
>+            // If this node is part of a readonly folder (e.g. a livemark) it 
>+            // cannot be moved, only copied, so we must change the action used
>+            // by the drag session.
>+            var parent = node.parent;
>+            if (PlacesController.nodeIsFolder(parent) &&
>+                bms.getFolderReadonly(asFolder(parent).folderId)) 
>+              dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
>+
>+            // "static" top level "special" items cannot be dragged. They are 
>+            // at the root level of the view and are at a lower index in the 
>+            // root container than user-configurable items. 
>+            var treeIndex = this.getResult().treeIndexForNode(nodes[i]);
>+            if (this.view.getLevel(treeIndex) == 0) {
>+              var index = PlacesController.getIndexOfNode(nodes[i]);
>+              if (index < this.firstDropIndex)
>+                throw Cr.NS_OK;
>+            }
>+          }

Make sure the user cannot drag any items whose index within the root container is less than the "first drop index" - i.e. items in the static-"special" region. We cancel the drag by throwing NS_OK. 

>       <!-- nsDragAndDrop -->
>       <method name="onDragOver">
>         <parameter name="event"/>
>         <parameter name="flavor"/>
>         <parameter name="session"/>
>-        <body><![CDATA[ ]]></body>
>+        <body><![CDATA[ 
>+          // When the user is dragging over an empty area of the tree, make 
>+          // sure canDrop is set to true to indicate dropping into the bucket.
>+          var row = { }, col = { }, child = { };
>+          this.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, 
>+                                       child);
>+          if (row.value == -1) {
>+            var dragService = 
>+                Cc["@mozilla.org/widget/dragservice;1"].
>+                getService(Ci.nsIDragService);
>+            var session = dragService.getCurrentSession();
>+            session.canDrop = this._viewObserver.canDrop(-1, 1);
>+          }
>+        ]]></body>

For some reason, the tree observer's |canDrop| method is not called when the user is dragging over a region of the tree that does not contain rows - i.e. the empty region at the bottom. So we use the XUL widget's dragOver method to detect to see if the user is dragging over this region, then call the tree observer's canDrop method manually and tell the session about the result. 

>       <method name="_getInsertionIndex">
...
>           var node = this._getInsertionNode(insertionPoint, excludeItems);
>           // This is the insertion index of the pivot. 
>-          return this.getResult().treeIndexForNode(node);
>+          if (node)
>+            return this.getResult().treeIndexForNode(node);
>+          else if (insertionPoint.orientation == NHRVO.DROP_ON)
>+            return Ci.nsINavHistoryResult.INDEX_INVISIBLE;
>+          return -1;

Better handling. Sometimes the item being dropped can effectively "vanish" from the current view - into a subfolder that is closed, so it is now "invisible". 


>         canDrop: function VO_canDrop(index, orientation) {
>-          var root = this._self.getResult();
>-          var node = index != -1 ? root.nodeForTreeIndex(index) : root;
>-          const DROP_ON = Ci.nsINavHistoryResultViewObserver.DROP_ON;
>+          var result = this._self.getResult();
>+          var node = index != -1 ? result.nodeForTreeIndex(index) : result.root;
>           // Cannot drop before fixed items in the list. 
>-          if (node.parent == root && 
>+          if (node.parent == result.root && 

This was also pretty badly broken. node.parent != a nsINavHistoryResult, it's a nsINavHistoryResultNode - i.e. use .root. 

>-          // Cannot drop into a read-only container
>-          if (orientation == DROP_ON) {
>-            if (node.containerReadOnly) {
>+          if (orientation == NHRVO.DROP_ON) {
>+            // The user cannot drop an item into itself or a read-only container
>+            var droppingOnSelf = 
>+                this._getSourceView() == this._self &&
>+                this._self.view.selection.isSelected(index);
>+            if (droppingOnSelf || node.containerReadOnly)
>               return false;

You can't drop a folder into itself - that doesn't make sense. 

>+        /**
>+         * Adjusts an InsertionPoint's insertion index using these rules:
>+         *  XXXben define rules
>+         */
>         _adjustInsertionPoint: function VO__adjustInsertionPoint(insertionPoint) {
>           var index = this._self._getInsertionIndex(insertionPoint);
>+          
>+          // If the dropped items are invisible, we don't need to adjust the 
>+          // insertion point. 
>+          if (index == Ci.nsINavHistoryResult.INDEX_INVISIBLE)
>+            return;

No adjustment is needed if the item is being dropped into a closed container. I think there might be more conditions here, but all seems to be well so far. 

>+        /**
>+         *
>+         */
>+        _getSourceView: function VO__getSourceView() {
>+          var session = this._getCurrentSession();
>+          var sourceView = session.sourceNode.wrappedJSObject;
>+          while (sourceView && sourceView.localName != "tree")
>+            sourceView = sourceView.parentNode;
>+          return sourceView;
>+        },

This gets the view where the drag initiated. I have added a comment as such in my source. 

>+        
>+        /**
>+         *
>+         */
>+        _getCurrentSession: function VO__getCurrentSession() {
>+          var dragService = 
>+              Cc["@mozilla.org/widget/dragservice;1"].
>+              getService(Ci.nsIDragService);
>+          return dragService.getCurrentSession();
>+        },

Current drag session. Comment added in my source.

Comment 5

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

Attachment: patch

this is for checkin

Comment 6

[Peter van der Woude [:Peter6]]

this was only checked in on Branch

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Will this patch fix cutting and pasting too? Right now, that doesn't work either (and has caused my browser to crash once).
TB15095194Q

Comment 8

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

I know I was waiting for the red to clear on the trunk. It is now landed on the trunk. FIXED. 

Comment 9

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv