Ubuntu UI Toolkit

Merge lp:~tpeeters/ubuntu-ui-toolkit/toolbar-reveal2 into lp:ubuntu-ui-toolkit

toolbar-reveal2
Merge into trunk

Proposed by Tim Peeters on 2013-09-09

Status:

Merged

Approved by:

Cris Dywan on 2013-10-22

Approved revision:

778

Merged at revision:

805

Proposed branch:

lp:~tpeeters/ubuntu-ui-toolkit/toolbar-reveal2

Merge into:

lp:ubuntu-ui-toolkit

Diff against target:

278 lines (+89/-43)

6 files modified

components.api (+1/-0)
modules/Ubuntu/Components/Toolbar.qml (+33/-16)
modules/Ubuntu/Components/ToolbarItems.qml (+4/-1)
tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py (+27/-16)
tests/unit/tst_components/tst_toolbaritems.qml (+4/-4)
tests/unit_x11/tst_components/tst_toolbar.qml (+20/-6)

To merge this branch:

bzr merge lp:~tpeeters/ubuntu-ui-toolkit/toolbar-reveal2

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Cris Dywan		2013-09-09	Approve on 2013-10-22
PS Jenkins bot	continuous-integration		Approve on 2013-10-21
Leo Arias (community)	code review		Needs Fixing on 2013-10-10
Review via email: mp+184678@code.launchpad.net

Commit message

Initially show toolbar, but automatically hide after timeout.

Description of the change

Update toolbar reveal/hide behavior, as specified by design. Initially show toolbar, but automatically hide after timeout.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-09:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-ui-toolkit-ci/626/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-11:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-ui-toolkit-ci/634/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-17:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-ui-toolkit-ci/663/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-09:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/925/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-10-10:

open_toolbar() in our emulators does the right thing, it only swipes in the toolbar when it is not already opened. So apps autopilot tests using that should be fine.

Revision history for this message

Leo Arias (elopio) wrote on 2013-10-10:

I think some emulator tests should be updated to take this into account.

test_toolbar_custom_emulator:

It does self.assertFalse(toolbar.opened) just after the application is opened. This now should be different.
You can change it to self.assertThat(toolbar.opened, Eventually(Equals(False))

On all the ToolbarTestCase tests, it would be better if we wait for the toolbar to hide.

You can add a setUp with: self.toolbar = self.main_view.get_toolbar(); self.toolbar.visible.wait_for(False). Then all the tests will be correct.

There might be a case where things can go wrong. What would happen if we check toolbar.visible while it's on the process of being hidden? If it is False just when it's starting to hide, I think the emulator will show it again properly without problems. If it's False until it's completely hidden, then the emulator will do nothing and we will see failures.

review: Needs Fixing (code review)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-10-10:

I don't think we should wait for the toolbar to be hidden. This is currently 5 seconds, but it can change depending on what design comes up with. Why not have no assumptions on whether it is opened, and open it when it is needed and not opened?

Revision history for this message

Leo Arias (elopio) wrote on 2013-10-10:

Yes, that's how the emulator works. But I'm referring to the self-tests we have on test_emulators.py.
For example, on test_open_toolbar, if we don't wait for the toolbar to be closed, we will not be testing the action that swipes up to open the toolbar. We will still have test_opened_toolbar_is_not_opened_again to check that it is still working as you mention. I'm not sure I'm making myself clear, maybe IRC would be easier :)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-10:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/940/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-10:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/941/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-11:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/953/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2013-10-11:

There's no test case for the new timeout property - it should be tested if it's meant to be used by apps, otherwise I would prefer to hide the API from apps or make it readonly.

I still see no testcase checking that the toolbar is truly hidden, though it was mentioned in the comments already. I'd say we want to check both API and the fact that it's not visible.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-21:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1020/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-21:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1024/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-21:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1026/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-21:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1028/rebuild

review: Needs Fixing (continuous-integration)

lp:~tpeeters/ubuntu-ui-toolkit/toolbar-reveal2 updated on 2013-10-21

776. By Tim Peeters on 2013-10-21: sleep() seems to stop the Timer. Use wait() in tests instead
777. By Tim Peeters on 2013-10-21: remove debugging code
778. By Tim Peeters on 2013-10-21: clean

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-21:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/1032/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2013-10-21:

I like the update!

Locally I'm getting 1 failure in ubuntuuitoolkit.tests.test_emulators.ToolbarTestCase.test_click_toolbar_button

MismatchError: String(u'Button not clicked.') != 'Button clicked.'

review: Needs Fixing

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-10-22:

For me the autopilot tests work fine on PC:

https://pastebin.canonical.com/99332/

Revision history for this message

Cris Dywan (kalikiana) wrote on 2013-10-22:

All pass with a new checkout in another folder. Well, seems good then.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Brian Blevins

Caco V. Topia

Jaroslavas Karmazinas

Tim Peeters

Ubuntu SDK team

to status/vote changes:

william

 === modified file 'components.api'
 --- components.api	2013-10-18 02:41:30 +0000
 +++ components.api	2013-10-21 17:30:28 +0000
@@ -357,6 +357,7 @@
  modules/Ubuntu/Components/Toolbar.qml
  Panel
      property Item tools
++    property int hideTimeout
  modules/Ubuntu/Components/ToolbarButton.qml
  ActionItem
  modules/Ubuntu/Components/ToolbarItems.qml
 === modified file 'modules/Ubuntu/Components/Toolbar.qml'
 --- modules/Ubuntu/Components/Toolbar.qml	2013-09-04 14:39:00 +0000
 +++ modules/Ubuntu/Components/Toolbar.qml	2013-10-21 17:30:28 +0000
@@ -39,34 +39,51 @@
       */
      property Item tools: null
++    /*!
++      \preliminary
++      The time in milliseconds before the toolbar automatically hides after inactivity
++      when it is not locked.
++     */
++    property int hideTimeout: 5000
++
      /*! \internal */
      onToolsChanged: {
--        if (tools && tools.hasOwnProperty("locked")) locked = tools.locked;
--        if (tools && tools.hasOwnProperty("locked") && tools.hasOwnProperty("opened")
--                && tools.opened && tools.locked) {
--            // toolbar is locked in visible state.
--            internal.updateVisibleTools();
--            toolbar.open();
--        } else if (!opened && !animating) {
--            // toolbar is closed
--            internal.updateVisibleTools();
--        } else {
--            toolbar.close()
--            // internal.visibleTools will be updated
--            // when the hide animation is finished
--        }
--        if (tools && tools.hasOwnProperty("opened")) {
--            tools.opened = toolbar.opened;
++        internal.updateVisibleTools();
++        if (tools) {
++            if (tools && tools.hasOwnProperty("locked")) locked = tools.locked;
++            // open the toolbar, except when it is locked in closed position
++            if (tools && tools.hasOwnProperty("locked") && tools.hasOwnProperty("opened")
++                    && !tools.opened && tools.locked) {
++                // toolbar is locked in closed state
++                toolbar.close();
++            } else {
++                toolbar.open();
++            }
++
++            if (tools && tools.hasOwnProperty("opened")) {
++                tools.opened = toolbar.opened;
++            }
++        } else { // no tools
++            locked = true;
++            toolbar.close();
+         }
+     }
      // if tools is not specified, lock the toolbar in closed position
      locked: tools && tools.hasOwnProperty("locked") ? tools.locked : false
++    Timer {
++        id: hideTimer
++        interval: toolbar.hideTimeout
++        running: toolbar.opened && !toolbar.locked
++        onTriggered: toolbar.close()
++    }
++
      onOpenedChanged: {
          if (tools && tools.hasOwnProperty("opened")) {
              tools.opened = toolbar.opened;
+         }
++        if (!toolbar.locked) hideTimer.restart()
+     }
      Connections {
 === modified file 'modules/Ubuntu/Components/ToolbarItems.qml'
 --- modules/Ubuntu/Components/ToolbarItems.qml	2013-10-14 16:37:43 +0000
 +++ modules/Ubuntu/Components/ToolbarItems.qml	2013-10-21 17:30:28 +0000
@@ -155,7 +155,10 @@
      property Item pageStack: null
      /*!
--      The toolbar is opened
++      The toolbar is opened.
++      When the toolbar is not locked, this value is automatically updated
++      when the toolbar is opened/closed by user interaction or by other events (such as changing
++      the active \l Page).
       */
      property bool opened: false
 === modified file 'tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py'
 --- tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py	2013-10-14 16:40:43 +0000
 +++ tests/autopilot/ubuntuuitoolkit/tests/test_emulators.py	2013-10-21 17:30:28 +0000
@@ -58,7 +58,10 @@
      def test_toolbar_custom_emulator(self):
          toolbar = self.main_view.get_toolbar()
          self.assertIsInstance(toolbar, emulators.Toolbar)
--        self.assertFalse(toolbar.opened)
++
++    def test_open_toolbar_returns_the_toolbar(self):
++        toolbar = self.main_view.open_toolbar()
++        self.assertIsInstance(toolbar, emulators.Toolbar)
      def test_get_tabs_without_tabs(self):
          error = self.assertRaises(
@@ -107,6 +110,11 @@
      width: units.gu(50)
      height: units.gu(50)
++    // Make sure that for these tests the toolbar starts closed.
++    Component.onCompleted: {
++        __propagated.toolbar.close();
++    }
++
      Page {
          Label {
@@ -124,32 +132,35 @@
                      onTriggered: label.text = "Button clicked."
+                 }
+             }
--            locked: false
--            opened: false
+         }
+     }
+ }
  """)
++    def setUp(self):
++        super(ToolbarTestCase, self).setUp()
++        self.toolbar = self.main_view.get_toolbar()
++        self.assertFalse(self.toolbar.opened)
++
      def test_open_toolbar(self):
--        toolbar = self.main_view.open_toolbar()
--        self.assertTrue(toolbar.opened)
--        self.assertFalse(toolbar.animating)
++        self.main_view.open_toolbar()
++        self.assertTrue(self.toolbar.opened)
++        self.assertFalse(self.toolbar.animating)
      def test_opened_toolbar_is_not_opened_again(self):
--        toolbar = self.main_view.open_toolbar()
++        self.main_view.open_toolbar()
          with mock.patch.object(
                  self.main_view.pointing_device, 'drag') as mock_drag:
              self.main_view.open_toolbar()
          self.assertFalse(mock_drag.called)
--        self.assertTrue(toolbar.opened)
++        self.assertTrue(self.toolbar.opened)
      def test_close_toolbar(self):
--        toolbar = self.main_view.open_toolbar()
++        self.main_view.open_toolbar()
          self.main_view.close_toolbar()
--        self.assertFalse(toolbar.opened)
--        self.assertFalse(toolbar.animating)
++        self.assertFalse(self.toolbar.opened)
++        self.assertFalse(self.toolbar.animating)
      def test_closed_toolbar_is_not_closed_again(self):
          with mock.patch.object(
@@ -157,19 +168,19 @@
              self.main_view.close_toolbar()
          self.assertFalse(mock_drag.called)
--        self.assertFalse(self.main_view.get_toolbar().opened)
++        self.assertFalse(self.toolbar.opened)
      def test_click_toolbar_button(self):
          label = self.app.select_single('Label', objectName='clicked_label')
          self.assertNotEqual(label.text, 'Button clicked.')
--        toolbar = self.main_view.open_toolbar()
--        toolbar.click_button('buttonName')
++        self.main_view.open_toolbar()
++        self.toolbar.click_button('buttonName')
          self.assertEqual(label.text, 'Button clicked.')
      def test_click_unexisting_button(self):
--        toolbar = self.main_view.open_toolbar()
++        self.main_view.open_toolbar()
          error = self.assertRaises(
--            ValueError, toolbar.click_button, 'unexisting')
++            ValueError, self.toolbar.click_button, 'unexisting')
          self.assertEqual(
              error.message, 'Button with objectName "unexisting" not found.')
 === modified file 'tests/unit/tst_components/tst_toolbaritems.qml'
 --- tests/unit/tst_components/tst_toolbaritems.qml	2013-09-03 10:51:40 +0000
 +++ tests/unit/tst_components/tst_toolbaritems.qml	2013-10-21 17:30:28 +0000
@@ -61,11 +61,11 @@
+         }
          function test_opened() {
--            compare(toolbarItems.opened, false, "Toolbar items initially closed");
++            compare(toolbarItems.opened, true, "Toolbar items initially opened");
++            toolbarItems.opened = false;
++            compare(toolbarItems.opened, false, "Toolbar items can be closed");
              toolbarItems.opened = true;
--            compare(toolbarItems.opened, true, "Toolbar items can be made opened");
--            toolbarItems.opened = false;
--            compare(toolbarItems.opened, false, "Toolbar items can be made closed");
++            compare(toolbarItems.opened, true, "Toolbar items can be opened");
+         }
          function test_locked() {
 === renamed file 'tests/unit/tst_components/tst_toolbar.qml' => 'tests/unit_x11/tst_components/tst_toolbar.qml'
 --- tests/unit/tst_components/tst_toolbar.qml	2013-09-09 19:27:26 +0000
 +++ tests/unit_x11/tst_components/tst_toolbar.qml	2013-10-21 17:30:28 +0000
@@ -19,14 +19,21 @@
  import Ubuntu.Components 0.1
  Item {
--    width: 200
--    height: 200
++    width: units.gu(50)
++    height: units.gu(80)
      MainView {
          anchors.fill: parent
++        width: units.gu(50)
++        height: units.gu(80)
          id: mainView
          Page {
              id: page
++            title: "test page"
++            Label {
++                anchors.centerIn: parent
++                text: "testing the toolbar"
++            }
              tools: ToolbarItems {
                  id: toolbarItems
                  ToolbarButton {
@@ -43,13 +50,12 @@
          function initTestCase() {
              compare(page.tools, toolbarItems, "Page tools are set initially");
              compare(page.__propagated, mainView.__propagated, "propagated property is propagated from mainView to page")
--            compare(mainView.__propagated.toolbar.tools, page.tools, "Toolbar tools are set to page tools initially");
--            compare(mainView.__propagated.toolbar.tools.opened, false, "Toolbar is closed initially");
--            compare(mainView.__propagated.toolbar.tools.locked, false, "Toolbar is initially not locked");
++            compare(mainView.__propagated.toolbar.tools, toolbarItems, "Toolbar tools are set to page tools initially");
++            compare(toolbarItems.opened, true, "Toolbar is opened initially");
++            compare(toolbarItems.locked, false, "Toolbar is initially not locked");
+         }
          function test_opened() {
--            compare(mainView.__propagated.toolbar.tools.opened, false, "Toolbar initially closed");
              mainView.__propagated.toolbar.open()
              compare(mainView.__propagated.toolbar.opened, true, "Toolbar can be made opened");
              mainView.__propagated.toolbar.close();
@@ -60,6 +66,14 @@
              compare(mainView.__propagated.toolbar.opened, false, "Toolbar can be made closed by setting page.tools.opened to false");
+         }
++        function test_hideTimeout() {
++            compare(mainView.__propagated.toolbar.hideTimeout, 5000, "Toolbar hide timeout is initially 5 seconds.");
++            mainView.__propagated.toolbar.open();
++            compare(mainView.__propagated.toolbar.opened, true, "Toolbar can be made opened");
++            wait(mainView.__propagated.toolbar.hideTimeout + 500); // add 500 ms margin
++            compare(mainView.__propagated.toolbar.opened, false, "Toolbar automatically closes after timeout");
++        }
++
          function test_locked() {
              compare(mainView.__propagated.toolbar.tools.locked, false, "Toolbar initially not locked");
              mainView.__propagated.toolbar.locked = true;