Merge lp:~victored/granite/source-list-tweaks into lp:~elementary-pantheon/granite/granite

[Victor Martinez]

[SourceList] [API] Allow every ExpandableItem to define its own sorting method.

The previous global sorting API has been deprecated but it's still supported, so sorting is done on a per-expandable-item basis whenever the global sort function is set to "null".

Fixes lp:1188694 and lp:1182400.

[SourceList] [API] Allow every ExpandableItem to define its own sorting method.

The previous global sorting API has been deprecated but it's still supported, so sorting is done on a per-expandable-item basis whenever the global sort function is set to "null".

Fixes lp:1188694 and lp:1182400.

[Julián Unrrein (junrrein)]

Commit 600 looks good to me code-wise, and I confirm it fixes bug #1182400 using Julien's example in the bug report.

[Julián Unrrein (junrrein)]

Commit 601 looks good to me too.

[Julián Unrrein (junrrein)]

Commit 602 looks good to me code-wise, but you'd better ask Julien for both a test case and to confirm this API works well for him.

[Victor Martinez (victored)]

Thank you for the review Julian.

If you have any objection or suggestion for the API proposed here, please let me know before it gets merged.

=== modified file 'demo/GraniteDemo.vala'
--- demo/GraniteDemo.vala	2013-09-16 23:14:20 +0000
+++ demo/GraniteDemo.vala	2013-11-21 05:06:14 +0000
@@ -51,6 +51,18 @@
         }
     }
 
+    private class SourceListExpandableItem : Granite.Widgets.SourceList.ExpandableItem {
+        public SourceListExpandableItem (string name) {
+            base (name);
+        }
+
+        public override int compare (Granite.Widgets.SourceList.Item a,
+                                     Granite.Widgets.SourceList.Item b)
+        {
+            return strcmp (a.name, b.name);
+        }
+    }
+
     /**
      * SourceList item. It stores the number of the corresponding page in the notebook widget.
      */
@@ -134,8 +146,8 @@
         });
 
         // Main sidebar categories
-        var widgets_category = new Granite.Widgets.SourceList.ExpandableItem ("Widgets");
-        var services_category = new Granite.Widgets.SourceList.ExpandableItem ("Services");
+        var widgets_category = new SourceListExpandableItem ("Widgets");
+        var services_category = new SourceListExpandableItem ("Services");
 
         // Add and expand categories
         sidebar.root.add (widgets_category);

=== modified file 'lib/Widgets/SourceList.vala'
--- lib/Widgets/SourceList.vala	2013-08-16 22:19:48 +0000
+++ lib/Widgets/SourceList.vala	2013-11-21 05:06:14 +0000
@@ -139,7 +139,6 @@
      *   - icon          | DataModel::on_item_prop_changed   | Tree::icon_cell_data_func
      *   - activatable   | Same as @icon                     | Same as @icon
      * + ExpandableItem  |                                   |
-     *   - no_caption    | DataModel::on_item_prop_changed   | Tree::name_cell_data_func
      *   - collapsible   | DataModel::on_item_prop_changed   | Tree::update_expansion
      *                   |                                   | Tree::expander_cell_data_func
      *   - expanded      | Same as @collapsible              | Same as @collapsible
@@ -391,7 +390,7 @@
          * @since 0.2
          */
         public uint n_children {
-            get { return children_set.size; }
+            get { return children_list.size; }
         }
 
         /**
@@ -401,17 +400,11 @@
          */
         public Gee.Collection<Item> children {
             owned get {
-                // Create a copy of the children so that it's safe to iterate it
-                // (e.g. by using foreach) while removing items. See clear() for
-                // an example of such case.
-                var copy = new Gee.LinkedList<Item> ();
-                foreach (var item in children_set)
-                    copy.add (item);
-                return copy;
+                return children_list.read_only_view;
             }
         }
 
-        private Gee.Set<Item> children_set = new Gee.HashSet<Item> ();
+        private Gee.Collection<Item> children_list = new Gee.ArrayList<Item> ();
 
         /**
          * Creates a new {@link Granite.Widgets.SourceList.ExpandableItem}
@@ -426,6 +419,27 @@
         }
 
         /**
+         * Should return a negative integer, zero, or a positive integer if ''a'' sorts //before//
+         * ''b'', ''a'' sorts //with// ''b'', or ''a'' sorts //after// ''b'' respectively. If two
+         * items compare as equal, their order in the sorted source list is undefined.
+         *
+         * In order to ensure that the source list behaves as expected, this method must define a
+         * partial order on the source list tree; i.e. it must be reflexive, antisymmetric and
+         * transitive.
+         *
+         * (Same description as {@link Gtk.TreeIterCompareFunc}.)
+         *
+         * @param a First item.
+         * @param b Second item.
+         * @return A //negative// integer if //a// sorts after //b//, //zero// if //a// equals //b//,
+         *         or a //positive// integer if //a// sorts before //b//.
+         * @since 0.2
+         */
+        public virtual int compare (Item a, Item b) {
+            return 0;
+        }
+
+        /**
          * Checks whether the item contains the specified child.
          *
          * This method only considers the item's immediate children.
@@ -435,7 +449,7 @@
          * @since 0.2
          */
         public bool contains (Item item) {
-            return item in children_set;
+            return item in children_list;
         }
 
         /**
@@ -458,9 +472,9 @@
          * @see Granite.Widgets.SourceList.ExpandableItem.remove
          * @since 0.2
          */
-        public void add (Item item) requires (item.parent == null) requires (!contains (item)) {
+        public void add (Item item) requires (item.parent == null) {
             item.parent = this;
-            children_set.add (item);
+            children_list.add (item);
             child_added (item);
         }
 
@@ -478,8 +492,8 @@
          * @see Granite.Widgets.SourceList.ExpandableItem.clear
          * @since 0.2
          */
-        public void remove (Item item) requires (item.parent == this) requires (contains (item)) {
-            children_set.remove (item);
+        public void remove (Item item) requires (item.parent == this) {
+            children_list.remove (item);
             child_removed (item);
             item.parent = null;
         }
@@ -493,7 +507,12 @@
          * @since 0.2
          */
         public void clear () {
-            foreach (var item in children)
+            // Create a copy of the children so that it's safe to iterate it
+            // (e.g. by using foreach) while removing items
+            var children_list_copy = new Gee.ArrayList<Item> ();
+            children_list_copy.add_all (children_list);
+
+            foreach (var item in children_list_copy)
                 remove (item);
         }
 
@@ -675,11 +694,6 @@
             }
         }
 
-        private enum SortColumn {
-            UNSORTED = Gtk.SortColumn.UNSORTED,
-            SORTED = Gtk.SortColumn.DEFAULT + 1
-        }
-
         public signal void item_updated (Item item);
 
         /**
@@ -692,7 +706,7 @@
             get { return sort_dir; }
             set {
                 sort_dir = value;
-                child_tree.set_sort_column_id (this.sort_column, sort_dir);
+                resort ();
             }
         }
 
@@ -731,12 +745,15 @@
         private Gtk.TreeStore child_tree;
         private SourceList.SortFunc? sort_func;
         private unowned SourceList.VisibleFunc? filter_func;
-        private SortColumn sort_column = SortColumn.UNSORTED;
 
         public DataModel () {
             var child_tree = new Gtk.TreeStore (Column.N_COLUMNS, Column.ITEM.type ());
             Object (child_model: child_tree, virtual_root: null);
             this.child_tree = child_tree;
+
+            child_tree.set_default_sort_func (child_model_sort_func);
+            resort ();
+
             set_visible_func (filter_visible_func);
         }
 
@@ -967,10 +984,7 @@
          */
         public void set_sort_func (owned SourceList.SortFunc? sort_func) {
             this.sort_func = (owned) sort_func;
-            this.sort_column = this.sort_func != null ? SortColumn.SORTED : SortColumn.UNSORTED;
-
-            child_tree.set_sort_func (SortColumn.SORTED, child_model_sort_func);
-            sort_direction = sort_dir;
+            resort ();
         }
 
         /**
@@ -1013,20 +1027,31 @@
             return path.get_depth () == 1;
         }
 
-        /**
-         * Actual sort function. It simply returns zero if sort_func is null.
-         */
+        private void resort () {
+            child_tree.set_sort_column_id (Gtk.SortColumn.UNSORTED, sort_direction);
+            child_tree.set_sort_column_id (Gtk.SortColumn.DEFAULT, sort_direction);
+        }
+
         private int child_model_sort_func (Gtk.TreeModel model, Gtk.TreeIter a, Gtk.TreeIter b) {
-            // Return zero by default, since a different value would not be reflexive nor symmetric when
-            // sort_func is null.
             int sort = 0;
 
             Item? item_a, item_b;
             child_tree.get (a, Column.ITEM, out item_a, -1);
             child_tree.get (b, Column.ITEM, out item_b, -1);
 
-            if (sort_func != null && item_a != null && item_b != null)
-                sort = sort_func (item_a, item_b);
+            // If the sort function is not null use old sorting API. Otherwise, use each
+            // item's compare() method.
+            if (sort_func != null) {
+                if (item_a != null && item_b != null)
+                    sort = sort_func (item_a, item_b);
+            } else {
+                // code should only compare items on same hierarchy level
+                assert (item_a.parent == item_b.parent);
+
+                var parent = item_a.parent;
+                if (parent != null)
+                    sort = parent.compare (item_a, item_b);
+            }
 
             return sort;
         }
@@ -1063,7 +1088,7 @@
             if (item != null) {
                item_visible = item.visible;
 
-                // If the item is a category, also query the number of visible child items
+                // If the item is a category, also query the number of visible children
                 // because empty categories should not be displayed.
                 var expandable = item as ExpandableItem;
                 if (expandable != null && child_tree.iter_depth (iter) == 0) {
@@ -1936,6 +1961,7 @@
      *         or a //positive// integer if //a// sorts before //b//.
      * @since 0.2
      */
+    [Deprecated (replacement = "ExpandableItem.compare", since = "0.2")]
     public delegate int SortFunc (Item a, Item b);
 
     /**
@@ -1970,7 +1996,10 @@
      *
      * @since 0.2
      */
-    public ExpandableItem root { get; private set; default = new ExpandableItem (); }
+    public ExpandableItem root {
+        get { return data_model.root; }
+        set { data_model.root = value; }
+    }
 
     /**
      * The current selected item.
@@ -2022,20 +2051,19 @@
     }
 
     private Tree tree;
-    private DataModel data_model { get { return tree.data_model; } }
+    private DataModel data_model = new DataModel ();
 
     /**
      * Creates a new {@link Granite.Widgets.SourceList}.
      *
-     * @return (transfer full) a new {@link Granite.Widgets.SourceList}.
+     * @return A new {@link Granite.Widgets.SourceList}.
      * @since 0.2
      */
-    public SourceList () {
-        var model = new DataModel ();
-        model.root = root;
+    public SourceList (ExpandableItem root = new ExpandableItem ()) {
+        this.root = root;
 
         push_composite_child ();
-        tree = new Tree (model);
+        tree = new Tree (data_model);
         tree.set_composite_name ("treeview");
         pop_composite_child ();
 
@@ -2064,6 +2092,7 @@
      * @see Granite.Widgets.SourceList.SortFunc
      * @since 0.2
      */
+    [Deprecated (replacement = "ExpandableItem.compare", since = "0.2")]
     public void set_sort_func (owned SortFunc? sort_func) {
         data_model.set_sort_func ((owned) sort_func);
     }