Merge lp:~elementary-apps/granite/dynamicnotebook into lp:~elementary-pantheon/granite/granite

[Rico Tzschichholz]

Just here to see a nice diff against trunk.

[lp:~elementary-apps/granite/dynamicnotebook]

559. By Rico Tzschichholz on 2013-04-10

Move dummy new_window callback for tab

[Cody Garver (codygarver)]

What's the hold up here?

[David Gomes (davidgomes)]

It's not perfect yet. I've been wanting to work on time but I still didn't have enough free time.

[Cody Garver (codygarver)]

Should the status of this branch be Rejected now that xapantu has got a DN fix in trunk?

review: Needs Information

[xapantu (xapantu)]

This branch does not fix the same bug, while my code just fixed the current bug, this is a larger refactoring of the way the DynamicNotebook works, which is interesting since this way is much more logical. The tab_moved signal is really not practical (the only good point in it is that everything can be handled with one signal, but I am not sure it is a very good idea).

Unmerged revisions

559. By Rico Tzschichholz on 2013-04-10

Move dummy new_window callback for tab

558. By Rico Tzschichholz on 2013-04-09

Avoid breaking ABI/API

557. By Tom Beckmann on 2013-03-29

Do proper signal handling fixing detaching tabs

=== modified file 'lib/Widgets/DynamicNotebook.vala'
--- lib/Widgets/DynamicNotebook.vala	2013-04-05 07:11:07 +0000
+++ lib/Widgets/DynamicNotebook.vala	2013-04-10 09:31:27 +0000
@@ -314,13 +314,40 @@
         private int tab_width = 150;
         private int max_tab_width = 150;
 
+        /**
+         * A new tab was added
+         * @param tab the new Tab
+         */
         public signal void tab_added (Tab tab);
+
+        /**
+         * A new tab was added
+         * Note: The return value has no meaning!
+         * @param tab the removed Tab
+         */
         public signal bool tab_removed (Tab tab);
-        Tab? old_tab; //stores a reference for tab_switched
+
+        unowned Tab? old_tab; //stores a reference for tab_switched
         public signal void tab_switched (Tab? old_tab, Tab new_tab);
+
+        //FIXME can be removed on API cleaning
+        /**
+         * Connect to tab_create_window and tab_reordered instead
+         */
+        [Deprecated (since=0.2)]
         public signal void tab_moved (Tab tab, int new_pos, bool new_window, int x, int y);
+
         public signal void tab_duplicated (Tab duplicated_tab);
 
+        public signal void tab_reordered (Tab tab, int new_pos);
+
+        /**
+         * A tab was detached. You're supposed to connect to this signal
+         * and return the notebook this tab should be added or null, if 
+         * you want to handle adding it manually
+         */
+        public signal DynamicNotebook? tab_detached (Tab tab, int x, int y);
+
         private static const string CLOSE_BUTTON_STYLE = """
         * {
             -GtkButton-default-border : 0;
@@ -366,14 +393,12 @@
             new_tab_m.activate.connect (() => {
                 var t = new Tab ();
                 notebook.page = (int) this.insert_tab (t, -1);
-                this.tab_added (t);
             });
 
             this.button_press_event.connect ((e) => {
                 if (e.type == Gdk.EventType.2BUTTON_PRESS && e.button == 1) {
                     var t = new Tab ();
                     notebook.page = (int) this.insert_tab (t, -1);
-                    this.tab_added (t);
                 } else if (e.button == 3) {
                     menu.popup (null, null, null, 3, e.time);
                 }
@@ -393,7 +418,6 @@
             add.clicked.connect ( () => {
                 var t = new Tab ();
                 notebook.page = (int) this.insert_tab (t, -1);
-                this.tab_added (t);
             });
 
             this.size_allocate.connect (() => {
@@ -416,7 +440,6 @@
                     case Gdk.Key.@T:
                         if ((e.state & Gdk.ModifierType.CONTROL_MASK) == Gdk.ModifierType.CONTROL_MASK) {
                             var t = new Tab ();
-                            this.tab_added (t);
                             notebook.page = (int) this.insert_tab (t, -1);
                             return true;
                         }
@@ -467,35 +490,80 @@
                 return false;
             });
 
+            notebook.page_added.connect (on_page_added);
+            notebook.page_removed.connect (on_page_removed);
             notebook.switch_page.connect (on_switch_page);
             notebook.page_reordered.connect (on_page_reordered);
             notebook.create_window.connect (on_create_window);
         }
 
         ~Notebook () {
+            notebook.page_added.disconnect (on_page_added);
+            notebook.page_removed.disconnect (on_page_removed);
             notebook.switch_page.disconnect (on_switch_page);
             notebook.page_reordered.disconnect (on_page_reordered);
             notebook.create_window.disconnect (on_create_window);
         }
 
-        void on_switch_page (Gtk.Widget page, uint pagenum) {
-            var new_tab = notebook.get_tab_label (page) as Tab;
+        void on_switch_page (Gtk.Notebook notebook, Gtk.Widget page, uint pagenum) {
+            unowned Tab? new_tab = notebook.get_tab_label (page) as Tab;
+            if (new_tab == null)
+                return;
 
-            tab_switched (old_tab, new_tab);
+            unowned Tab? current_tab = old_tab as Tab;
             old_tab = new_tab;
-        }
-
-        void on_page_reordered (Gtk.Widget page, uint pagenum) {
-            tab_moved (notebook.get_tab_label (page) as Tab, (int) pagenum, false, -1, -1);
+
+            tab_switched (current_tab, new_tab);
+        }
+
+        void on_page_reordered (Gtk.Notebook notebook, Gtk.Widget page, uint pagenum) {
+            unowned Tab? tab = notebook.get_tab_label (page) as Tab;
+            if (tab == null)
+                return;
+
+            //FIXME must be removed on API cleaning
+            if (Signal.has_handler_pending (this, Signal.lookup ("tab-moved", typeof (DynamicNotebook)), 0, true)) {
+                // Filled up with dummy values to preserve API
+                tab_moved (tab, (int)pagenum, false, 0, 0);
+                return;
+            }
+
+            tab_reordered (tab, (int)pagenum);
+        }
+
+        void on_page_removed (Gtk.Notebook notebook, Gtk.Widget page, uint pagenum) {
+            unowned Tab? tab = page as Tab;
+            if (tab == null)
+                return;
+
+            disconnect_tab_signals (tab);
+            tab_removed (tab);
+        }
+
+        void on_page_added (Gtk.Notebook notebook, Gtk.Widget page, uint pagenum) {
+            unowned Tab? tab = notebook.get_tab_label (page) as Tab;
+            if (tab == null)
+                return;
+
+            connect_tab_signals (tab);
+            tab_added (tab);
         }
 
         unowned Gtk.Notebook on_create_window (Gtk.Widget page, int x, int y) {
-            var tab = notebook.get_tab_label (page) as Tab;
-            notebook.remove_page (notebook.page_num (tab.page_container));
-            tab.page_container.destroy ();
-
-            tab_moved (tab, 0, true, x, y);
-            return null;
+            Tab? tab = notebook.get_tab_label (page) as Tab;
+            if (tab == null)
+                return null;
+
+            remove_tab (tab);
+
+            //FIXME must be removed on API cleaning
+            if (Signal.has_handler_pending (this, Signal.lookup ("tab-moved", typeof (DynamicNotebook)), 0, true)) {
+                tab_moved (tab, 0, true, x, y);
+                return null;
+            }
+
+            var new_notebook = tab_detached (notebook.get_tab_label (page) as Tab, x, y);
+            return new_notebook != null ? new_notebook.notebook : null;
         }
 
         private void recalc_size () {
@@ -516,25 +584,19 @@
         }
 
         public void remove_tab (Tab tab) {
-            if (Signal.has_handler_pending (this, Signal.lookup ("tab-removed", typeof (DynamicNotebook)), 0, true)) {
-                var sure = tab_removed (tab);
-                if (!sure)
-                    return;
-            }
-
             var pos = get_tab_position (tab);
-            if (pos != -1)
-                notebook.remove_page (pos);
-            tab.page_container.destroy ();
+            if (pos < 0)
+                return;
+
+            notebook.remove_page (pos);
         }
 
         public void next_page () {
-            this.notebook.page = this.notebook.page + 1 >= this.notebook.get_n_pages () ? this.notebook.page = 0 : this.notebook.page + 1;
+            notebook.next_page ();
         }
 
         public void previous_page () {
-            this.notebook.page = this.notebook.page - 1 < 0 ?
-                                 this.notebook.page = this.notebook.get_n_pages () - 1 : this.notebook.page - 1;
+            notebook.prev_page ();
         }
 
         public override void show () {
@@ -558,7 +620,6 @@
 
         public void set_tab_position (Tab tab, int position) {
             notebook.reorder_child (tab.page_container, position);
-            tab_moved (tab, position, false, -1, -1);
         }
 
         public Tab? get_tab_by_index (int index) {
@@ -576,15 +637,6 @@
         public uint insert_tab (Tab tab, int index) {
             return_if_fail (tabs.index (tab) < 0);
 
-            var i = 0;
-            if (index == -1)
-                i = this.notebook.insert_page (tab.page_container, tab, this.notebook.get_n_pages ());
-            else
-                i = this.notebook.insert_page (tab.page_container, tab, index);
-
-            this.notebook.set_tab_reorderable (tab.page_container, this.allow_drag);
-            this.notebook.set_tab_detachable  (tab.page_container, this.allow_new_window);
-
             tab._icon.visible = show_icons;
             tab.duplicate_m.visible = allow_duplication;
             tab.new_window_m.visible = allow_new_window;
@@ -592,41 +644,63 @@
             tab.width_request = tab_width;
             tab.close.get_style_context ().add_provider (button_fix,
                                                          Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);
-
-            tab.closed.connect ( () => {
-                remove_tab (tab);
-            });
-
-            tab.close_others.connect ( () => {
-                var num = 0; //save num, in case a tab refused to close so we don't end up in an infinite loop
-
-                for (var j = 0; j < tabs.length (); j++) {
-                    if (tab != tabs.nth_data (j)) {
-                        tabs.nth_data (j).closed ();
-                        if (num == n_tabs) break;
-                        j--;
-                    }
-
-                    num = n_tabs;
-                }
-            });
-
-            tab.new_window.connect (() => {
-                notebook.remove_page (notebook.page_num (tab.page_container));
-                tab.page_container.destroy ();
-                tab_moved (tab, 0, true, 0, 0);
-            });
-
-            tab.duplicate.connect (() => {
-                tab_duplicated (tab);
-            });
-
-            this.recalc_size ();
-
-            if (!tabs_closable)
-                tab.close.visible = false;
+            tab.close.visible = tabs_closable;
+
+            uint i = 0;
+            if (index == -1)
+                i = notebook.insert_page (tab.page_container, tab, notebook.get_n_pages ());
+            else
+                i = notebook.insert_page (tab.page_container, tab, index);
+
+            notebook.set_tab_reorderable (tab.page_container, allow_drag);
+            notebook.set_tab_detachable  (tab.page_container, allow_new_window);
+
+            recalc_size ();
 
             return i;
         }
+
+        void connect_tab_signals (Tab tab) {
+            tab.closed.connect (on_tab_closed);
+            tab.close_others.connect (on_close_others);
+            tab.new_window.connect (on_tab_new_window);
+            tab.duplicate.connect (on_tab_duplicated);
+        }
+
+        void disconnect_tab_signals (Tab tab) {
+            tab.closed.disconnect (on_tab_closed);
+            tab.close_others.disconnect (on_close_others);
+            tab.new_window.disconnect (on_tab_new_window);
+            tab.duplicate.disconnect (on_tab_duplicated);
+        }
+
+        void on_tab_duplicated (Tab tab) {
+            tab_duplicated (tab);
+        }
+
+        void on_tab_new_window (Tab tab) {
+            //notebook.remove_page (notebook.page_num (tab.page_container));
+            //tab.page_container.destroy ();
+            //notebook.create_window (tab.page_container, -1, -1);
+        }
+
+        void on_tab_closed (Tab tab) {
+            remove_tab (tab);
+        }
+
+        void on_close_others (Tab tab) {
+            var num = 0; //save num, in case a tab refused to close so we don't end up in an infinite loop
+
+            for (var j = 0; j < tabs.length (); j++) {
+                unowned Tab t = tabs.nth_data (j);
+                if (tab != t) {
+                    t.closed ();
+                    if (num == n_tabs) break;
+                    j--;
+                }
+
+                num = n_tabs;
+            }
+        }
     }
 }