Merge into trunk : toolbar-tabs-deactivate : Code : Ubuntu UI Toolkit

Reviewer	Review Type	Status
Cris Dywan		Approve on 2013-11-04
PS Jenkins bot	continuous-integration	Approve on 2013-11-04
Tim Peeters		Approve on 2013-10-31
Review via email: mp+186065@code.staging.launchpad.net

Revision history for this message

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

#

FAILED: Continuous integration, rev:755
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/667/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy/3534/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1062/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/524/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/524/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/3546/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1064/console

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

review: Needs Fixing (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/669/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-09-18:

#

I downloaded and unzipped http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1082/artifact/work/output/*zip*/output.zip and installed the deb files on a maguro device that I flashed today with 'phablet-flash ubuntu-system' (which installed 20130916.3 image).

Tested with gallery-app, clock-app, notes-app and seems to work well. Tabs and toolbar are visible initially and will hide after a timeout, or when the user interacts with the app contents.

For proposed updates to the behavior, please leave a comment on this MR.

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-09-18:

#

After installing the packages above you should get the following new behaviors:
- Toolbar visible initially when starting app or changing views
- Toolbar automatically hides after a timeout, or when user interacts with app contents
- Tabs initially visible
- Tab bar automatically leaves selection mode (only current tab title visible) after timeout or when interacting with app contents

The following new behaviors will be implemented later:
- Hide toolbar when interacting with tabs
- Hide tabs when interacting with toolbar

Revision history for this message

Vesa Rautiainen (vesar) wrote on 2013-09-20:

#

We reviewed this MR in design team and here are our comments and issues we found:

Different timeouts in tabs and in toolbar
- Should have the same timeout value. Use the same as what launcher uses.

Showing toolbar when page changes doesn't work with Page Stack
- Happens in e.g. system settings app.
- Seems to work though when going back in stack. Then toolbar visibility is retained.

Re-launching an application: tabs should be expanded and toolbar open
- When opening an application that is in recent application category (stopped/suspended) toolbar is not shown and tabs are not expanded.
- This causes inconsistency between different app launches.
- So every time the app is launched/opened toolbar should be in open state and tabs in expanded state. In place. No animation. No animation because it would be too intrusive when going through app stack with right edge swipe.

Initial tabs scrolling animation in startup
- Sometimes when opening an app tabs are scrolling while expanding like a focused item would have been changed.
- It's not too bad animation and it even might be that we in design team proposed it. We just didn't take right edge back swipe in to account when proposing this. So let's take the animation away and have tabs in open state when application is launched.

Timeout timer doesn't restart when switching page from tabs
- this fails for example in Gallery, Weather and Calendar. In shorts (rss reader) app seems to work fine.
- To reproduce keep tapping on tabs. At some point toolbar hides quite quickly after page switch. It's a sign that timer is never restarted if running on page switch.
- Or is this something that hasn't been implemented because this will not be valid anymore when "tabs interaction closes toolbar" feature lands.

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

This MR is still valid, but should not be accepted yet. I will ask for a review when ready.

review: Needs Fixing

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-28

757. By Tim Peeters on 2013-10-28: merge trunk

Revision history for this message

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

#

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

review: Approve (continuous-integration)

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-29

758. By Tim Peeters on 2013-10-29: update tabbar timeout to match the timeout of toolbar and unity's launcher

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-30

759. By Tim Peeters on 2013-10-30: empty commit for CI

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-31

760. By Tim Peeters on 2013-10-31: fix bug that doesn't show toolbar the first time a page with no tools is pushed

Revision history for this message

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

#

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

review: Approve (continuous-integration)

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-31

761. By Tim Peeters on 2013-10-31: add SimpleTabs test program
762. By Tim Peeters on 2013-10-31: add SimpleTabs test program
763. By Tim Peeters on 2013-10-31: previous change: debugging
764. By Tim Peeters on 2013-10-31: reset toolbar hide timer when tools change

Revision history for this message

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

#

> We reviewed this MR in design team and here are our comments and issues we
> found:
>
> Different timeouts in tabs and in toolbar
> - Should have the same timeout value. Use the same as what launcher uses.

Ok, all timeouts are 5s now.

> Showing toolbar when page changes doesn't work with Page Stack
> - Happens in e.g. system settings app.
> - Seems to work though when going back in stack. Then toolbar visibility is
> retained.

It should work, but I noticed a bug where it does not work the first time a page is pushed to the stack. The bug was fixed in this MR.

> Re-launching an application: tabs should be expanded and toolbar open
> - When opening an application that is in recent application category
> (stopped/suspended) toolbar is not shown and tabs are not expanded.
> - This causes inconsistency between different app launches.
> - So every time the app is launched/opened toolbar should be in open state and
> tabs in expanded state. In place. No animation. No animation because it would
> be too intrusive when going through app stack with right edge swipe.

Agreed. I will work on that in a separate MR. I reported a bug to track the progress: https://bugs.launchpad.net/ubuntu-ux/+bug/1246790

>
> Initial tabs scrolling animation in startup
> - Sometimes when opening an app tabs are scrolling while expanding like a
> focused item would have been changed.
> - It's not too bad animation and it even might be that we in design team
> proposed it. We just didn't take right edge back swipe in to account when
> proposing this. So let's take the animation away and have tabs in open state
> when application is launched.

I agreed. I reported this bug https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1246792 and will fix it in a separate MR.

> Timeout timer doesn't restart when switching page from tabs
> - this fails for example in Gallery, Weather and Calendar. In shorts (rss
> reader) app seems to work fine.
> - To reproduce keep tapping on tabs. At some point toolbar hides quite quickly
> after page switch. It's a sign that timer is never restarted if running on
> page switch.
> - Or is this something that hasn't been implemented because this will not be
> valid anymore when "tabs interaction closes toolbar" feature lands.

Fixed.

Indeed, the fix may not be valid anymore when the "tabs interaction closes toolbar" feature lands, but I will think about that when implementing that feature.

> We reviewed this MR in design team and here are our comments and issues we
> found:
> 
> Different timeouts in tabs and in toolbar
> - Should have the same timeout value. Use the same as what launcher uses.

Ok, all timeouts are 5s now.
 
> Showing toolbar when page changes doesn't work with Page Stack
> - Happens in e.g. system settings app.
> - Seems to work though when going back in stack. Then toolbar visibility is
> retained.

It should work, but I noticed a bug where it does not work the first time a page is pushed to the stack. The bug was fixed in this MR.
 
> Re-launching an application: tabs should be expanded and toolbar open
> - When opening an application that is in recent application category
> (stopped/suspended) toolbar is not shown and tabs are not expanded.
> - This causes inconsistency between different app launches.
> - So every time the app is launched/opened toolbar should be in open state and
> tabs in expanded state. In place. No animation. No animation because it would
> be too intrusive when going through app stack with right edge swipe.

Agreed. I will work on that in a separate MR. I reported a bug to track the progress: https://bugs.launchpad.net/ubuntu-ux/+bug/1246790

> 
> Initial tabs scrolling animation in startup
> - Sometimes when opening an app tabs are scrolling while expanding like a
> focused item would have been changed.
> - It's not too bad animation and it even might be that we in design team
> proposed it. We just didn't take right edge back swipe in to account when
> proposing this. So let's take the animation away and have tabs in open state
> when application is launched.

I agreed. I reported this bug https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1246792 and will fix it in a separate MR.

> Timeout timer doesn't restart when switching page from tabs
> - this fails for example in Gallery, Weather and Calendar. In shorts (rss
> reader) app seems to work fine.
> - To reproduce keep tapping on tabs. At some point toolbar hides quite quickly
> after page switch. It's a sign that timer is never restarted if running on
> page switch.
> - Or is this something that hasn't been implemented because this will not be
> valid anymore when "tabs interaction closes toolbar" feature lands.

Fixed.

Indeed, the fix may not be valid anymore when the "tabs interaction closes toolbar" feature lands, but I will think about that when implementing that feature.

Revision history for this message

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

#

I think everything is ready now.

review: Approve

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-10-31

765. By Tim Peeters on 2013-10-31: merge trunk
766. By Tim Peeters on 2013-10-31: clean text

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

I'm worried by the fact that you didn't change any tests but they still pass. Even though your code looks sane it means it's completely untested.

review: Needs Fixing

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

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

#

> I'm worried by the fact that you didn't change any tests but they still pass.
> Even though your code looks sane it means it's completely untested.

There were some related MRs before this one that changed the tests for the Tabs, including checking whether the tab bar is active before switching tabs (by clicking a tab button), and only clicking the tab bar to activate it if it is not active yet.

So the description at the top describes changes of this MR including changes in previous MRs (they were ready a while ago but I couldn't merge them because of the jenkins issues we had back then. And I wanted to give this MR that included all the changes to design so they could have a look).

The main change that was left for this MR is to deactivate/close the tab bar and toolbar when the user interacts with the app contents (the page). I will add a test for that and let you know when it is ready.

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-11-04

767. By Tim Peeters on 2013-11-04: trying to make the tests for automatic timeout of activated tabbar
768. By Tim Peeters on 2013-11-04: fix bug in MainView
769. By Tim Peeters on 2013-11-04: remove debugging code
770. By Tim Peeters on 2013-11-04: fix test_deactivateByTimeout()
771. By Tim Peeters on 2013-11-04: add test

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-11-04:

#

I moved the tabs test to unit_x11, and added the test_deactivateByTimeout() and test_deactivateByAppInteraction() tests.

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-11-04

772. By Tim Peeters on 2013-11-04: fixed in r768 of this branch

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-11-04:

#

linked this bug that I fixed here: https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1244660

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

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

#

Your fix for bug 1244660 isn't covered by the tests. The following patch however does catch it:

=== modified file 'tests/unit/tst_mainview/tst_mainview.cpp'
--- tests/unit/tst_mainview/tst_mainview.cpp 2013-09-27 10:56:31 +0000
+++ tests/unit/tst_mainview/tst_mainview.cpp 2013-11-04 17:32:13 +0000
@@ -128,6 +128,9 @@ class tst_MainView : public QObject
}

     void testLocalStorage() {
+ QSignalSpy spy(view->engine(), SIGNAL(warnings(QList<QQmlError>)));
+ spy.setParent(view);
+
         QQuickItem *root = loadTest("LocalStorage.qml");
         QVERIFY(root);
         QQuickItem *mainView = root;
@@ -141,6 +144,9 @@ class tst_MainView : public QObject
         QString hash(QCryptographicHash::hash("pacific.island.tv", QCryptographicHash::Md5).toHex());
         QString database(databaseFolder + "/" + hash + ".sqlite");
         QVERIFY(QFile::exists(database));
+
+ // No warnings from QML
+ QCOMPARE(spy.count(), 0);
     }
};

review: Needs Fixing

Revision history for this message

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

#

Aside from the above the revamped tests look very nice!

Revision history for this message

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

#

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

review: Approve (continuous-integration)

lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate updated on 2013-11-04

773. By Tim Peeters on 2013-11-04: unfix bug. doing that in separate MR

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-11-04:

#

Removed bugfix and unlinked bug. It is not related to the other changes of this MR, so I'll do it in a separate MR.

Revision history for this message

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

#

Makes sense.

review: Approve

Ubuntu UI Toolkit

Merge lp://staging/~tpeeters/ubuntu-ui-toolkit/toolbar-tabs-deactivate into lp://staging/ubuntu-ui-toolkit

Commit message

Description of the change

Preview Diff

Subscribers