Merge lp:~dpm/ubuntu-terminal-app/migrate-new-header into lp:ubuntu-terminal-app

[David Planella]

Migrate to the new header

Migrate to the new header.

Caveats:
- This is only the first cut. I'm not too convinced by the actions to show extra panels, which require an additional one to close them all. But this is the easiest path to migrate to the new panel, and if we need a redesign of the panels, it can be discussed and implemented as a separate MP.
- I'd like to show only the settings icon on the header, and the rest grouped in the list of actions. head.actions does not seem to give me any control over this, and shows always the 2 first actions as an icon, and the rest grouped.

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

113. By David Planella on 2014-08-10

Give back the objectName to the settings action

114. By David Planella on 2014-08-10

Removed obsolete pixmap icons

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:114
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/71/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1446
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1446/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-terminal-app-utopic-amd64-ci/15

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/71/rebuild

review: Needs Fixing (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

Action {
86 + objectName: "hidepanelaction"
87 + iconName: "edit-clear"
88 + text: i18n.tr("Hide all key panels")
89 + onTriggered: pgTerm.showExtraPanel(0)
90 + },

Shouldn't the clear button be shown only when a panel is shown? Otherwise it is a bit confusing what the button does where there are no panels open.

> and shows always the 2 first actions as an icon, and the rest grouped.

The header by default shows a maximum of 3 icons. So due to that in your case it shows the first 2 icons and then the 3rd one in the list. I don't think (nor believe) that a public API will be provided to control this.

review: Needs Fixing

[Nekhelesh Ramananthan (nik90)]

As you said we could decide on the redesign of the panels in another panel, just jotting down my idea here. Ideally it would be nice to show,

- Setting icon (visible always)
- Clear icon (conditionally visible)
- Panels Button to show popover for additional functions like the original implementation. This way it won't be grouped and also a popover seems like a good way to show this.

[Nekhelesh Ramananthan (nik90)]

93 + text: i18n.tr("Show control keys panel")
88 + text: i18n.tr("Hide all key panels")
103 + text: i18n.tr("Show arrow keys panel")
98 + text: i18n.tr("Show function keys panel")

Why the additional strings? Feel like "Hide Panels", "Control Keys", "Arrow Keys", "Function Keys" should the job. Either way with the long description they will be truncated (elipsed, cut of). Might as well shorten them ourselves.

review: Needs Fixing

[David Planella (dpm)]

On Sun, Aug 10, 2014 at 2:07 PM, Nekhelesh Ramananthan <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Action {
> 86 + objectName: "hidepanelaction"
> 87 + iconName: "edit-clear"
> 88 + text: i18n.tr("Hide all key panels")
> 89 + onTriggered: pgTerm.showExtraPanel(0)
> 90 + },
>
> Shouldn't the clear button be shown only when a panel is shown? Otherwise
> it is a bit confusing what the button does where there are no panels open.
>

Indeed, I agree it's not optimal, I'm just thinking of a first cut. But the
alternative of having it conditionally visible might not be too optimal, as
it would make the second action in the header change. I'll give it a go.

>
> > and shows always the 2 first actions as an icon, and the rest grouped.
>
> The header by default shows a maximum of 3 icons. So due to that in your
> case it shows the first 2 icons and then the 3rd one in the list. I don't
> think (nor believe) that a public API will be provided to control this.
>

Indeed, I noticed this, which seems to be undocumented. However, some
existing core apps which use the deprecated 'tools' property (see Calendar,
Reminders) manage to have only the first item visible and then the others
grouped, so I was wondering why the behaviour was different with
header.actions.

[David Planella (dpm)]

On Sun, Aug 10, 2014 at 2:15 PM, Nekhelesh Ramananthan <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> 93 + text: i18n.tr("Show control keys panel")
> 88 + text: i18n.tr("Hide all key panels")
> 103 + text: i18n.tr("Show arrow keys panel")
> 98 + text: i18n.tr("Show function keys panel")
>
> Why the additional strings? Feel like "Hide Panels", "Control Keys",
> "Arrow Keys", "Function Keys" should the job. Either way with the long
> description they will be truncated (elipsed, cut of). Might as well shorten
> them ourselves.
>

Thanks for the thorough review,btw!

I feel if it's an action it needs to tell the user exactly what the action
does. "Arrow keys" does not tell me anything until I click it, whereas I
will know exactly what "Show arrow keys panel" does and will be able to
decide if I want to click it or not, without surprises. In any case, the
shorter texts don't quite fit, either.

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

115. By David Planella on 2014-08-10

Made panels actions in the header conditionally visible, added icons to ensure the second action is visible

[David Planella (dpm)]

On Sun, Aug 10, 2014 at 2:13 PM, Nekhelesh Ramananthan <
<email address hidden>> wrote:

> As you said we could decide on the redesign of the panels in another
> panel, just jotting down my idea here. Ideally it would be nice to show,
>
> - Setting icon (visible always)

- Clear icon (conditionally visible)
>

Both done, thanks for the suggestion!

> - Panels Button to show popover for additional functions like the original
> implementation. This way it won't be grouped and also a popover seems like
> a good way to show this.
>

Not too sure about this one - in a way, we'd be overriding the default
behaviour of actionsby replacing them with a popup, just to work around the
bug whereby the space of the text in actions is limited. For now, I can
shorten the text.

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

116. By David Planella on 2014-08-10

Shortened action texts again to make them fit the available space

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:116
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/72/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1447
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1447/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-terminal-app-utopic-amd64-ci/16

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/72/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

117. By David Planella on 2014-08-10

Fixed autopilot tests to click the header actions instead of those in the bottom toolbar

[David Planella (dpm)]

Ok, fixed the tests now to use header actions instead of toolbar actions, but it seems they don't manage to close the extra panels (or to detect they are closed, can't figure out which).

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:117
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/73/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1450
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1450/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-terminal-app-utopic-amd64-ci/17

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/73/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

118. By David Planella on 2014-08-10

Fix autopilot tests

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:118
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/74/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1451/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-terminal-app-utopic-amd64-ci/18/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/74/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dpm/ubuntu-terminal-app/migrate-new-header]

119. By David Planella on 2014-08-10

pep8 tests are ludicrous

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:119
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/75/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1452
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1452/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-terminal-app-utopic-amd64-ci/19

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-terminal-app-ci/75/rebuild

review: Approve (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

There are several places where things could be improved as you mentioned. But for a first cut this is looking good. AP tests transition code looks good.

review: Approve

=== modified file 'src/app/qml/Terminal.qml'
--- src/app/qml/Terminal.qml	2014-04-27 09:11:14 +0000
+++ src/app/qml/Terminal.qml	2014-08-10 16:58:23 +0000
@@ -48,6 +48,36 @@
         }
     }
 
+    // FIXME: there is probably a better way to do this.
+    // For now, we're just keeping consistency with showExtraPanel()
+    function isExtraPanelVisible(i) {
+        var isPanelVisible = false;
+
+        switch (i) {
+            case 1:
+                isPanelVisible = kbCtrl.visible;
+                break;
+            case 2:
+                isPanelVisible = kbFn.visible;
+                break;
+            case 3:
+                isPanelVisible = kbScrl.visible;
+                break;
+            default:
+                isPanelVisible = false;
+        }
+
+        return isPanelVisible;
+    }
+
+    function areExtraPanelsVisible() {
+        if (kbCtrl.visible || kbFn.visible || kbScrl.visible) {
+            return true;
+        } else {
+            return false;
+        }
+    }
+
     Component.onCompleted: {
         pgConf.colorSchemes = kterm.availableColorSchemes();
 

=== removed file 'src/app/qml/[e-mail]'
Binary files src/app/qml/[e-mail]	2014-04-27 09:11:14 +0000 and src/app/qml/[e-mail]	1970-01-01 00:00:00 +0000 differ
=== removed file 'src/app/qml/settings.png'
Binary files src/app/qml/settings.png	2014-04-27 09:11:14 +0000 and src/app/qml/settings.png	1970-01-01 00:00:00 +0000 differ
=== modified file 'src/app/qml/ubuntu-terminal-app.qml'
--- src/app/qml/ubuntu-terminal-app.qml	2014-08-07 01:22:55 +0000
+++ src/app/qml/ubuntu-terminal-app.qml	2014-08-10 16:58:23 +0000
@@ -1,5 +1,5 @@
 import QtQuick 2.0
-import Ubuntu.Components 0.1
+import Ubuntu.Components 1.1
 import Ubuntu.Components.Popups 0.1
 
 
@@ -9,40 +9,11 @@
     objectName: "terminal"
     applicationName: "com.ubuntu.terminal"
     automaticOrientation: true
+    useDeprecatedToolbar: false
 
     width: units.gu(50)
     height: units.gu(75)
 
-    Component {
-        id: actionSelectionPopover
-
-        ActionSelectionPopover {
-            objectName: "panelpopover"
-            actions: ActionList {
-                Action {
-                    objectName: "controlkeysaction"
-                    text: i18n.tr("Control keys")
-                    onTriggered: pgTerm.showExtraPanel(1)
-                }
-                Action {
-                    objectName: "functionkeysaction"
-                    text: i18n.tr("Function keys")
-                    onTriggered: pgTerm.showExtraPanel(2)
-                }
-                Action {
-                    objectName: "textkeysaction"
-                    text: i18n.tr("Arrow keys")
-                    onTriggered: pgTerm.showExtraPanel(3)
-                }
-                Action {
-                    objectName: "hidepanelaction"
-                    text: i18n.tr("Hide extra panel")
-                    onTriggered: pgTerm.showExtraPanel(0)
-                }
-            }
-        }
-    }
-
     PageStack {
         id: pageStack
 
@@ -70,26 +41,42 @@
                 objectName: "pgTerm"
             }
 
-            tools: ToolbarItems {
-                ToolbarButton {
-                    id: toolbarAction
-                    objectName: "PanelsButton"
-                    action: Action {
-                        text: i18n.tr("Panels")
-                        iconSource: Qt.resolvedUrl("avatar.png")
-                        onTriggered: PopupUtils.open(actionSelectionPopover, toolbarAction)
-                    }
-                }
-                ToolbarButton {
-                    id: settingsButton
+            head.actions: [
+                Action {
                     objectName: "SettingsButton"
-                    action: Action {
-                        text: i18n.tr("Settings")
-                        iconSource: Qt.resolvedUrl("settings.png")
-                        onTriggered: pageStack.push(settingsPage)
-                    }
+                    iconName: "settings"
+                    text: i18n.tr("Settings")
+                    onTriggered: pageStack.push(settingsPage)
+                },
+                Action {
+                    objectName: "hidepanelaction"
+                    iconName: "edit-clear"
+                    text: i18n.tr("Hide all key panels")
+                    visible: pgTerm.areExtraPanelsVisible()
+                    onTriggered: pgTerm.showExtraPanel(0)
+                },
+                Action {
+                    objectName: "controlkeysaction"
+                    iconName: "browser-tabs"
+                    text: i18n.tr("Control keys")
+                    visible: !pgTerm.isExtraPanelVisible(1)
+                    onTriggered: pgTerm.showExtraPanel(1)
+                },
+                Action {
+                    objectName: "functionkeysaction"
+                    iconName: "browser-tabs"
+                    text: i18n.tr("Function keys")
+                    visible: !pgTerm.isExtraPanelVisible(2)
+                    onTriggered: pgTerm.showExtraPanel(2)
+                },
+                Action {
+                    objectName: "textkeysaction"
+                    iconName: "browser-tabs"
+                    text: i18n.tr("Arrow keys")
+                    visible: !pgTerm.isExtraPanelVisible(3)
+                    onTriggered: pgTerm.showExtraPanel(3)
                 }
-            }
+            ]
         }
 
         Page {

=== modified file 'tests/autopilot/ubuntu_terminal_app/tests/test_terminal.py'
--- tests/autopilot/ubuntu_terminal_app/tests/test_terminal.py	2014-07-18 10:37:26 +0000
+++ tests/autopilot/ubuntu_terminal_app/tests/test_terminal.py	2014-08-10 16:58:23 +0000
@@ -14,6 +14,7 @@
 from autopilot.platform import model
 
 from ubuntu_terminal_app.tests import TerminalTestCase, DbMan
+from ubuntuuitoolkit import ToolkitException as ToolkitException
 
 from time import sleep
 import unittest
@@ -32,19 +33,17 @@
 
     def hide_panels(self):
         """Click hide panels button"""
-        toolbar = self.main_view.open_toolbar()
-        timeout = 0
-        toolbar.click_button("PanelsButton")
-        panel_popover = self.main_view.get_panel_actions_popover()
-
-        # we have to poll the complex action because
-        # we can't check the animation for the header
-        while panel_popover is None and timeout < 10:
-            toolbar.click_button("PanelsButton")
-            panel_popover = self.main_view.get_panel_actions_popover()
-            sleep(1)
-            timeout += 1
-        panel_popover.click_button("Hide extra panel")
+        header = self.main_view.get_header()
+        header.click_action_button('hidepanelaction')
+        # Click on the button a second time in case that the
+        # actions list was open and the first click just closed it
+        # without further effect. If that was not the case, then
+        # be prepared to catch the exception, as once clicked on,
+        # the action to hide panels makes the button no longer visible
+        try:
+            header.click_action_button('hidepanelaction')
+        except ToolkitException:
+            pass
 
     def click_item_selector_item(self, selector, value):
         """Clicks item from item selector"""
@@ -79,11 +78,8 @@
     def test_control_panel(self):
         """Make sure that Control Keys Panel is visible
         when clicking the toolbar popup items and hides properly."""
-        toolbar = self.main_view.open_toolbar()
-        toolbar.click_button("PanelsButton")
-
-        panel_popover = self.main_view.get_panel_actions_popover()
-        panel_popover.click_button("Control keys")
+        header = self.main_view.get_header()
+        header.click_action_button('controlkeysaction')
 
         panelCtrl = lambda: self.main_view.get_control_panel().visible
         self.assertThat(panelCtrl, Eventually(Equals(True)))
@@ -93,11 +89,8 @@
     def test_function_panel(self):
         """Make sure that function Keys Panel is visible
         when clicking the toolbar popup items and hides properly."""
-        toolbar = self.main_view.open_toolbar()
-        toolbar.click_button("PanelsButton")
-
-        panel_popover = self.main_view.get_panel_actions_popover()
-        panel_popover.click_button("Function keys")
+        header = self.main_view.get_header()
+        header.click_action_button('functionkeysaction')
 
         panelFunc = lambda: self.main_view.get_function_panel().visible
         self.assertThat(panelFunc, Eventually(Equals(True)))
@@ -107,11 +100,8 @@
     def test_textctrl_panel(self):
         """Make sure that Text Control Keys Panel is visible
         when clicking the toolbar popup items and hides properly."""
-        toolbar = self.main_view.open_toolbar()
-        toolbar.click_button("PanelsButton")
-
-        panel_popover = self.main_view.get_panel_actions_popover()
-        panel_popover.click_button("Arrow keys")
+        header = self.main_view.get_header()
+        header.click_action_button('textkeysaction')
 
         panelScrl = lambda: self.main_view.get_scroll_panel().visible
         self.assertThat(panelScrl, Eventually(Equals(True)))
@@ -172,8 +162,8 @@
 
     def test_font_size_changes(self):
         """Make sure that font size is set correctly"""
-        toolbar = self.main_view.open_toolbar()
-        toolbar.click_button("SettingsButton")
+        header = self.main_view.get_header()
+        header.click_action_button('SettingsButton')
 
         # change font size to max
         self.main_view.drag_horizontal_slider("slFont", 32)