Merge lp:~cimi/unity8/fix-open-new-scope-from-tmp into lp:unity8

[Andrea Cimitan]

Fix temp scopes opening temp scopes

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~stolowski/unity-scopes-shell/fix-temp-scopes/+merge/248235
 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes
 * Did you make sure that your branch does not contain spurious tags?
Yes
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
n/a
 * If you changed the UI, has there been a design review?
n/a

[lp:~cimi/unity8/fix-open-new-scope-from-tmp]

1552. By Andrea Cimitan on 2015-02-04

whitespace

[Albert Astals Cid (aacid)]

I don't understand the MR, you're adding to fake_scopes three methods that you don't use and you're removing the code in closeScopes that checks that the scope you closed is indeed a scope that was opened.

Also what's the point of having the scopeThatOpenedScope variable now that you can simply use scopes.closeScope?

review: Needs Information

[Andrea Cimitan (cimi)]

> I don't understand the MR, you're adding to fake_scopes three methods that you
> don't use

future proof, if we wanted to test those, since they added API for this branch

and you're removing the code in closeScopes that checks that the
> scope you closed is indeed a scope that was opened.
>
that code is broken... m_openScope is never updated

> Also what's the point of having the scopeThatOpenedScope variable now that you
> can simply use scopes.closeScope?

I can switch to that too... but didn't want to move far from the previous implementation

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1552
http://jenkins.qa.ubuntu.com/job/unity8-ci/5235/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1161
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/399
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/400
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/400
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1024
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1159
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1159/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17754

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5235/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

> > I don't understand the MR, you're adding to fake_scopes three methods that
> you
> > don't use
>
> future proof, if we wanted to test those, since they added API for this branch

Hmmm, no, the only added api for the branch is
Q_INVOKABLE void closeScope(unity::shell::scopes::ScopeInterface* scope);
the others are private methods you'll never see

>
> and you're removing the code in closeScopes that checks that the
> > scope you closed is indeed a scope that was opened.
> >
> that code is broken... m_openScope is never updated

Broken where? after your changes? may be, then what about fixing it?

> > Also what's the point of having the scopeThatOpenedScope variable now that
> you
> > can simply use scopes.closeScope?
>
> I can switch to that too... but didn't want to move far from the previous
> implementation

Well, you made Pawel to add this new method and now you're not using it?

Also, why are you using
  dashTempScopeItem.scope.openScope(scopes.getScopeFromAll("MockScope9"));
?
Can you please make it work with a click that calls activate in the scope and thus ends up in Scope::activate that emits openScope?
That way we're properly exercising all the parts of the code

[Michał Sawicz (saviq)]

You need to depend on the new unity-api version, bump the API requirement in tests/mocks/Unity/CMakeLists.txt and bump unity-scopes-impl dep in debian/control.

review: Needs Fixing

[lp:~cimi/unity8/fix-open-new-scope-from-tmp]

1553. By Andrea Cimitan on 2015-02-11

Merged albert branch, more implementations on the mock

1554. By Andrea Cimitan on 2015-02-11

Moar fixes

1555. By Andrea Cimitan on 2015-02-12

More fixes

1556. By Andrea Cimitan on 2015-02-12

bump versions

1557. By Andrea Cimitan on 2015-02-12

remove console log

1558. By Andrea Cimitan on 2015-02-12

as review

1559. By Andrea Cimitan on 2015-02-12

As suggested

1560. By Andrea Cimitan on 2015-02-12

Small tweak

1561. By Andrea Cimitan on 2015-02-13

Bump version

1562. By Andrea Cimitan on 2015-02-13

merge el trunko

1563. By Andrea Cimitan on 2015-02-16

Fix testPreview

Unmerged revisions

=== modified file 'qml/Dash/Dash.qml'
--- qml/Dash/Dash.qml	2015-01-12 08:31:10 +0000
+++ qml/Dash/Dash.qml	2015-02-12 10:33:33 +0000
@@ -130,7 +130,7 @@
             UbuntuNumberAnimation {
                 onRunningChanged: {
                     if (!running && dashContent.x == 0) {
-                        scopeItem.scopeThatOpenedScope.closeScope(scopeItem.scope);
+                        scopes.closeScope(scopeItem.scope);
                         scopeItem.scope = null;
                     }
                 }
@@ -199,8 +199,7 @@
         }
     }
 
-    DashBackground
-    {
+    DashBackground {
         anchors.fill: scopeItem
         visible: scopeItem.visible
     }
@@ -229,7 +228,10 @@
                 dashContent.gotoScope(scopeId);
             }
             onOpenScope: {
-                dashContent.openScope(scope);
+                scopeItem.closePreview();
+                var oldScope = scopeItem.scope;
+                scopeItem.scope = scope;
+                scopes.closeScope(oldScope);
             }
         }
     }

=== modified file 'qml/Dash/DashContent.qml'
--- qml/Dash/DashContent.qml	2015-01-05 15:08:04 +0000
+++ qml/Dash/DashContent.qml	2015-02-12 10:33:33 +0000
@@ -84,8 +84,7 @@
 
         set_current_index = undefined;
 
-        if (dashContentList.count > index)
-        {
+        if (dashContentList.count > index) {
             dashContentList.currentIndex = index
 
             if (reset) {

=== modified file 'qml/Dash/Previews/Preview.qml'
--- qml/Dash/Previews/Preview.qml	2014-10-23 11:59:22 +0000
+++ qml/Dash/Previews/Preview.qml	2015-02-12 10:33:33 +0000
@@ -65,6 +65,7 @@
 
             delegate: ListView {
                 id: column
+                objectName: "previewListRow" + index
                 anchors { top: parent.top; bottom: parent.bottom }
                 width: row.columnWidth
                 spacing: row.spacing
@@ -89,6 +90,7 @@
                     }
 
                     onTriggered: {
+                        console.log(widgetId, actionId, data);
                         previewModel.triggered(widgetId, actionId, data);
                     }
 

=== modified file 'qml/Dash/Previews/PreviewActions.qml'
--- qml/Dash/Previews/PreviewActions.qml	2014-11-20 11:44:44 +0000
+++ qml/Dash/Previews/PreviewActions.qml	2015-02-12 10:33:33 +0000
@@ -55,6 +55,7 @@
             Connections {
                 target: loader.item
                 onTriggeredAction: {
+                    console.log(root.widgetId, actionData.id, actionData);
                     root.triggered(root.widgetId, actionData.id, actionData);
                 }
             }

=== modified file 'tests/mocks/Unity/fake_previewmodel.cpp'
--- tests/mocks/Unity/fake_previewmodel.cpp	2014-08-11 09:57:29 +0000
+++ tests/mocks/Unity/fake_previewmodel.cpp	2015-02-12 10:33:33 +0000
@@ -22,18 +22,23 @@
 #include "fake_previewmodel.h"
 
 // local
+#include "fake_scope.h"
+#include "fake_scopes.h"
 #include "fake_previewwidgetmodel.h"
 
 // Qt
 #include <QDebug>
 
-PreviewModel::PreviewModel(QObject* parent)
+PreviewModel::PreviewModel(QObject* parent, Scope* scope)
  : unity::shell::scopes::PreviewModelInterface(parent)
  , m_loaded(true)
+ , m_scope(scope)
 {
     // we have one column by default
     PreviewWidgetModel* columnModel = new PreviewWidgetModel(this);
     m_previewWidgetModels.append(columnModel);
+    connect(this, SIGNAL(triggered(QString const&, QString const&, QVariantMap const&)),
+            this, SLOT(triggeredSlot(QString const&, QString const&, QVariantMap const&)));
 }
 
 void PreviewModel::setWidgetColumnCount(int count)
@@ -80,3 +85,10 @@
         Q_EMIT loadedChanged();
     }
 }
+
+void PreviewModel::triggeredSlot(QString const&, QString const&, QVariantMap const&) {
+    Scopes *scopes = dynamic_cast<Scopes*>(m_scope->parent());
+    Scope* scope = scopes->getScopeFromAll("MockScope9");
+    scopes->addTempScope(scope);
+    Q_EMIT m_scope->openScope(scope);
+}

=== modified file 'tests/mocks/Unity/fake_previewmodel.h'
--- tests/mocks/Unity/fake_previewmodel.h	2014-08-11 09:57:29 +0000
+++ tests/mocks/Unity/fake_previewmodel.h	2015-02-12 10:33:33 +0000
@@ -25,12 +25,14 @@
 
 class PreviewWidgetModel;
 
+class Scope;
+
 class PreviewModel : public unity::shell::scopes::PreviewModelInterface
 {
     Q_OBJECT
 
 public:
-    explicit PreviewModel(QObject* parent = 0);
+    explicit PreviewModel(QObject* parent = 0, Scope* scope = 0);
 
     QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
     int rowCount(const QModelIndex& parent = QModelIndex()) const override;
@@ -42,9 +44,13 @@
 
     Q_INVOKABLE void setLoaded(bool); // Only available for testing
 
+private Q_SLOTS:
+    void triggeredSlot(QString const&, QString const&, QVariantMap const&);
+
 private:
     QList<PreviewWidgetModel*> m_previewWidgetModels;
     bool m_loaded;
+    Scope* m_scope;
 };
 
 Q_DECLARE_METATYPE(PreviewModel*)

=== modified file 'tests/mocks/Unity/fake_previewstack.cpp'
--- tests/mocks/Unity/fake_previewstack.cpp	2014-05-19 11:10:58 +0000
+++ tests/mocks/Unity/fake_previewstack.cpp	2015-02-12 10:33:33 +0000
@@ -21,10 +21,11 @@
 #include "fake_previewmodel.h"
 #include "fake_scope.h"
 
-PreviewStack::PreviewStack(QObject* parent)
+PreviewStack::PreviewStack(QObject* parent, Scope *scope)
  : unity::shell::scopes::PreviewStackInterface(parent)
+ , m_scope(scope)
 {
-    m_previews << new PreviewModel(this);
+    m_previews << new PreviewModel(this, m_scope);
 }
 
 PreviewStack::~PreviewStack()

=== modified file 'tests/mocks/Unity/fake_previewstack.h'
--- tests/mocks/Unity/fake_previewstack.h	2014-05-19 11:10:58 +0000
+++ tests/mocks/Unity/fake_previewstack.h	2015-02-12 10:33:33 +0000
@@ -25,12 +25,14 @@
 
 class PreviewModel;
 
+class Scope;
+
 class PreviewStack : public unity::shell::scopes::PreviewStackInterface
 {
     Q_OBJECT
 
 public:
-    explicit PreviewStack(QObject* parent = 0);
+    explicit PreviewStack(QObject* parent = 0, Scope* scope = 0);
     virtual ~PreviewStack();
 
     QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
@@ -43,6 +45,7 @@
 
 private:
     QList<PreviewModel*> m_previews;
+    Scope* m_scope;
 };
 
 Q_DECLARE_METATYPE(PreviewStack*)

=== modified file 'tests/mocks/Unity/fake_previewwidgetmodel.cpp'
--- tests/mocks/Unity/fake_previewwidgetmodel.cpp	2014-05-14 08:36:42 +0000
+++ tests/mocks/Unity/fake_previewwidgetmodel.cpp	2015-02-12 10:33:33 +0000
@@ -53,6 +53,17 @@
         PreviewData* preview_data = new PreviewData(QString("widget-%1").arg(i), QString("text"), attributes);
         m_previewWidgets.append(QSharedPointer<PreviewData>(preview_data));
     }
+
+    QVariantMap attributes;
+    QVariantMap buttonData;
+    buttonData["label"] = "Button";
+    buttonData["id"] = "open_click";
+    QVariantList buttons;
+    buttons << buttonData << buttonData << buttonData;
+    attributes["actions"] = QVariant::fromValue(buttons);
+    PreviewData* preview_data = new PreviewData(QString("widget-21"), QString("actions"), attributes);
+    m_previewWidgets.append(QSharedPointer<PreviewData>(preview_data));
+
     endResetModel();
 
 }

=== modified file 'tests/mocks/Unity/fake_scope.cpp'
--- tests/mocks/Unity/fake_scope.cpp	2014-12-15 12:39:00 +0000
+++ tests/mocks/Unity/fake_scope.cpp	2015-02-12 10:33:33 +0000
@@ -185,6 +185,7 @@
     if (result.toString() == "Result.2.2") {
         Scopes *scopes = dynamic_cast<Scopes*>(parent());
         m_openScope = scopes->getScopeFromAll("MockScope9");
+        scopes->addTempScope(m_openScope);
         Q_EMIT openScope(m_openScope);
     }
 }
@@ -198,7 +199,7 @@
     } else {
         // This probably leaks, do we don't care
         // it's a  test after all
-        return new PreviewStack;
+        return new PreviewStack(this, this);
     }
 }
 
@@ -208,11 +209,10 @@
 
 void Scope::closeScope(unity::shell::scopes::ScopeInterface* scope)
 {
-    if (scope != m_openScope) {
-        qDebug() << scope << m_openScope;
-        qFatal("Scope::closeScope got wrong scope in closeScope");
+    Scopes *scopes = dynamic_cast<Scopes*>(parent());
+    if (scopes) {
+        return scopes->closeScope(scope);
     }
-    m_openScope = nullptr;
 }
 
 QString Scope::currentNavigationId() const

=== modified file 'tests/mocks/Unity/fake_scopes.cpp'
--- tests/mocks/Unity/fake_scopes.cpp	2014-12-16 09:03:37 +0000
+++ tests/mocks/Unity/fake_scopes.cpp	2015-02-12 10:33:33 +0000
@@ -23,6 +23,7 @@
 // TODO: Implement remaining pieces, like Categories (i.e. LensView now gives warnings)
 
 // Qt
+#include <QDebug>
 #include <QTimer>
 
 Scopes::Scopes(QObject *parent)
@@ -177,6 +178,19 @@
     return m_scopesOverview;
 }
 
+void Scopes::addTempScope(unity::shell::scopes::ScopeInterface* scope)
+{
+    if (!m_tempScopes.contains(scope)) {
+        m_tempScopes.insert(scope);
+    }
+}
+
+void Scopes::closeScope(unity::shell::scopes::ScopeInterface* scope)
+{
+    Q_ASSERT(m_tempScopes.contains(scope));
+    m_tempScopes.remove(scope);
+}
+
 void Scopes::setFavorite(const QString& scopeId, bool favorite)
 {
     if (favorite) {

=== modified file 'tests/mocks/Unity/fake_scopes.h'
--- tests/mocks/Unity/fake_scopes.h	2014-12-10 09:07:14 +0000
+++ tests/mocks/Unity/fake_scopes.h	2015-02-12 10:33:33 +0000
@@ -26,6 +26,7 @@
 
 // Qt
 #include <QList>
+#include <QSet>
 #include <QTimer>
 
 class ScopesOverview;
@@ -59,6 +60,10 @@
     Q_INVOKABLE void setFavorite(const QString& scopeId, bool favorite) override;
     Q_INVOKABLE void moveFavoriteTo(const QString& scopeId, int index) override;
 
+    unity::shell::scopes::ScopeInterface* findTempScope(QString const& id) const;
+    void addTempScope(unity::shell::scopes::ScopeInterface* scope);
+    Q_INVOKABLE void closeScope(unity::shell::scopes::ScopeInterface* scope);
+
     // This is used as part of implementation of the other C++ code, not API
     QList<Scope*> favScopes() const;
     QList<Scope*> nonFavScopes() const;
@@ -70,6 +75,7 @@
 private:
     QList<Scope*> m_scopes; // the favorite ones
     QList<Scope*> m_allScopes;
+    QSet<unity::shell::scopes::ScopeInterface*> m_tempScopes;
     ScopesOverview *m_scopesOverview;
     bool m_loaded;
     QTimer timer;

=== modified file 'tests/mocks/Unity/fake_scopesoverview.cpp'
--- tests/mocks/Unity/fake_scopesoverview.cpp	2014-12-16 09:03:37 +0000
+++ tests/mocks/Unity/fake_scopesoverview.cpp	2015-02-12 10:33:33 +0000
@@ -48,6 +48,7 @@
         Q_EMIT gotoScope(result.toString());
     } else {
         m_openScope = scopes->getScopeFromAll(result.toString());
+        scopes->addTempScope(m_openScope);
         Q_EMIT openScope(m_openScope);
     }
 }

=== modified file 'tests/qmltests/Dash/Previews/tst_Preview.qml'
--- tests/qmltests/Dash/Previews/tst_Preview.qml	2014-07-04 16:20:48 +0000
+++ tests/qmltests/Dash/Previews/tst_Preview.qml	2015-02-12 10:33:33 +0000
@@ -54,6 +54,11 @@
         name: "Preview"
         when: windowShown
 
+        function init() {
+            var widget = findChild(preview, "previewListRow0");
+            widget.positionViewAtBeginning();
+        }
+
         function test_triggered() {
             waitForRendering(preview);
             var widget = findChild(preview, "widget-3");
@@ -102,5 +107,31 @@
                 return bottomLeft.y <= preview.height
             }, true);
         }
+
+        function test_comboEnsureVisible() {
+            waitForRendering(preview);
+
+            // Scroll down
+            var previewListRow0 = findChild(preview, "previewListRow0");
+            touchFlick(preview, preview.width / 2, units.gu(20), preview.width / 2, units.gu(1));
+            tryCompare(previewListRow0, "atYEnd", true);
+            tryCompare(previewListRow0, "moving", false);
+
+            // Click on the combo
+            var widget = findChild(preview, "widget-21");
+            var initialWidgetHeight = widget.height;
+            var moreLessButton = findChild(widget, "moreLessButton");
+            mouseClick(moreLessButton);
+
+            // Make sure the combo is growing
+            tryCompareFunction(function () { return widget.height > 2 * initialWidgetHeight; }, true);
+
+            // Wait for the combo to stop growing
+            tryCompareFunction(function () { var currentWidgetHeight = widget.height; wait(200); return currentWidgetHeight === widget.height;}, true);
+
+            // Make sure the combo bottom is on the viewport
+            var bottomLeft = preview.mapFromItem(widget, 0, widget.height);
+            verify(bottomLeft.y <= preview.height);
+        }
     }
 }

=== modified file 'tests/qmltests/Dash/tst_Dash.qml'
--- tests/qmltests/Dash/tst_Dash.qml	2015-01-09 10:42:42 +0000
+++ tests/qmltests/Dash/tst_Dash.qml	2015-02-12 10:33:33 +0000
@@ -428,5 +428,70 @@
             tryCompare(bottomEdgeController, "progress", 0);
             tryCompare(dashContentList, "currentIndex", 1)
         }
+
+        function test_close_temp_scope_preview_opening_scope() {
+            // Show the manage dash
+            touchFlick(dash, dash.width / 2, dash.height - 1, dash.width / 2, units.gu(2));
+            var bottomEdgeController = findInvisibleChild(dash, "bottomEdgeController");
+            tryCompare(bottomEdgeController, "progress", 1);
+
+            // Make sure stuff is loaded
+            var nonfavScopesListCategory = findChild(dash, "scopesListCategoryother");
+            var nonfavScopesListCategoryList = findChild(nonfavScopesListCategory, "scopesListCategoryInnerList");
+            tryCompare(nonfavScopesListCategoryList, "currentIndex", 0);
+
+            // Click on a non favorite scope
+            mouseClick(nonfavScopesListCategoryList.currentItem);
+
+            // Check the bottom edge (manage dash) is disabled from temp scope
+            var overviewDragHandle = findChild(dash, "overviewDragHandle");
+            compare(overviewDragHandle.enabled, false);
+
+            // Check temp scope is there
+            var dashTempScopeItem = findChild(dash, "dashTempScopeItem");
+            tryCompare(dashTempScopeItem, "x", 0);
+            tryCompare(dashTempScopeItem, "visible", true);
+
+            // Check the manage dash is gone
+            tryCompare(bottomEdgeController, "progress", 0);
+
+            // Open preview
+            var categoryListView = findChild(dashTempScopeItem, "categoryListView");
+            categoryListView.positionAtBeginning();
+            tryCompareFunction(function() {
+                                    var cardGrid = findChild(dashTempScopeItem, "dashCategory0");
+                                    if (cardGrid != null) {
+                                        var tile = findChild(cardGrid, "delegate0");
+                                        return tile != null;
+                                    }
+                                    return false;
+                                },
+                                true);
+            var tile = findChild(findChild(dashTempScopeItem, "dashCategory0"), "delegate0");
+            waitForRendering(tile);
+            mouseClick(tile);
+            var subPageLoader = findChild(dashTempScopeItem, "subPageLoader");
+            tryCompare(subPageLoader, "open", true);
+            tryCompare(subPageLoader, "x", 0);
+            tryCompare(findChild(dashTempScopeItem, "categoryListView"), "visible", false);
+            var previewListRow0 = findChild(subPageLoader, "previewListRow0");
+            touchFlick(previewListRow0, previewListRow0.width / 2, units.gu(20), previewListRow0.width / 2, units.gu(1));
+            tryCompare(previewListRow0, "atYEnd", true);
+            tryCompare(previewListRow0, "moving", false);
+            var widget = findChild(subPageLoader, "widget-21");
+            var initialWidgetHeight = widget.height;
+            var openButton = findChild(widget, "buttonopen_click");
+            mouseClick(openButton);
+
+            tryCompare(subPageLoader, "open", false);
+            compare(dashTempScopeItem.scope.id, "MockScope9");
+
+            // Go back
+            dashTempScopeItem.backClicked();
+
+            // Check temp scope is gone
+            tryCompare(dashTempScopeItem, "x", dash.width);
+            tryCompare(dashTempScopeItem, "visible", false);
+        }
     }
 }

=== modified file 'tests/qmltests/Dash/tst_GenericScopeView.qml'
--- tests/qmltests/Dash/tst_GenericScopeView.qml	2015-01-09 09:15:45 +0000
+++ tests/qmltests/Dash/tst_GenericScopeView.qml	2015-02-12 10:33:33 +0000
@@ -380,8 +380,8 @@
 
                 tryCompare(testCase.subPageLoader, "open", false);
 
+                tryCompareFunction(function() { return findChild(category, "delegate1") != null; }, true);
                 var tile = findChild(category, "delegate1");
-                verify(tile, "Could not find delegate");
 
                 mouseClick(tile);
                 tryCompare(testCase.subPageLoader, "open", true);