Improve API and documentation of tree models.

Converts TreeNodeModel to allow an index-based removal for cases where
an index is known. This prevents a linear search in that case. A
pass-through function is provided that does the search if the index is
not known.

This requires a similar change in TreeNode. TreeNode::Add and ::Remove
used to be virtual but these were never overridden. They are changed
to non-virtual member functions.

Additional documentation is added referencing TreeNodeModel from TreeModel, and
some additional examples are provided.

Change-Id: I5c062420bada437f1ef127ce80518e37f5c2aaae
Reviewed-on: https://chromium-review.googlesource.com/820671
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523591}

ui/base/models/tree_model.h

@@ -20,7 +20,10 @@ class TreeModel;
 
 // TreeModelNode --------------------------------------------------------------
 
-// Type of class returned from the model.
+// Type of class returned from the model. This is a low-level interface.
+// Generally you will want to use TreeNode or TreeNodeWithValue which provides
+// a basic implementation for storing the tree hierarchy. See
+// tree_node_model.h.
 class TreeModelNode {
  public:
   // Returns the title for the node.
@@ -54,7 +57,10 @@ class UI_BASE_EXPORT TreeModelObserver {
 
 // TreeModel ------------------------------------------------------------------
 
-// The model for TreeView.
+// The model for TreeView. This is a low-level interface and requires a lot
+// of bookkeeping for the tree to be implemented. Generally you will want to
+// use TreeNodeModel which provides a standard implementation for basic
+// hierarchy and observer notification. See tree_node_model.h.
 class UI_BASE_EXPORT TreeModel {
  public:
   // Returns the root of the tree. This may or may not be shown in the tree,

ui/base/models/tree_node_model.h

@@ -63,6 +63,12 @@ namespace ui {
 
 // TreeNode -------------------------------------------------------------------
 
+// See above for documentation. Example:
+//
+//   class MyNode : public ui::TreeNode<MyNode> {
+//     ...<custom class logic>...
+//   };
+//   using MyModel = ui::TreeNodeModel<MyNode>;
 template <class NodeType>
 class TreeNode : public TreeModelNode {
  public:
@@ -75,7 +81,7 @@ class TreeNode : public TreeModelNode {
 
   // Adds |node| as a child of this node, at |index|. Returns a raw pointer to
   // the node.
-  virtual NodeType* Add(std::unique_ptr<NodeType> node, int index) {
+  NodeType* Add(std::unique_ptr<NodeType> node, int index) {
     DCHECK(node);
     DCHECK_GE(index, 0);
     DCHECK_LE(index, child_count());
@@ -86,17 +92,24 @@ class TreeNode : public TreeModelNode {
     return node_ptr;
   }
 
-  // Removes |node| from this node and returns it.
-  virtual std::unique_ptr<NodeType> Remove(NodeType* node) {
+  // Removes the node at the given index. Returns the removed node.
+  std::unique_ptr<NodeType> Remove(int index) {
+    DCHECK(index >= 0 && index < child_count());
+    children_[index]->parent_ = nullptr;
+    std::unique_ptr<NodeType> ptr = std::move(children_[index]);
+    children_.erase(children_.begin() + index);
+    return ptr;
+  }
+
+  // Removes the given node. Prefer to remove by index if you know it to avoid
+  // the search for the node to remove.
+  std::unique_ptr<NodeType> Remove(NodeType* node) {
     auto i = std::find_if(children_.begin(), children_.end(),
                           [node](const std::unique_ptr<NodeType>& ptr) {
                             return ptr.get() == node;
                           });
     DCHECK(i != children_.end());
-    node->parent_ = nullptr;
-    std::unique_ptr<NodeType> ptr = std::move(*i);
-    children_.erase(i);
-    return ptr;
+    return Remove(i - children_.begin());
   }
 
   // Removes all the children from this node.
@@ -179,6 +192,10 @@ class TreeNode : public TreeModelNode {
 
 // TreeNodeWithValue ----------------------------------------------------------
 
+// See top of file for documentation. Example:
+//
+//  using MyNode = ui::TreeNodeWithValue<MyData>;
+//  using MyModel = ui::TreeNodeModel<MyNode>;
 template <class ValueType>
 class TreeNodeWithValue : public TreeNode<TreeNodeWithValue<ValueType>> {
  public:
@@ -220,14 +237,18 @@ class TreeNodeModel : public TreeModel {
     return node_ptr;
   }
 
-  std::unique_ptr<NodeType> Remove(NodeType* parent, NodeType* node) {
+  std::unique_ptr<NodeType> Remove(NodeType* parent, int index) {
     DCHECK(parent);
-    int index = parent->GetIndexOf(node);
-    std::unique_ptr<NodeType> owned_node = parent->Remove(node);
+    std::unique_ptr<NodeType> owned_node = parent->Remove(index);
     NotifyObserverTreeNodesRemoved(parent, index, 1);
     return owned_node;
   }
 
+  std::unique_ptr<NodeType> Remove(NodeType* parent, NodeType* node) {
+    DCHECK(parent);
+    return Remove(parent, parent->GetIndexOf(node));
+  }
+
   void NotifyObserverTreeNodesAdded(NodeType* parent, int start, int count) {
     for (TreeModelObserver& observer : observer_list_)
       observer.TreeNodesAdded(this, parent, start, count);
@@ -244,6 +265,15 @@ class TreeNodeModel : public TreeModel {
   }
 
   // TreeModel:
+
+  // C++ allows one to override a base class' virtual function with one that
+  // returns a different type, as long as that type implements the base class'
+  // return type. This is convenient because it allows callers with references
+  // to the specific TreeNodeModel to get the proper return type without
+  // casting.
+  //
+  // However, this does require that the NodeType be defined when this is
+  // parsed (normally one could forward define this).
   NodeType* GetRoot() override {
     return root_.get();
   }

ui/views/controls/tree/tree_view.cc

@@ -461,7 +461,7 @@ void TreeView::TreeNodesRemoved(TreeModel* model,
     InternalNode* child_removing = parent_node->GetChild(start);
     if (selected_node_ && selected_node_->HasAncestor(child_removing))
       reset_selection = true;
-    parent_node->Remove(child_removing);
+    parent_node->Remove(start);
   }
   if (reset_selection) {
     // selected_node_ is no longer valid (at the time we enter this function