Merge lp:~niclasl/pantheon-terminal/fix-1218681 into lp:~elementary-apps/pantheon-terminal/trunk

[Niclas Lockner]

Implemented support for restoring tabs.
Fixes bug #1218681.

DynamicNotebook: Fixed support for restoring closed tabs #1218681.

[Julián Unrrein (junrrein)]

Code style:

Newline after diff line 29.

Testing:

1) Have at least two tabs opened in a Terminal window.
2) Drag one of them to form a new window.
3) Open at least one other tab in this new window (so that the dragged tab isn't the only one).
4) Close the dragged tab.

The closed dragged tab goes to the trash bin in the original window, instead of the new one as it should be.

Note that the new window may close itself when closing the dragged tab (this is known to happen in trunk).

review: Needs Fixing

[David Gomes (davidgomes)]

>tab.restore_data = t.get_shell_location ();

If you have two tabs in the same location won't that mess it up? Shouldn't restore_data be an unique ID? I'm never sure what restore_data is for.

In any case, remove the trailing whitespace you added.

review: Needs Fixing

[Niclas Lockner (niclasl)]

I'm not by my computer at the moment, but if I remember it correctly, the tab is only added to the ClosedTabs object in the DynamicNotebook object if it has a unique restore_data. At the moment, two closed tabs with the same location leads to only one of the tabs being added to the list of restorable tabs. This is the intended behavior, right?

And it seems that restore_data is intended to be whatever helps restoring the tab. I looked at how it was done in Files, and I think it also used the tab's current location.

Have you had time to speak to voldyman about the signals?

[Julien Spautz (julien-spautz)]

As Niclas said, the restore_data has to be unique.

There is no real limitation as to what you put into restore_data. In most cases a file path or URL/URI is the easiest thing to do, but it could also be some sort of ID.

See the original blueprint: https://blueprints.launchpad.net/granite/+spec/restoring-closed-tabs

I haven't tested this branch, but I assume restoring a tab will only restore the location without the content, i.e. previously entered commands. I think it would be better to restore the content too. For this, you could store the terminal widgets in a hashmap, mapped to some ID, which you'll assign to restore_data.

I hope that clears things up a little.

[Niclas Lockner (niclasl)]

I agree that saving the actual terminal and not only the directory would be more useful, but if I'm going to save the TerminalWidget object when a tab is closed, shouldn't there be a maximum number of stored objects, e.g. 10 last closed? If not, then hidden terminal widgets and bash processes would keep accumulating while the window is kept open. If it is limited, the same limit would need to be added to DynamicNotebook so that it doesn't store unlimited number of tabs.

The problem with the tab that gets added to the first window's trash bin even though it was closed in the second window seems to be related to DynamicNotebook as well. When the tab is moved from window A to window B, the signal handlers that the notebook in window A added to the tab are kept alongside the new handlers added by the notebook in window B. When the tab is closed, the connected signals from both window A and B are activated and therefore the tab ends up in window A's trash bin.

So how should I proceed with this? It looks like this cannot be fixed (in a nice way) without also touching DynamicNotebook.

[Julien Spautz (julien-spautz)]

The tab ending up in the wrong window is a granite bug that you cannot fix here.

I'm not sure if the maximum amount of closed tabs should be hardcoded or be allowed to vary across apps, that needs some discussion, but there should definitely be a limit.

I don't know Terminal's code very well, but is there a way to store the content of a terminal widget more efficiently? Assuming we don't allow closing tabs with background processes, it would be sufficient to save the text only, which might be a more viable solution, if storing the terminal widgets themselves takes up too much memory.

[Niclas Lockner (niclasl)]

I'll file a separate bug report for granite then.

I cannot find anything in the API that suggests that it's possible to insert a text to a Vte.Terminal object, which makes sense I guess. And even though it would be possible, the terminal widget is not completely restored anyway since the command history probably wont be restored (it is the bash process that holds that information, right?)

[Niclas Lockner (niclasl)]

Okay, so my plan to solve this is as follows.

I create my own branch of Granite, where I
* solve the signal issue
* implement an upper limit (changeable with a setter) of the number of restorable tabs
* implement a new signal on the tab that gets activated when DynamicNotebook removes the tab from the set of restorable tabs (due to the limit being exceeded, or "Clear All" being pressed)

In this pantheon-terminal branch, I will
* store closed terminal widgets in a hashmap, as Julien suggests.
* let restored tabs reuse the existing terminal widget
* connect a callback to the "tab no longer restorable" signal that removes the terminal widget from the hashmap.

The Granite branch is just so that I have something to work with in pantheon-terminal, but if you like the changes you can use them as you like.

Does this sound good?

[Julián Unrrein (junrrein)]

Hi Niclas,

Your plan sounds perfectly reasonable, you can go with it.

However, the Granite branch will need to get merged for your Terminal branch to get merged too.

[David Gomes (davidgomes)]

Remove the extra trailing whitepsace you added.

review: Needs Fixing

[David Gomes (davidgomes)]

review: Approve

[lp:~niclasl/pantheon-terminal/fix-1218681]

521. By Launchpad Translations on behalf of elementary-apps on 2013-09-24

Launchpad automatic translations update.

522. By Cody Garver on 2013-09-29

Silence C compilation warnings.

523. By David Gomes on 2013-09-29

Got rid of all the using statements.

524. By Rui Gomes on 2013-09-30

Minor fixes to the INSTALL file.

525. By Jeremy Wootten on 2013-10-02

Fixed bug #1208140, moved -d to -w, steps necessary taken to fix #1033456 in Files.

526. By Cody Garver on 2013-10-02

Release 0.2.4

527. By Sergey "Shnatsel" Davidoff on 2013-10-03

Use proper NoDisplay value to stop "Open Terminal Here" from showing up in Slingshot and other application launchers.

528. By Cody Garver on 2013-10-03

Release 0.2.4.1

529. By Launchpad Translations on behalf of elementary-apps on 2013-10-07

Launchpad automatic translations update.

530. By Launchpad Translations on behalf of elementary-apps on 2013-10-27

Launchpad automatic translations update.

531. By Niclas Lockner on 2013-11-15

Removed unused code

532. By Akshay Shekher on 2013-11-16

License fixes

533. By Launchpad Translations on behalf of elementary-apps on 2013-11-23

Launchpad automatic translations update.

[David Gomes (davidgomes)]

review: Abstain

[lp:~niclasl/pantheon-terminal/fix-1218681]

534. By Niclas Lockner on 2013-12-23

Made compatible with DynamicNotebook's new API.

535. By David Gomes on 2013-12-23

Granite version bump.

536. By David Gomes on 2013-12-23

Fixed typo.

537. By Niclas Lockner on 2013-12-27

Small-scale refactorization.

Simplified tab closing and window state saving, as well as the visibility of the tab bar code.
Silenced runtime errors and compiler warnings.
Fixed shell processes not being killed as well as follow-last-tab behavior not working correctly.

538. By Jeremy Wootten on 2013-12-27

Changed "Open Terminal Here" to simply "Terminal".

539. By Niclas Lockner on 2013-12-27

Implemented support for restoring tabs

[Niclas Lockner (niclasl)]

This is now ready to be reviewed again

[lp:~niclasl/pantheon-terminal/fix-1218681]

540. By Niclas Lockner on 2013-12-28

Remove removed tab from the terminals list

[Niclas Lockner (niclasl)]

I just noticed the missing space after create_tab. I'll fix that later today.

review: Needs Fixing

[lp:~niclasl/pantheon-terminal/fix-1218681]

541. By Niclas Lockner on 2013-12-28

Code-style fix

542. By Niclas Lockner on 2013-12-28

Code-style fix

[Niclas Lockner (niclasl)]

Code-style fixed

review: Approve

[lp:~niclasl/pantheon-terminal/fix-1218681]

543. By Niclas Lockner on 2014-01-19

Catch Ctrl+D

544. By Niclas Lockner on 2014-01-19

Catch both Ctrl + d and D

[David Gomes (davidgomes)]

There seems to be a bug. Say I have two tabs open and I close the second one with Ctrl+Shift+W or Ctrl+D, the first tab isn't focused. I think it's a problem with your branch because it doesn't happen with trunk.

review: Needs Fixing

[lp:~niclasl/pantheon-terminal/fix-1218681]

545. By Niclas Lockner on 2014-01-21

Let the new terminal grab the focus after the old one is closed

[David Gomes (davidgomes)]

Diff lines 46-47. Maybe you should just call action_close_tab () there?

review: Needs Fixing

=== modified file 'src/PantheonTerminalWindow.vala'
--- src/PantheonTerminalWindow.vala	2013-12-26 21:48:07 +0000
+++ src/PantheonTerminalWindow.vala	2014-01-21 20:49:25 +0000
@@ -29,6 +29,8 @@
         private Gtk.Clipboard clipboard;
 
         private GLib.List <TerminalWidget> terminals = new GLib.List <TerminalWidget> ();
+        private HashTable<string, TerminalWidget> restorable_terminals;
+        private int restorable_counter = 0;
 
         public TerminalWidget current_terminal = null;
         private bool is_fullscreen = false;
@@ -139,6 +141,9 @@
                 open_tabs ();
 
             set_size_request (app.minimum_width, app.minimum_height);
+
+            destroy.connect (on_destroy);
+            restorable_terminals = new HashTable<string, TerminalWidget> (str_hash, str_equal);
         }
 
         private void setup_ui () {
@@ -161,11 +166,14 @@
             notebook.tab_switched.connect (on_switch_page);
             notebook.tab_moved.connect (on_tab_moved);
             notebook.tab_reordered.connect (on_tab_reordered);
+            notebook.tab_restored.connect (on_tab_restored);
             notebook.tab_duplicated.connect (on_tab_duplicated);
             notebook.close_tab_requested.connect (on_close_tab_requested);
             notebook.new_tab_requested.connect (on_new_tab_requested);
             notebook.allow_new_window = true;
             notebook.allow_duplication = true;
+            notebook.allow_restoring = true;
+            notebook.max_restorable_tabs = 5;
             notebook.group_name = "pantheon-terminal";
             notebook.can_focus = false;
             add (notebook);
@@ -218,6 +226,17 @@
                         }
 
                         break;
+                    case Gdk.Key.@D:
+                    case Gdk.Key.@d:
+                        if ((e.state & Gdk.ModifierType.CONTROL_MASK) != 0) {
+                            if (!current_terminal.has_foreground_process ()) {
+                                current_terminal.tab.close ();
+                                current_terminal.grab_focus ();
+                                return true;
+                            }
+                        }
+
+                        break;
                 }
 
                 return false;
@@ -291,7 +310,14 @@
                 }
             }
 
-            t.kill_ps ();
+            if (!t.child_has_exited) {
+                if (notebook.n_tabs >= 2) {
+                    make_restorable (tab);
+                } else {
+                    t.kill_ps ();
+                }
+            }
+
             if (notebook.n_tabs - 1 == 0) {
                 update_saved_window_state ();
                 reset_saved_tabs ();
@@ -304,6 +330,16 @@
             current_terminal.grab_focus ();
         }
 
+        private void on_tab_restored (string label, string restore_key, GLib.Icon? icon) {
+            TerminalWidget term = restorable_terminals.get (restore_key);
+            var tab = create_tab (label, icon, term);
+
+            restorable_terminals.remove (restore_key);
+            notebook.insert_tab (tab, -1);
+            notebook.current = tab;
+            term.grab_focus ();
+        }
+
         private void on_tab_moved (Granite.Widgets.Tab tab, int x, int y) {
             var new_window = app.new_window_with_coords (x, y, false);
             var terminal = (tab.page as Gtk.Grid).get_child_at (0, 0) as TerminalWidget;
@@ -407,10 +443,6 @@
             /* Set up terminal */
             var t = new TerminalWidget (this);
             t.scrollback_lines = settings.scrollback_lines;
-            var g = new Gtk.Grid ();
-            var sb = new Gtk.Scrollbar (Gtk.Orientation.VERTICAL, t.vadjustment);
-            g.attach (t, 0, 0, 1, 1);
-            g.attach (sb, 1, 0, 1, 1);
 
             /* Make the terminal occupy the whole GUI */
             t.vexpand = true;
@@ -426,14 +458,11 @@
                 t.run_program (program);
             }
 
-            var tab = new Granite.Widgets.Tab (_("Terminal"), null, g);
-
-            t.tab = tab;
-            tab.ellipsize_mode = Pango.EllipsizeMode.START;
+            var tab = create_tab (_("Terminal"), null, t);
 
             t.child_exited.connect (() => {
                 if (!t.killed) {
-                    tab.close ();
+                    t.tab.close ();
                 }
             });
 
@@ -450,6 +479,34 @@
             t.grab_focus ();
         }
 
+        private Granite.Widgets.Tab create_tab (string label, GLib.Icon? icon, TerminalWidget term) {
+            var g = new Gtk.Grid ();
+            var sb = new Gtk.Scrollbar (Gtk.Orientation.VERTICAL, term.vadjustment);
+            g.attach (term, 0, 0, 1, 1);
+            g.attach (sb, 1, 0, 1, 1);
+            var tab = new Granite.Widgets.Tab (label, icon, g);
+            term.tab = tab;
+            tab.ellipsize_mode = Pango.EllipsizeMode.START;
+
+            return tab;
+        }
+
+        private void make_restorable (Granite.Widgets.Tab tab) {
+            var restore_key = "%d".printf (restorable_counter++);
+            var page = tab.page as Gtk.Grid;
+            var term = page.get_child_at (0, 0) as TerminalWidget;
+
+            terminals.remove (term);
+            page.remove (term);
+            restorable_terminals.insert (restore_key, term);
+            tab.restore_data = restore_key;
+            tab.dropped_callback = (() => {
+                unowned TerminalWidget t = restorable_terminals.get (tab.restore_data);
+                t.kill_ps ();
+                restorable_terminals.remove (tab.restore_data);
+            });
+        }
+
         public void run_program_term (string program) {
             new_tab ("", program);
         }
@@ -494,6 +551,12 @@
             return false;
         }
 
+        private void on_destroy () {
+            foreach (unowned TerminalWidget t in restorable_terminals.get_values ()) {
+                t.kill_ps ();
+            }
+        }
+
         void action_quit () {
 
         }
@@ -516,6 +579,7 @@
 
         void action_close_tab () {
             current_terminal.tab.close ();
+            current_terminal.grab_focus ();
         }
 
         void action_new_window () {