Merge lp:~gangsterveggies/pantheon-terminal/fix-bug-1166013 into lp:~elementary-apps/pantheon-terminal/trunk

[Pedro Paredes]

Fixed bug #1166013, to allow tabs to be hidden. Added a dconf-editor setting "tab-behavior" to set tab bar behavior.

Fixed bug #1166013, to allow tabs to be hidden. Added a dconf-editor setting to hide-tabs by default and added a right-click menu button to toggle tabs visibility.

[David Gomes (davidgomes)]

Good fix, good code, but the right-click I'm pretty sure is a no-no. I'm adding the ~elementary-design team, but we want to keep the terminal simple and we don't want people accidentally hitting the switch and going crazy. We'll keep it dconf-editor only.

review: Needs Fixing

[Pedro Paredes (gangsterveggies)]

> Good fix, good code, but the right-click I'm pretty sure is a no-no. I'm
> adding the ~elementary-design team, but we want to keep the terminal simple
> and we don't want people accidentally hitting the switch and going crazy.
> We'll keep it dconf-editor only.

Mmmm, you are probably right. I think another option it should have would be to hide tabs if there is only one tab. For example, I use terminator and by default I don't have tabs showing. If I add a tab the header shows up and it works fine. But that only works if we have a add tab button on the actual terminal and not the header. What do you think about this?

[David Gomes (davidgomes)]

>Mmmm, you are probably right. I think another option it should have would be to hide tabs if there is only one tab.

Has been discussed and it's not going to happen. We want the tabs always visible, we don't like bouncing interfaces. Plus, the Add button has to be visible at all times and we're not going to put an Add button on the terminal widget.

We'll just keep hide-tabs as a dconf-only variable as so many others.

[Pedro Paredes (gangsterveggies)]

> >Mmmm, you are probably right. I think another option it should have would be
> to hide tabs if there is only one tab.
>
> Has been discussed and it's not going to happen. We want the tabs always
> visible, we don't like bouncing interfaces. Plus, the Add button has to be
> visible at all times and we're not going to put an Add button on the terminal
> widget.
>
> We'll just keep hide-tabs as a dconf-only variable as so many others.

Ok, I guess in that case the "Toggle Tabs" button should come out then. I just feel like the menu is a bit empty, if you check menus from other terminals they have a lot of options and obviously I'm not saying we should make an impossibly long menu, but I feel like more options could help. It's just an opinion, but I get what you say.

[Danielle Foré (danrabbit)]

"Toggle Tabs" is not really an appropriate string in my opinion. "Show Tab Bar" and "Hide Tab Bar" would be much better.

Hiding the tabbar should probably not be possible when you have more than 1 tab open. (insensitive menu entry)
If you open a new tab (via shortcut for example), we should probably show the tabbar.

Basically if tabs > 1, always show the tabbar. Even if we'd previously chosen to hide it.

review: Needs Fixing

[Julián Unrrein (junrrein)]

What about the string being "Always Show Tab Bar"?

In this way, the UI can keep Daniel's proposed behavior, and the menu entry doesn't need to ever be insensitive, because the setting makes sense regardless of the number of open tabs.

[Pedro Paredes (gangsterveggies)]

> What about the string being "Always Show Tab Bar"?
>
> In this way, the UI can keep Daniel's proposed behavior, and the menu entry
> doesn't need to ever be insensitive, because the setting makes sense
> regardless of the number of open tabs.

I like your idea. I think it's the one that solves the problem the most elegantly. But what is the "final call" then? What should I implement?

[Akshay Shekher (voldyman)]

You should remove the menu item and leave it in the dconf. Tabs should be shown by default.

[Pedro Paredes (gangsterveggies)]

> You should remove the menu item and leave it in the dconf. Tabs should be
> shown by default.

Ok, I just did it.

[David Gomes (davidgomes)]

Have you considered that some people might want it to Never Show Tabs even when there is more than one tab open? That is why I recommend an enum with 3 modes:

Always Show Tabs - Default.
Show Tabs When More Than One Open
Never Show Tabs

Pedro told me on IRC he found this confusing, but it's not, it's just 3 modes and it's the only way to satisfy all users. Besides, only experienced (so called power users) people will mess with this setting on dconf so we know it won't confuse any simple user.

[David Gomes (davidgomes)]

If I put "hide-tabs" as true, when I load up the terminal won't be 80*24. It'll be slightly more than 80 per 24.

review: Needs Fixing

[Alex Lourie (alourie)]

Suggesting text change in schema:

<summary>Hide Tabs</summary>
<description>Hides terminal tabs if set to True</description>

[Pedro Paredes (gangsterveggies)]

> Have you considered that some people might want it to Never Show Tabs even
> when there is more than one tab open? That is why I recommend an enum with 3
> modes:
>
> Always Show Tabs - Default.
> Show Tabs When More Than One Open
> Never Show Tabs
>
> Pedro told me on IRC he found this confusing, but it's not, it's just 3 modes
> and it's the only way to satisfy all users. Besides, only experienced (so
> called power users) people will mess with this setting on dconf so we know it
> won't confuse any simple user.

I just pushed a revision with that behavior. Test it and tell me if it works. And check the texts on dconf because you might not like what I wrote.

> If I put "hide-tabs" as true, when I load up the terminal won't be 80*24. It'll be slightly more than 80 per 24.

I can't test it, can you send me a screen? (or two, one for each setting).

[David Gomes (davidgomes)]

>if (notebook.n_tabs == 2) {
Shouldn't this be >1? I know we always have to go through 2 to get to 3, but it's better if you put it >1.

review: Needs Fixing

[Pedro Paredes (gangsterveggies)]

> >if (notebook.n_tabs == 2) {
> Shouldn't this be >1? I know we always have to go through 2 to get to 3, but
> it's better if you put it >1.

Nope, the idea is that when you get back to a single tab you want to hide tabs. The == 2 is because this is done pre-removing a tab, thus you have 2 tabs and you are removing one of them. If you put it >1 when you have say four tabs and remove one it'd hide tabs, not what we want.

[David Gomes (davidgomes)]

Play with tab dragging. For example, if I have 2 tabs and I drag 1 out on "Hide when single tab", I get 1 tab and I still see the tab bar.

review: Needs Fixing

[Pedro Paredes (gangsterveggies)]

> Play with tab dragging. For example, if I have 2 tabs and I drag 1 out on
> "Hide when single tab", I get 1 tab and I still see the tab bar.

Ups, I forgot about that, nicely spotted. On it.

[David Gomes (davidgomes)]

review: Approve

[David Gomes (davidgomes)]

Rename tab-behavior to tab-bar-behavior and it's a merge.

review: Needs Fixing

[lp:~gangsterveggies/pantheon-terminal/fix-bug-1166013]

512. By Pedro Paredes on 2013-08-26

Changed tab_behavior to tab_bar_behavior

[Pedro Paredes (gangsterveggies)]

> Rename tab-behavior to tab-bar-behavior and it's a merge.

Done.

[David Gomes (davidgomes)]

review: Approve

=== modified file 'org.pantheon.terminal.gschema.xml'
--- org.pantheon.terminal.gschema.xml	2013-05-17 06:29:54 +0000
+++ org.pantheon.terminal.gschema.xml	2013-08-26 17:07:35 +0000
@@ -6,6 +6,12 @@
     <value nick="Fullscreen" value="2" />
   </enum>
 
+  <enum id="pantheon-terminal-tab-bar-behavior">
+    <value value="0" nick="Always Show Tabs"/>
+    <value value="1" nick="Hide When Single Tab"/>
+    <value value="2" nick="Never Show Tabs"/>
+  </enum>
+
   <schema path="/org/pantheon/terminal/saved-state/" 
   id="org.pantheon.terminal.saved-state" 
   gettext-domain="pantheon-terminal">
@@ -127,5 +133,10 @@
       <summary>Whether to allow bold fonts.</summary>
       <description>Defines whether fonts can be bold or not.</description>
     </key>
+    <key name="tab-bar-behavior" enum="pantheon-terminal-tab-bar-behavior">
+      <default>'Always Show Tabs'</default>
+      <summary>Tab Behavior</summary>
+      <description>Whether to always show tabs, hide tabs when there is only one tab and never show tabs.</description>
+    </key>
   </schema>
 </schemalist>

=== modified file 'src/PantheonTerminalWindow.vala'
--- src/PantheonTerminalWindow.vala	2013-08-18 21:03:49 +0000
+++ src/PantheonTerminalWindow.vala	2013-08-26 17:07:35 +0000
@@ -147,6 +147,15 @@
             /* Set up the Notebook */
             notebook = new Granite.Widgets.DynamicNotebook ();
             notebook.show_icons = false;
+            
+            if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.ALWAYS) {
+                notebook.show_tabs = true;
+            } else if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                notebook.show_tabs = false;
+            } else if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.NEVER) {
+                notebook.show_tabs = false;
+            }
+
             notebook.tab_switched.connect (on_switch_page);
             notebook.tab_moved.connect (on_tab_moved);
             notebook.allow_new_window = true;
@@ -177,6 +186,12 @@
 
                         d.destroy ();
 
+                        if (notebook.n_tabs == 2) {
+                            if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                                notebook.show_tabs = false;
+                            }
+                        }
+
                         return true;
                     }
 
@@ -207,6 +222,12 @@
                 closed_by_exit = false;
                 t.kill_ps ();
 
+                if (notebook.n_tabs == 2) {
+                    if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                        notebook.show_tabs = false;
+                    }
+                }
+
                 return true;
             });
 
@@ -325,8 +346,12 @@
             } else {
                 current_terminal.grab_focus ();
             }
-            if (notebook.n_tabs == 0)
+
+            if (notebook.n_tabs == 0) {
                 destroy ();
+            } else if (notebook.n_tabs == 1 && settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                notebook.show_tabs = false;
+            }
         }
 
         private void update_context_menu () {
@@ -379,6 +404,16 @@
             current_terminal = ((Grid) new_tab.page).get_child_at (0, 0) as TerminalWidget;
             title = current_terminal.window_title ?? "";
             new_tab.page.grab_focus ();
+
+            if (notebook.n_tabs == 1) {
+                if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                    notebook.show_tabs = false;
+                }
+            } else {
+                if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                    notebook.show_tabs = true;
+                }
+            }
         }
 
         private void open_tabs () {
@@ -412,6 +447,13 @@
             t.vexpand = true;
             t.hexpand = true;
 
+            /* If tabs were hidden and this is not the first tab show tabs */
+            if (notebook.n_tabs == 1) {
+                if (settings.tab_bar_behavior == PantheonTerminalTabBarBehavior.SINGLE) {
+                    notebook.show_tabs = true;
+                }
+            }
+            
             if (program == null) {
                 /* Set up the virtual terminal */
                 if (location == "")

=== modified file 'src/Settings.vala'
--- src/Settings.vala	2013-05-17 06:16:04 +0000
+++ src/Settings.vala	2013-08-26 17:07:35 +0000
@@ -29,6 +29,12 @@
         FULLSCREEN = 2
     }
 
+    public enum PantheonTerminalTabBarBehavior {
+        ALWAYS = 0,
+        SINGLE = 1,
+        NEVER = 2
+    }
+
     public class SavedState : Granite.Services.Settings {
 
         public int window_width { get; set; }
@@ -63,6 +69,7 @@
         public string encoding { get; set; }
         public string font { get; set; }
         public bool allow_bold { get; set; }
+        public PantheonTerminalTabBarBehavior tab_bar_behavior { get; set; }
 
         public Settings ()  {
             base ("org.pantheon.terminal.settings");