Merge lp:~julien-spautz/pantheon-terminal/1157350 into lp:~elementary-apps/pantheon-terminal/trunk

[Julien Spautz]

This should fix bug #1157350 together with https://code.launchpad.net/~julien-spautz/granite/1157350/+merge/155123

[Julien Spautz (julien-spautz)]

And btw, the first part of the diff actually fixes bug #1059940

[David Gomes (davidgomes)]

So that's what I had to do, group_name, I've been looking for this for so long! Thank you Julien.

Anyways, the code style is approved, but this doesn't work so well.

Steps:
1. Open pantheon-terminal.
2. Open 3 tabs
3. In the middle tab type in "ls" or something
4. Select the middle tab and Open it in a New Window
5. The new window has a brand new TerminalWidget.

Same thing for dragging the window out. I realize this is better behavior than just a blank Pantheon Terminal, but it's still not right.

review: Needs Fixing

[lp:~julien-spautz/pantheon-terminal/1157350]

465. By Julien Spautz on 2013-03-24

just insert the tab, granite should not destroy it

[Julien Spautz (julien-spautz)]

One more bug, when dragging a tab to create a new window, and then dragging it back to the old window, the new one is empty but doesn't close.

[lp:~julien-spautz/pantheon-terminal/1157350]

466. By Julien Spautz on 2013-03-24

connect to tab_detached signal so we can destroy the notebook when it's empty

[Julien Spautz (julien-spautz)]

Seems to work now, don't forget I pushed a new revision to the granite branch, too.

Not sure if that's the way to go. DynamicNotebook is kind of a mess because it doesn't inherit from Gtk.Notebook, but owns it as a private member, so we have to manually propagate it's signals, which doesn't seem to be the best option…

[Julien Spautz (julien-spautz)]

Terminal still sometimes segfaults when closing a tab. Not sure why this happens.

[David Gomes (davidgomes)]

I can confirm the SEGFAULTs with both branches activated.

[Julien Spautz (julien-spautz)]

I've done some testing with DynamicNotebook in a different app but I couldn't reproduce the crashes, so it seems to be terminals fault.

The crashes in terminal always happen when closing a tab, and it seems that the tabs content is sometimes destroyed before the actual "tab_removed" signal. It also only happens when creating multiple windows, i.e., you can't crash terminal when setting "allow_new_window" to false and making the tabs non-detachable.

[David Gomes (davidgomes)]

I am not entirely sure about what I'm saying but a week or two ago the terminal never SEGFAULT'd, so it must have been the new Granite branches, right?

[David Gomes (davidgomes)]

I approve this.

review: Approve

[David Gomes (davidgomes)]

It needs approval actually, my bad I approved the wrong branch.

review: Needs Fixing

Unmerged revisions

466. By Julien Spautz on 2013-03-24

connect to tab_detached signal so we can destroy the notebook when it's empty

465. By Julien Spautz on 2013-03-24

just insert the tab, granite should not destroy it

464. By Julien Spautz on 2013-03-24

fix bug #1157350

=== modified file 'src/PantheonTerminalWindow.vala'
--- src/PantheonTerminalWindow.vala	2013-03-14 15:55:01 +0000
+++ src/PantheonTerminalWindow.vala	2013-03-24 16:53:20 +0000
@@ -151,10 +151,16 @@
             notebook.allow_new_window = true;
             notebook.allow_duplication = false;
             notebook.margin_top = 3;
-
+            notebook.group_name = "pantheon-terminal";
+            
             notebook.tab_added.connect ((tab) => {
                 new_tab ("", tab);
             });
+            
+            notebook.tab_detached.connect ((tab) => {
+                if (notebook.n_tabs == 0)
+                    this.destroy ();
+            });
 
             notebook.tab_removed.connect ((tab) => {
                 var t = ((tab.page as Gtk.Grid).get_child_at (0, 0) as TerminalWidget);
@@ -299,14 +305,9 @@
             if (new_window) {
                 app.new_window_with_coords (x, y, false);
                 var win = app.windows.last ().data;
-                //win.move (x, y);
-
-                var n = win.notebook;
-                //remove the one automatically created after inserting
-                n.insert_tab (tab, -1);
-                n.remove_tab (n.tabs.nth_data (1));
-
-                //notebook.remove_tab (tab);
+
+                win.notebook.insert_tab (tab, -1);
+
                 if (notebook.n_tabs == 0)
                     destroy ();
             } else {