Merge lp:~julien-spautz/granite/undo-close-tab into lp:~elementary-pantheon/granite/granite

[Julien Spautz]

Fixes bug #1020350 in granite by implementing this blueprint: https://blueprints.launchpad.net/granite/+spec/restoring-closed-tabs

I also updated granite-demo to allow duplicating and restoring tabs.

Existing applications will have to be patched to implement this feature, but this branch won't break anything, since it's disabled by default.

[David Gomes (davidgomes)]

Align lines 30, 31, 47, 53 and 54.
Newline after lines 54, 304, 150, 157, 189, 193.
Align line 57.
Remove all trailing whitespace (I wouldn't normally ask you to do this, but you added a lot of trailing whitespace on this branch).

That is just code style, I don't have time to read actual code or test it now.

review: Needs Fixing

[David Gomes (davidgomes)]

>void restore_menu_positioning
Also, maybe that function should be called void restore_menu_position.

[David Gomes (davidgomes)]

Newlines after lines 199, 186 too.

[David Gomes (davidgomes)]

Also, the lines mentioned are diff lines.

[Danielle Foré (danrabbit)]

Looks good here Julien. Nice work!

review: Approve

[David Gomes (davidgomes)]

Daniel, don't you think that clicking the trash when there's only one tab to restore should just restore it instead of showing a list of to-restore tabs (that is actually 1 element long)? My 2 cents here.

[Danielle Foré (danrabbit)]

David, hmm. Possibly, but maybe not because what if you're not sure what's in the trash?

[David Gomes (davidgomes)]

I guess you're right. I'll finish this review tomorrow, it's a lot of code and it needs a lot of testing.

Also Daniel, which apps would ship the trash button? I can't think of any it'd look good on except for maybe Scratch.

It clutters the view on a web browser IMO.

[Danielle Foré (danrabbit)]

Midori already includes the option to have the trash can (but in the toolbar). Do you think we should make showing the trash can optional?

[Julien Spautz (julien-spautz)]

One thing I'm not sure about is what should happen when the closed tabs menu gets extremely long. In a web browser this can easily happen. The number of tabs (open and closed) can only grow, but never shrink. One solution would be to implement a maximum amount of closed tabs. There's no use in having a list with 50 closed tabs, also that's what history is for.

[Danielle Foré (danrabbit)]

Hey Julien,

Perhaps we should look to Midori to see how their trash behaves. I agree with setting a reasonable limit. Perhaps 10 or 15?

I also noticed something small: I think the trash button should remain in the :active state while the menu is shown.

[Julien Spautz (julien-spautz)]

:active meaning 'pressed'? I think it's currently using a normal button but I can change it to a toggle button. That should fix it…

[Danielle Foré (danrabbit)]

Yep that's what I meant. Thanks!

[Cody Garver (codygarver)]

This branch is delayed until DynamicNotebook is fixed.

[Cody Garver (codygarver)]

bzr merge lp:~julien-spautz/granite/undo-close-tab
 M demo/main.vala
 M lib/Widgets/DynamicNotebook.vala
Text conflict in lib/Widgets/DynamicNotebook.vala
1 conflicts encountered.

review: Needs Fixing

[lp:~julien-spautz/granite/undo-close-tab]

556. By Julien Spautz on 2013-06-18

fixed conflicts

557. By Julien Spautz on 2013-06-18

fixed regressions

[Julien Spautz (julien-spautz)]

The conflicts should now be resolved.

[xapantu (xapantu)]

It looks good. However, I think that adding a tab member to the Tab class is not necessary and may be confusing. If you want to attach some data to a GObject, you can use the builtin set_data functions.

But apart from that, it looks great, so, I'll remove that and adapt the code in the demo before merging. Good work!

review: Approve

[xapantu (xapantu)]

Oh, I read the code too quickly, I didn't see that it was actually used...

[xapantu (xapantu)]

Merged, I just rename data to restore_data, the name data was a bit too generic for this in my opinion (and it is a public API, so we can't afford changing it every release ;) ).

=== modified file 'demo/main.vala'
--- demo/main.vala	2013-06-17 00:40:26 +0000
+++ demo/main.vala	2013-06-18 17:46:27 +0000
@@ -377,35 +377,61 @@
     }
 
     private Granite.Widgets.DynamicNotebook create_dynamic_notebook () {
+
+        int i = 3;
+
         var dynamic_notebook = new Granite.Widgets.DynamicNotebook ();
 
-        var tab = new Granite.Widgets.Tab ("user@elementaryos: ~",
-                                            new ThemedIcon ("empty"),
-                                            new Gtk.Label ("Page 1"));
+        dynamic_notebook.allow_duplication = true;
+        dynamic_notebook.allow_restoring = true;
+
+        var tab = new Granite.Widgets.Tab ("user1@elementaryos: ~",
+                                           new ThemedIcon ("empty"),
+                                           new Gtk.Label ("Page 1"));
+        tab.data = "1";
         tab.working = true;
-
         dynamic_notebook.insert_tab (tab, -1);
-        dynamic_notebook.insert_tab (new Granite.Widgets.Tab ("user2@elementaryos: ~",
-                                                              new ThemedIcon ("empty"),
-                                                              new Gtk.Label ("Page 2")), -1);
+
+        var tab2 = new Granite.Widgets.Tab ("user2@elementaryos: ~",
+                                            new ThemedIcon ("empty"),
+                                            new Gtk.Label ("Page 2"));
+        tab2.data = "2";
+        dynamic_notebook.insert_tab (tab2, -1);
 
         dynamic_notebook.tab_added.connect ((t) => {
-        	t.page = new Gtk.Label ("newuser@elementaryos: ~");
-        	t.label = "newuser@elementaryos: ~";
-    	});
-
-    	dynamic_notebook.tab_moved.connect ((t, p) => {
-    	    print ("Moved tab %s to %i\n", t.label, p);
-    	});
-
-    	dynamic_notebook.tab_switched.connect ((old_t, new_t) => {
-    	    print ("Switched from %s to %s\n", old_t.label, new_t.label);
-	    });
-
-		dynamic_notebook.tab_removed.connect ((t) => {
-		    print ("Going to remove %s\n", t.label);
-		    return true;
-	    });
+            t.page = new Gtk.Label (@"Page $i");
+            t.label = @"user$i@elementaryos: ~";
+            t.data = i.to_string ();
+            i++;
+        });
+
+        dynamic_notebook.tab_restored.connect ((t) => {
+            print ("Restored tab %s\n", t.label);
+            t.page = new Gtk.Label ("Page " + t.data);
+        });
+
+        dynamic_notebook.tab_duplicated.connect ((t) => {
+            var num = t.data;
+            var t2 = new Granite.Widgets.Tab (@"user$num@elementaryos: ~",
+                                              new ThemedIcon ("empty"),
+                                              new Gtk.Label (@"Page $num"));
+            t2.data = t.data;
+            dynamic_notebook.insert_tab (t2, -1);
+            print ("Duplicated tab %s\n", t2.label);
+        });
+
+        dynamic_notebook.tab_moved.connect ((t, p) => {
+            print ("Moved tab %s to %i\n", t.label, p);
+        });
+
+        dynamic_notebook.tab_switched.connect ((old_t, new_t) => {
+            print ("Switched from %s to %s\n", old_t.label, new_t.label);
+        });
+
+        dynamic_notebook.tab_removed.connect ((t) => {
+            print ("Going to remove %s\n", t.label);
+            return true;
+        });
 
         return dynamic_notebook;
     }

=== modified file 'lib/Widgets/DynamicNotebook.vala'
--- lib/Widgets/DynamicNotebook.vala	2013-06-17 18:38:22 +0000
+++ lib/Widgets/DynamicNotebook.vala	2013-06-18 17:46:27 +0000
@@ -28,12 +28,15 @@
      * This is a standard tab which can be used in a notebook to form a tabbed UI.
      */
     public class Tab : Gtk.Box {
+
         Gtk.Label _label;
         public string label {
             get { return _label.label;  }
             set { _label.label = value; }
         }
 
+        public string data { get; set; }
+
         internal Gtk.EventBox page_container;
         public Gtk.Widget page {
             get {
@@ -101,7 +104,7 @@
             this.close = new Gtk.Button ();
 
             close.add (new Gtk.Image.from_icon_name ("window-close-symbolic", Gtk.IconSize.MENU));
-            close.tooltip_text = _("Close tab");
+            close.tooltip_text = _("Close Tab");
             close.relief = Gtk.ReliefStyle.NONE;
 
             var lbl = new Gtk.EventBox ();
@@ -147,7 +150,7 @@
                             return true;
                         }
                         break;
-                        
+
                     case Gdk.ScrollDirection.DOWN:
                     case Gdk.ScrollDirection.RIGHT:
                         if (notebook.page < notebook.get_n_pages ()) {
@@ -156,14 +159,14 @@
                         }
                         break;
                 }
-                
+
                 return false;
             });
 
             lbl.button_press_event.connect ((e) => {
-            
+
                 e.state &= MODIFIER_MASK;
-                
+
                 if (e.button == 2 && e.state == 0 && close.visible) {
                     this.closed ();
                 } else if (e.button == 2 && e.state == Gdk.ModifierType.SHIFT_MASK && close.visible) {
@@ -174,25 +177,106 @@
                     menu.popup (null, null, null, 3, e.time);
                     uint num_tabs = (this.get_parent () as Gtk.Container).get_children ().length ();
                     close_other_m.label = ngettext (_("Close Other Tab"), _("Close Other Tabs"), num_tabs - 1);
-                    if (num_tabs == 1)
-                        close_other_m.sensitive = false;
+                    close_other_m.sensitive = (num_tabs != 1);
                 } else {
                     return false;
                 }
-                
+
                 return true;
             });
-            
+
             this.button_press_event.connect ((e) => {
                 return (e.type == Gdk.EventType.2BUTTON_PRESS || e.button != 1);
             });
-            
+
             page_container.button_press_event.connect (() => { return true; }); //dont let clicks pass through
             close.clicked.connect ( () => this.closed () );
             working = false;
         }
     }
 
+    private class ClosedTabs : GLib.Object {
+
+        public signal void restored (Tab tab);
+
+        private struct Entry {
+            string title;
+            string data;
+            GLib.Icon? icon;
+        }
+
+        private Entry[] closed_tabs;
+
+        public ClosedTabs () {
+            closed_tabs = {};
+        }
+
+        public bool empty {
+            get {
+                return closed_tabs.length == 0;
+            }
+        }
+
+        public void push (Tab tab) {
+            foreach (var entry in closed_tabs)
+                if (tab.data == entry.data)
+                    return;
+            Entry tmp = { tab.label, tab.data, tab.icon };
+            closed_tabs += tmp;
+        }
+
+        public Tab pop () {
+            assert (closed_tabs.length > 0);
+            Entry entry = closed_tabs[closed_tabs.length - 1];
+            var tab = new Tab (entry.title);
+            tab.data = entry.data;
+            tab.icon = entry.icon;
+            closed_tabs.resize (closed_tabs.length - 1);
+            return tab;
+        }
+
+        public Tab? pick (string search) {
+            var tab = (Tab) null;
+            Entry[] copy = {};
+
+            foreach (var entry in closed_tabs) {
+                if (entry.data != search) {
+                    copy += entry;
+                } else {
+                    tab = new Tab (entry.title);
+                    tab.data = entry.data;
+                    tab.icon = entry.icon;
+                }
+            }
+
+            closed_tabs = copy;
+            return tab;
+        }
+
+        private Gtk.Menu _menu;
+        public Gtk.Menu menu {
+            get {
+                _menu = new Gtk.Menu ();
+
+                foreach (var entry in closed_tabs) {
+                    var item = new Gtk.ImageMenuItem.with_label (entry.title);
+                    item.set_always_show_image (true);
+                    if (entry.icon != null) {
+                        var icon = new Gtk.Image.from_gicon (entry.icon, Gtk.IconSize.MENU);
+                        item.set_image (icon);
+                    }
+                    _menu.prepend (item);
+
+                    item.activate.connect (() => {
+                        this.restored (pick (entry.data));
+                    });
+                }
+
+                return _menu;
+            }
+        }
+    }
+
     public class DynamicNotebook : Gtk.EventBox {
 
         /**
@@ -210,10 +294,10 @@
             set { notebook.show_tabs = value; }
         }
 
+        /**
+         * Toggle icon display
+         */
         bool _show_icons;
-        /**
-         * Toggle icon display
-         */
         public bool show_icons {
             get { return _show_icons; }
             set {
@@ -282,6 +366,19 @@
             }
         }
 
+        /**
+         * Allow restoring tabs
+         */
+        bool _allow_restoring = false;
+        public bool allow_restoring {
+            get { return _allow_restoring; }
+            set {
+                _allow_restoring = value;
+                restore_tab_m.visible = value;
+                restore_button.visible = value;
+            }
+        }
+
         public Tab current {
             get { return tabs.nth_data (notebook.get_current_page ()); }
             set { notebook.set_current_page (tabs.index (value)); }
@@ -298,6 +395,7 @@
             }
         }
 
+
         public string group_name {
             get { return notebook.group_name; }
             set { notebook.group_name = value; }
@@ -308,6 +406,8 @@
          */
         public Gtk.Menu menu { get; private set; }
 
+        private ClosedTabs closed_tabs;
+
         Gtk.Notebook notebook;
         private Gtk.CssProvider button_fix;
 
@@ -320,6 +420,13 @@
         public signal void tab_switched (Tab? old_tab, Tab new_tab);
         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_restored (Tab tab);
+
+        private Gtk.MenuItem new_tab_m;
+        private Gtk.MenuItem restore_tab_m;
+
+        private Gtk.Button add_button;
+        private Gtk.Button restore_button; // should be a Gtk.MenuButton when we have Gtk+ 3.6
 
         private static const string CLOSE_BUTTON_STYLE = """
         * {
@@ -357,19 +464,72 @@
             this.add (this.notebook);
 
             menu = new Gtk.Menu ();
-
-            var new_tab_m = new Gtk.MenuItem.with_label (_("New Tab"));
+            new_tab_m = new Gtk.MenuItem.with_label (_("New Tab"));
+            restore_tab_m = new Gtk.MenuItem.with_label (_("Undo Close Tab"));
+            restore_tab_m.sensitive = false;
             menu.append (new_tab_m);
-
+            menu.append (restore_tab_m);
             menu.show_all ();
 
             new_tab_m.activate.connect (() => {
                 insert_new_tab_at_end ();
             });
 
+            restore_tab_m.activate.connect (() => {
+                restore_last_tab ();
+            });
+
+            closed_tabs = new ClosedTabs ();
+            closed_tabs.restored.connect ((tab) => {
+                if (!allow_restoring)
+                    return;
+                restore_button.sensitive = !closed_tabs.empty;
+                insert_tab (tab, -1);
+                this.tab_restored (tab);
+            });
+
+            add_button = new Gtk.Button ();
+            add_button.add (new Gtk.Image.from_icon_name ("list-add-symbolic", Gtk.IconSize.MENU));
+            add_button.margin_left = 6;
+            add_button.relief = Gtk.ReliefStyle.NONE;
+            add_button.tooltip_text = _("New Tab");
+            this.notebook.set_action_widget (add_button, Gtk.PackType.START);
+            add_button.show_all ();
+            add_button.get_style_context ().add_provider (button_fix, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);
+
+            restore_button = new Gtk.Button ();
+            restore_button.add (new Gtk.Image.from_icon_name ("user-trash-symbolic", Gtk.IconSize.MENU));
+            restore_button.margin_right = 5;
+            restore_button.set_relief (Gtk.ReliefStyle.NONE);
+            restore_button.tooltip_text = _("Closed Tabs");
+            restore_button.sensitive = false;
+            this.notebook.set_action_widget (restore_button, Gtk.PackType.END);
+            restore_button.show_all ();
+
+            add_button.clicked.connect (() => {
+                insert_new_tab_at_end ();
+            });
+
+            restore_button.clicked.connect (() => {
+                var menu = closed_tabs.menu;
+                menu.attach_widget = restore_button;
+                menu.show_all ();
+                menu.popup (null, null, this.restore_menu_position, 1, 0);
+            });
+
+            restore_tab_m.visible = allow_restoring;
+            restore_button.visible = allow_restoring;
+
+            this.size_allocate.connect (() => {
+                this.recalc_size ();
+            });
+
             this.button_press_event.connect ((e) => {
                 if (e.type == Gdk.EventType.2BUTTON_PRESS && e.button == 1) {
                     insert_new_tab_at_end ();
+                } else if (e.button == 2 && allow_restoring) {
+                    restore_last_tab ();
+                    return true;
                 } else if (e.button == 3) {
                     menu.popup (null, null, null, 3, e.time);
                 }
@@ -377,28 +537,18 @@
                 return false;
             });
 
-            var add = new Gtk.Button ();
-            add.add (new Gtk.Image.from_icon_name ("list-add-symbolic", Gtk.IconSize.MENU));
-            add.margin_left = 6;
-            add.relief = Gtk.ReliefStyle.NONE;
-            add.tooltip_text = _("New tab");
-            this.notebook.set_action_widget (add, Gtk.PackType.START);
-            add.show_all ();
-            add.get_style_context ().add_provider (button_fix, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);
-
-            add.clicked.connect ( () => {
-                insert_new_tab_at_end ();
-            });
-
             this.size_allocate.connect (() => {
                 this.recalc_size ();
             });
             
             this.key_press_event.connect ((e) => {
+
+                e.state &= MODIFIER_MASK;
+
                 switch (e.keyval) {
                     case Gdk.Key.@w:
                     case Gdk.Key.@W:
-                        if ((e.state & Gdk.ModifierType.CONTROL_MASK) == Gdk.ModifierType.CONTROL_MASK) {
+                        if (e.state == Gdk.ModifierType.CONTROL_MASK) {
                             if (!tabs_closable) break;
                             remove_tab (current);
                             return true;
@@ -408,15 +558,18 @@
 
                     case Gdk.Key.@t:
                     case Gdk.Key.@T:
-                        if ((e.state & Gdk.ModifierType.CONTROL_MASK) == Gdk.ModifierType.CONTROL_MASK) {
+                        if (e.state == Gdk.ModifierType.CONTROL_MASK) {
                             insert_new_tab_at_end ();
                             return true;
+                        } else if (e.state == (Gdk.ModifierType.CONTROL_MASK | Gdk.ModifierType.SHIFT_MASK) && allow_restoring) {
+                            restore_last_tab ();
+                            return true;
                         }
 
                         break;
 
                     case Gdk.Key.Page_Up:
-                        if ((e.state & Gdk.ModifierType.CONTROL_MASK) == Gdk.ModifierType.CONTROL_MASK) {
+                        if (e.state == Gdk.ModifierType.CONTROL_MASK) {
                             next_page ();
                             return true;
                         }
@@ -424,7 +577,7 @@
                         break;
 
                     case Gdk.Key.Page_Down:
-                        if ((e.state & Gdk.ModifierType.CONTROL_MASK) == Gdk.ModifierType.CONTROL_MASK) {
+                        if (e.state == Gdk.ModifierType.CONTROL_MASK) {
                             previous_page ();
                             return true;
                         }
@@ -447,6 +600,7 @@
                         }
 
                         break;
+
                     case Gdk.Key.@9:
                         if ((e.state & Gdk.ModifierType.MOD1_MASK) == Gdk.ModifierType.MOD1_MASK) {
                             notebook.page = notebook.get_n_pages () - 1;
@@ -470,6 +624,15 @@
             notebook.create_window.disconnect (on_create_window);
         }
 
+        void restore_menu_position (Gtk.Menu menu, out int x, out int y, out bool p) {
+            Gtk.Allocation button_alloc, menu_alloc;
+            restore_button.get_allocation (out button_alloc);
+            menu.get_allocation (out menu_alloc);
+            restore_button.get_window ().get_origin (out x, out y);
+            x += button_alloc.x - menu_alloc.width + button_alloc.width + 5;
+            y += button_alloc.y + button_alloc.height + 1;
+        }
+
         void on_switch_page (Gtk.Widget page, uint pagenum) {
             var new_tab = notebook.get_tab_label (page) as Tab;
 
@@ -484,7 +647,7 @@
         unowned Gtk.Notebook on_create_window (Gtk.Widget page, int x, int y) {
             var tab = notebook.get_tab_label (page) as Tab;
             tab_moved (tab, 0, true, x, y);
-            return null;
+            return (Gtk.Notebook) null;
         }
 
         private void recalc_size () {
@@ -504,6 +667,17 @@
             }
         }
 
+        private void restore_last_tab () {
+            if (!allow_restoring || closed_tabs.empty)
+                return;
+
+            var tab = closed_tabs.pop ();
+            restore_button.sensitive = !closed_tabs.empty;
+            restore_tab_m.sensitive = !closed_tabs.empty;
+            insert_tab (tab, -1);
+            this.tab_restored (tab);
+        }
+
         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);
@@ -512,8 +686,15 @@
             }
 
             var pos = get_tab_position (tab);
+
             if (pos != -1)
                 notebook.remove_page (pos);
+            
+            if (tab.label != "" && tab.data != "") {
+                closed_tabs.push (tab);
+                restore_button.sensitive = !closed_tabs.empty;
+                restore_tab_m.sensitive = !closed_tabs.empty;
+            }
         }
         
 	[Deprecated (since=0.2)]