Merge lp:~stolowski/unity-scopes-shell/session-id into lp:unity-scopes-shell

[Paweł Stołowski]

Generate session_id (uuid) and pass it with search and preview request via SearchMetadata / ActionMetadata hints.

Generate session_id (uuid) and pass it with search and preview request via SearchMetadata / ActionMetadata hints. This requires https://code.launchpad.net/~stolowski/unity-scopes-api/session-id/+merge/233210 ; i'm assuming it lands together with other changes in scopes API, so it depends on scopes api 0.6.5.

The session id is re-created whenever a new search query is entered; appending or removing characters from existing search query keeps current session id.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:135
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/213/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-amd64-ci/110/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-armhf-ci/110/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-i386-ci/110/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/213/rebuild

review: Needs Fixing (continuous-integration)

[lp:~stolowski/unity-scopes-shell/session-id]

136. By Paweł Stołowski on 2014-09-03

Maintain and pass query_id.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:136
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/214/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-amd64-ci/111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-armhf-ci/111/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-i386-ci/111/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/214/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

99 + // don't regenerate it if currenty query appends to previous query or removes

currenty -> currently

100 + // caharcters from previous query.

caharcters -> characters

101 + if (m_session_id.isNull() ||
102 + m_searchQuery.isEmpty() || search_query.isEmpty() ||
103 + !(m_searchQuery.startsWith(search_query) ||
104 + search_query.startsWith(m_searchQuery))) {

this is quite hard to read, and I think its wrong. Rather do something like this:

bool search_empty = m_searchQuery.isEmpty() || search_query.isEmpty();

bool updates_previous = m_searchQuery.startsWith(search_query) || search_query.startsWith(m_searchQuery);

if((m_session_id.isNull() || search_empty) && !updates_previous)

review: Needs Fixing

[Marcus Tomlinson (marcustomlinson)]

Also could we add a simple test for this?

[lp:~stolowski/unity-scopes-shell/session-id]

137. By Paweł Stołowski on 2014-09-04

Refactored the conditional for re-creating session_id to be more readable.

138. By Paweł Stołowski on 2014-09-04

Added tests for session id in search metadata.

139. By Paweł Stołowski on 2014-09-04

Added test for passing session id with preview request.

[Paweł Stołowski (stolowski)]

> 99 + // don't regenerate it if currenty query appends to previous
> query or removes
>
> currenty -> currently
>
> 100 + // caharcters from previous query.
>
> caharcters -> characters
>
> 101 + if (m_session_id.isNull() ||
> 102 + m_searchQuery.isEmpty() || search_query.isEmpty() ||
> 103 + !(m_searchQuery.startsWith(search_query) ||
> 104 + search_query.startsWith(m_searchQuery))) {
>
> this is quite hard to read, and I think its wrong. Rather do something like
> this:
>
> bool search_empty = m_searchQuery.isEmpty() || search_query.isEmpty();
>
> bool updates_previous = m_searchQuery.startsWith(search_query) ||
> search_query.startsWith(m_searchQuery);
>
> if((m_session_id.isNull() || search_empty) && !updates_previous)

I think the logic in this conditional was correct though I agree it wasn't readable. The catch in it is that the startsWith check can only be made with non-empty strings (otherwise it yields true in cases where it shouldn't); I've refactored it a bit with helper variables.

[Paweł Stołowski (stolowski)]

> Also could we add a simple test for this?

Added.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:139
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/215/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-amd64-ci/112/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-armhf-ci/112/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-utopic-i386-ci/112/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/215/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Looks good, thanks!

review: Approve

=== modified file 'src/Unity/previewstack.cpp'
--- src/Unity/previewstack.cpp	2014-07-18 17:10:39 +0000
+++ src/Unity/previewstack.cpp	2014-09-04 09:34:32 +0000
@@ -31,6 +31,7 @@
 #include <QJsonDocument>
 #include <QJsonObject>
 #include <QJsonValue>
+#include <QUuid>
 
 #include <unity/scopes/ActionMetadata.h>
 #include <unity/scopes/Scope.h>
@@ -76,9 +77,10 @@
     return false;
 }
 
-void PreviewStack::setAssociatedScope(scopes_ng::Scope* scope)
+void PreviewStack::setAssociatedScope(scopes_ng::Scope* scope, QUuid const& session_id)
 {
     m_associatedScope = scope;
+    m_session_id = session_id;
 }
 
 void PreviewStack::loadForResult(scopes::Result::SPtr const& result)
@@ -119,6 +121,9 @@
         if (!extra_data.is_null()) {
             metadata.set_scope_data(extra_data);
         }
+        if (!m_session_id.isNull()) {
+            metadata["session-id"] = uuidToString(m_session_id).toStdString();
+        }
 
         std::shared_ptr<PreviewDataReceiver> listener(new PreviewDataReceiver(m_activePreview));
         std::weak_ptr<ScopeDataReceiverBase> wl(listener);

=== modified file 'src/Unity/previewstack.h'
--- src/Unity/previewstack.h	2014-05-19 09:58:14 +0000
+++ src/Unity/previewstack.h	2014-09-04 09:34:32 +0000
@@ -28,6 +28,7 @@
 #include <QSharedPointer>
 #include <QMultiMap>
 #include <QPointer>
+#include <QUuid>
 
 #include <unity/scopes/PreviewWidget.h>
 #include <unity/scopes/Result.h>
@@ -59,7 +60,7 @@
 
     void setWidgetColumnCount(int columnCount) override;
     int widgetColumnCount() const override;
-    void setAssociatedScope(scopes_ng::Scope*);
+    void setAssociatedScope(scopes_ng::Scope*, QUuid const&);
 
 private Q_SLOTS:
     void widgetTriggered(QString const&, QString const&, QVariantMap const&);
@@ -79,6 +80,7 @@
     std::shared_ptr<ScopeDataReceiverBase> m_lastActivation;
 
     unity::scopes::Result::SPtr m_previewedResult;
+    QUuid m_session_id;
 };
 
 } // namespace scopes_ng

=== modified file 'src/Unity/scope.cpp'
--- src/Unity/scope.cpp	2014-08-25 15:35:21 +0000
+++ src/Unity/scope.cpp	2014-09-04 09:34:32 +0000
@@ -54,6 +54,7 @@
 #include <unity/scopes/PreviewWidget.h>
 #include <unity/scopes/SearchMetadata.h>
 #include <unity/scopes/ActionMetadata.h>
+#include <unity/scopes/Variant.h>
 
 namespace scopes_ng
 {
@@ -67,6 +68,7 @@
 const int RESULTS_TTL_LARGE = 3600000; // 1 hour
 
 Scope::Scope(QObject *parent) : unity::shell::scopes::ScopeInterface(parent)
+    , m_query_id(0)
     , m_formFactor("phone")
     , m_isActive(false)
     , m_searchInProgress(false)
@@ -625,6 +627,10 @@
 
     if (m_proxy) {
         scopes::SearchMetadata meta(QLocale::system().name().toStdString(), m_formFactor.toStdString());
+        if (!m_session_id.isNull()) {
+            meta["session-id"] = uuidToString(m_session_id).toStdString();
+        }
+        meta["query-id"] = unity::scopes::Variant(m_query_id);
         if (m_settings) {
             QVariant remoteSearch(m_settings->get("remote-content-search"));
             if (remoteSearch.toString() == QString("none")) {
@@ -940,6 +946,21 @@
     */
 
     if (m_searchQuery.isNull() || search_query != m_searchQuery) {
+        // regenerate session id uuid if previous or current search string is empty or
+        // if current and previous query have no common prefix;
+        // don't regenerate it if current query appends to previous query or removes
+        // characters from previous query.
+        bool search_empty = m_searchQuery.isEmpty() || search_query.isEmpty();
+
+        // only check for common prefix if search is not empty
+        bool common_prefix = (!search_empty) && (m_searchQuery.startsWith(search_query) || search_query.startsWith(m_searchQuery));
+
+        if (m_session_id.isNull() || search_empty || !common_prefix) {
+            m_session_id = QUuid::createUuid();
+            m_query_id = 0;
+        } else {
+            ++m_query_id;
+        }
         m_searchQuery = search_query;
 
         // atm only empty query can have a filter state
@@ -1049,7 +1070,7 @@
     }
 
     PreviewStack* stack = new PreviewStack(nullptr);
-    stack->setAssociatedScope(this);
+    stack->setAssociatedScope(this, m_session_id);
     stack->loadForResult(result);
     return stack;
 }
@@ -1084,6 +1105,14 @@
     return m_resultsDirty;
 }
 
+QString Scope::sessionId() const {
+    return uuidToString(m_session_id);
+}
+
+int Scope::queryId() const {
+    return m_query_id;
+}
+
 void Scope::activateUri(QString const& uri)
 {
     /*

=== modified file 'src/Unity/scope.h'
--- src/Unity/scope.h	2014-08-25 15:35:21 +0000
+++ src/Unity/scope.h	2014-09-04 09:34:32 +0000
@@ -31,6 +31,7 @@
 #include <QMultiMap>
 #include <QSet>
 #include <QGSettings>
+#include <QUuid>
 
 // scopes
 #include <unity/scopes/ActivationResponse.h>
@@ -159,6 +160,9 @@
     bool resultsDirty() const;
     virtual unity::scopes::ScopeProxy proxy_for_result(unity::scopes::Result::SPtr const& result) const;
 
+    QString sessionId() const;
+    int queryId() const;
+
 public Q_SLOTS:
     void invalidateResults();
 
@@ -198,6 +202,8 @@
     static unity::scopes::Department::SCPtr findDepartmentById(unity::scopes::Department::SCPtr const& root, std::string const& id);
     static unity::scopes::Department::SCPtr findUpdateNode(DepartmentNode* node, unity::scopes::Department::SCPtr const& scopeNode);
 
+    QUuid m_session_id;
+    int m_query_id;
     QString m_searchQuery;
     QString m_noResultsHint;
     QString m_formFactor;

=== modified file 'src/Unity/utils.cpp'
--- src/Unity/utils.cpp	2014-03-25 16:34:26 +0000
+++ src/Unity/utils.cpp	2014-09-04 09:34:32 +0000
@@ -123,4 +123,14 @@
     }
 }
 
+Q_DECL_EXPORT QString uuidToString(QUuid const& uuid)
+{
+    // workaround: use mid to get rid of curly braces; see https://bugreports.qt-project.org/browse/QTBUG-885
+    auto const uuid_str = uuid.toString();
+    if (uuid_str.startsWith("{")) {
+        return uuid_str.mid(1, 36);
+    }
+    return uuid_str;
+}
+
 } // namespace scopes_ng

=== modified file 'src/Unity/utils.h'
--- src/Unity/utils.h	2014-03-25 16:34:26 +0000
+++ src/Unity/utils.h	2014-09-04 09:34:32 +0000
@@ -22,6 +22,7 @@
 
 // Qt
 #include <QVariant>
+#include <QUuid>
 
 #include <unity/scopes/Variant.h>
 
@@ -31,6 +32,7 @@
 Q_DECL_EXPORT QVariant scopeVariantToQVariant(unity::scopes::Variant const& variant);
 Q_DECL_EXPORT unity::scopes::Variant qVariantToScopeVariant(QVariant const& variant);
 Q_DECL_EXPORT QVariant backgroundUriToVariant(QString const& uri);
+Q_DECL_EXPORT QString uuidToString(QUuid const& uuid);
 
 } // namespace scopes_ng
 

=== modified file 'tests/data/mock-scope/mock-scope.cpp'
--- tests/data/mock-scope/mock-scope.cpp	2014-09-04 09:34:32 +0000
+++ tests/data/mock-scope/mock-scope.cpp	2014-09-04 09:34:32 +0000
@@ -233,6 +233,8 @@
             res.set_title("result for: \"" + query_ + "\"");
             res.set_art("art");
             res.set_dnd_uri("test:dnd_uri");
+            res["session-id"] = search_metadata()["session-id"].get_string();
+            res["query-id"] = Variant(search_metadata()["query-id"].get_int());
             res.set_intercept_activation();
             reply->push(res);
         }
@@ -326,13 +328,14 @@
         }
 
         PreviewWidgetList widgets;
-        PreviewWidget w1(R"({"id": "hdr", "type": "header", "components": {"title": "title", "subtitle": "uri", "attribute-1": "extra-data"}})");
+        PreviewWidget w1(R"({"id": "hdr", "type": "header", "components": {"title": "title", "subtitle": "uri", "attribute-1": "extra-data", "session-id": "session-id-val"}})");
         PreviewWidget w2(R"({"id": "img", "type": "image", "components": {"source": "art"}, "zoomable": false})");
         widgets.push_back(w1);
         widgets.push_back(w2);
         reply->push(widgets);
 
         reply->push("extra-data", Variant("foo"));
+        reply->push("session-id-val", Variant(action_metadata()["session-id"]));
     }
 
 private:

=== modified file 'tests/previewtest.cpp'
--- tests/previewtest.cpp	2014-09-04 09:34:32 +0000
+++ tests/previewtest.cpp	2014-09-04 09:34:32 +0000
@@ -115,6 +115,7 @@
         QCOMPARE(props[QString("title")].toString(), QString::fromStdString(result->title()));
         QCOMPARE(props[QString("subtitle")].toString(), QString::fromStdString(result->uri()));
         QCOMPARE(props[QString("attribute-1")].toString(), QString("foo"));
+        QCOMPARE(props[QString("session-id")].toString(), m_scope->sessionId());
 
         idx = preview_widgets->index(1);
         QCOMPARE(preview_widgets->data(idx, PreviewWidgetModel::RoleWidgetId).toString(), QString("img"));

=== modified file 'tests/resultstest.cpp'
--- tests/resultstest.cpp	2014-08-26 13:47:35 +0000
+++ tests/resultstest.cpp	2014-09-04 09:34:32 +0000
@@ -260,6 +260,167 @@
         QCOMPARE(results->data(idx, ResultsModel::Roles::RoleCategoryId), categories->data(categories->index(0), Categories::Roles::RoleCategoryId));
     }
 
+    void testSessionId()
+    {
+        std::string lastSessionId;
+
+        performSearch(m_scope, QString(""));
+
+        QVERIFY(!m_scope->sessionId().isEmpty());
+        QCOMPARE(m_scope->queryId(), 0);
+
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 0);
+
+            lastSessionId = sessionId;
+        }
+
+        // new search
+        performSearch(m_scope, QString("m"));
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 0);
+
+            // new session id
+            QVERIFY(sessionId != lastSessionId);
+
+            lastSessionId = sessionId;
+        }
+
+        // appends to previous search
+        performSearch(m_scope, QString("met"));
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 1);
+
+            // session id unchanged
+            QVERIFY(sessionId == lastSessionId);
+
+            lastSessionId = sessionId;
+        }
+
+        // removes characters from previous search
+        performSearch(m_scope, QString("me"));
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 2);
+
+            // session id unchanged
+            QVERIFY(sessionId == lastSessionId);
+
+            lastSessionId = sessionId;
+        }
+
+        // new non-empty search again
+        performSearch(m_scope, QString("iron"));
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 0);
+
+            // new session id
+            QVERIFY(sessionId != lastSessionId);
+
+            lastSessionId = sessionId;
+        }
+
+        // new empty search again
+        performSearch(m_scope, QString(""));
+        {
+            auto categories = m_scope->categories();
+            QVERIFY(categories->rowCount() > 0);
+            QVariant results_var = categories->data(categories->index(0), Categories::Roles::RoleResults);
+            auto results = results_var.value<ResultsModel*>();
+            QVERIFY(results->rowCount() > 0);
+
+            auto idx = results->index(0);
+            auto result = results->data(idx, ResultsModel::Roles::RoleResult).value<std::shared_ptr<unity::scopes::Result>>();
+
+            auto sessionId = (*result)["session-id"].get_string();
+            auto queryId = (*result)["query-id"].get_int();
+
+            // mock scope should send session-id and query-id it received back via custom result's values
+            QCOMPARE(sessionId, m_scope->sessionId().toStdString());
+            QCOMPARE(queryId, m_scope->queryId());
+            QCOMPARE(queryId, 0);
+
+            // new session id
+            QVERIFY(sessionId != lastSessionId);
+
+            lastSessionId = sessionId;
+        }
+    }
+
     void testResultMetadata()
     {
         performSearch(m_scope, QString("metadata"));