Merge lp:~fboucault/webbrowser-app/history_domain_no_extra_sorting into lp:webbrowser-app/staging

[Florian Boucault]

HistoryDomainListModel: preserve order of insertion which is de facto by 'lastVisit'. That makes it possible to not use an extra SortFilterModel in HistoryView which was slowing down loading dramatically when enough history is gathered.

HistoryDomainListModel: preserve order of insertion which is de facto by 'lastVisit'. That makes it possible to not use an extra SortFilterModel in HistoryView which was slowing down loading dramatically when enough history is gathered.

[Andrew Hayzen (ahayzen)]

I'm getting the following two failures with this branch that I wasn't getting in staging.

2: QWARN : QmlTests::HistoryView::test_delete_multiple_domains() file:///tmp/webbrowser/history_domain_no_extra_sorting/src/app/webbrowser/HistoryView.qml:179: TypeError: Property 'get' of object HistoryDomainListModel(0x146be40) is not a function
2: FAIL! : QmlTests::HistoryView::test_delete_multiple_domains() property selectMode
2: Actual (): true
2: Expected (): false
2: Loc: [/tmp/webbrowser/history_domain_no_extra_sorting/tests/unittests/qml/tst_HistoryView.qml(153)]

2: QWARN : QmlTests::HistoryView::test_select_all() file:///tmp/webbrowser/history_domain_no_extra_sorting/src/app/webbrowser/HistoryView.qml:179: TypeError: Property 'get' of object HistoryDomainListModel(0x1c73c70) is not a function
2: FAIL! : QmlTests::HistoryView::test_select_all() property count
2: Actual (): 3
2: Expected (): 0
2: Loc: [/tmp/webbrowser/history_domain_no_extra_sorting/tests/unittests/qml/tst_HistoryView.qml(190)]

Also note in both this branch and staging flake8 fails for me with the following, looks like a regression has slipped into staging. (this issue can be fixed in another branch or in this one)

1: /tmp/webbrowser/staging/tests/autopilot/webbrowser_app/tests/test_fullscreen.py:102:1: E302 expected 2 blank lines, found 1
1: /tmp/webbrowser/staging/tests/autopilot/webbrowser_app/tests/test_fullscreen.py:106:80: E501 line too long (82 > 79 characters)

review: Needs Fixing

[lp:~fboucault/webbrowser-app/history_domain_no_extra_sorting]

1628. By Florian Boucault on 2017-03-08

Merged from staging

1629. By Florian Boucault on 2017-03-08

Added HistoryDomainListModel::get: fixes unit tests HistoryView::test_delete_multiple_domains and HistoryView::test_select_all

[Andrew Hayzen (ahayzen)]

Doing some testing on a Nexus 4 (mako) :-)
I found the following speed improvements when opening the history view:

stock
1st time: 12 seconds
2nd time: 9.8 seconds
3rd time: 9.6 seconds

this branch
1st time: 8 seconds
2nd time: 6.3 seconds
3rd time: 6.2 seconds

So this branch has resulted in a reduction of 1/3 of the overall time :-)

[Florian Boucault (fboucault)]

> Doing some testing on a Nexus 4 (mako) :-)
> I found the following speed improvements when opening the history view:
>
> stock
> 1st time: 12 seconds
> 2nd time: 9.8 seconds
> 3rd time: 9.6 seconds
>
>
> this branch
> 1st time: 8 seconds
> 2nd time: 6.3 seconds
> 3rd time: 6.2 seconds
>
> So this branch has resulted in a reduction of 1/3 of the overall time :-)

Hmmm, not good enough

[Andrew Hayzen (ahayzen)]

LGTM, all unit tests now pass and functionally it looks good :-)

I have found one issue that looks like it is also in staging, so not created by this branch.
1) Open the history view
2) Scroll up/down, notice the scrolling works
3) Delete something from the history using the right click menu
4) Scroll up/down, notice the scrolling doesn't work, and if you click back in the header the scrolling is broken in the webview and you can't click to change tabs etc.

review: Approve

[flohack (flori-bin)]

UBports merged this here: https://github.com/ubports/webbrowser-component/commit/6164829c6ba06196a09c58d1d219137d02ce9289

Unmerged revisions

1629. By Florian Boucault on 2017-03-08

Added HistoryDomainListModel::get: fixes unit tests HistoryView::test_delete_multiple_domains and HistoryView::test_select_all

1628. By Florian Boucault on 2017-03-08

Merged from staging

1627. By Florian Boucault on 2017-03-07

HistoryDomainListModel: preserve order of insertion which is de facto by 'lastVisit'. That makes it possible to not use an extra SortFilterModel in HistoryView which was slowing down loading dramatically when enough history is gathered.

=== modified file 'src/app/webbrowser/HistoryView.qml'
--- src/app/webbrowser/HistoryView.qml	2016-06-06 09:09:47 +0000
+++ src/app/webbrowser/HistoryView.qml	2017-03-08 12:26:13 +0000
@@ -57,12 +57,8 @@
             bottom: toolbar.top
         }
 
-        model: SortFilterModel {
-            model: HistoryDomainListModel {
-                id: historyDomainListModel
-            }
-            sort.property: "lastVisit"
-            sort.order: Qt.DescendingOrder
+        model: HistoryDomainListModel {
+            id: historyDomainListModel
         }
 
         section.property: "lastVisitDate"
 
=== modified file 'src/app/webbrowser/history-domainlist-model.cpp'
--- src/app/webbrowser/history-domainlist-model.cpp	2015-10-28 21:50:08 +0000
+++ src/app/webbrowser/history-domainlist-model.cpp	2017-03-08 12:26:13 +0000
@@ -69,7 +69,7 @@
     if (!index.isValid()) {
         return QVariant();
     }
-    const QString domain = m_domains.keys().at(index.row());
+    const QString domain = m_domainsPerLastVisit.at(index.row());
     HistoryDomainModel* entries = m_domains.value(domain);
 
     switch (role) {
@@ -117,11 +117,32 @@
     }
 }
 
+QVariantMap HistoryDomainListModel::get(int row) const
+{
+    if (row < 0 || row >= rowCount()) {
+        return QVariantMap();
+    }
+
+    QVariantMap res;
+    QHash<int,QByteArray> names = roleNames();
+    QHashIterator<int, QByteArray> i(names);
+
+    while (i.hasNext()) {
+        i.next();
+        QModelIndex idx = index(row, 0);
+        QVariant data = idx.data(i.key());
+        res[i.value()] = data;
+    }
+
+    return res;
+}
+
 void HistoryDomainListModel::clearDomains()
 {
     Q_FOREACH(const QString& domain, m_domains.keys()) {
         delete m_domains.take(domain);
     }
+    m_domainsPerLastVisit.clear();
 }
 
 void HistoryDomainListModel::populateModel()
@@ -142,7 +163,7 @@
     for (int i = start; i <= end; ++i) {
         QString domain = getDomainFromSourceModel(m_sourceModel->index(i, 0, parent));
         if (!m_domains.contains(domain)) {
-            QStringList domains = m_domains.keys();
+            QStringList domains = m_domainsPerLastVisit;
             int insertAt = 0;
             while (insertAt < domains.count()) {
                 if (domain.compare(domains.at(insertAt)) < 0) {
@@ -178,6 +199,7 @@
     connect(model, SIGNAL(modelReset()), SLOT(onDomainDataChanged()));
     connect(model, SIGNAL(lastVisitChanged()), SLOT(onDomainDataChanged()));
     m_domains.insert(domain, model);
+    m_domainsPerLastVisit.append(domain);
 }
 
 QString HistoryDomainListModel::getDomainFromSourceModel(const QModelIndex& index) const
@@ -194,9 +216,10 @@
     if (model != 0) {
         const QString& domain = model->domain();
         if (model->rowCount() == 0) {
-            int removeAt = m_domains.keys().indexOf(domain);
+            int removeAt = m_domainsPerLastVisit.indexOf(domain);
             beginRemoveRows(QModelIndex(), removeAt, removeAt);
             delete m_domains.take(domain);
+            m_domainsPerLastVisit.removeAt(removeAt);
             endRemoveRows();
         } else {
             emitDataChanged(domain);
@@ -214,7 +237,7 @@
 
 void HistoryDomainListModel::emitDataChanged(const QString& domain)
 {
-    int i = m_domains.keys().indexOf(domain);
+    int i = m_domainsPerLastVisit.indexOf(domain);
     if (i != -1) {
         QModelIndex index = this->index(i, 0);
         Q_EMIT dataChanged(index, index, QVector<int>() << LastVisit << LastVisitDate << LastVisitedTitle << LastVisitedIcon << Entries);
 
=== modified file 'src/app/webbrowser/history-domainlist-model.h'
--- src/app/webbrowser/history-domainlist-model.h	2015-10-28 21:50:08 +0000
+++ src/app/webbrowser/history-domainlist-model.h	2017-03-08 12:26:13 +0000
@@ -56,6 +56,8 @@
     HistoryModel* sourceModel() const;
     void setSourceModel(HistoryModel* sourceModel);
 
+    Q_INVOKABLE QVariantMap get(int row) const;
+
 Q_SIGNALS:
     void sourceModelChanged() const;
 
@@ -69,6 +71,7 @@
 private:
     HistoryModel* m_sourceModel;
     QMap<QString, HistoryDomainModel*> m_domains;
+    QStringList m_domainsPerLastVisit;
 
     void clearDomains();
     void populateModel();
 
=== modified file 'tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp'
--- tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp	2015-10-28 21:50:08 +0000
+++ tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp	2017-03-08 12:26:13 +0000
@@ -89,14 +89,14 @@
         QCOMPARE(args.at(1).toInt(), 0);
         QCOMPARE(args.at(2).toInt(), 0);
         QCOMPARE(model->rowCount(), 2);
-        QCOMPARE(model->data(model->index(0, 0), HistoryDomainListModel::Domain).toString(), QString("example.com"));
+        QCOMPARE(model->data(model->index(1, 0), HistoryDomainListModel::Domain).toString(), QString("example.com"));
 
         history->add(QUrl("http://example.org/test.html"), "Test page", QUrl());
         QVERIFY(spyRowsInserted.isEmpty());
         QVERIFY(!spyDataChanged.isEmpty());
         args = spyDataChanged.takeFirst();
-        QCOMPARE(args.at(0).toModelIndex().row(), 1);
-        QCOMPARE(args.at(1).toModelIndex().row(), 1);
+        QCOMPARE(args.at(0).toModelIndex().row(), 0);
+        QCOMPARE(args.at(1).toModelIndex().row(), 0);
         QCOMPARE(model->rowCount(), 2);
     }
 
@@ -171,7 +171,7 @@
         QCOMPARE(model->rowCount(), 0);
     }
 
-    void shouldKeepDomainsSorted()
+    void shouldKeepDomainsSortedInsertionOrder()
     {
         history->add(QUrl("http://example.org/"), "Example Domain", QUrl());
         history->add(QUrl("http://www.gogle.com/lawnmower"), "Gogle Lawn Mower", QUrl());
@@ -183,8 +183,8 @@
         history->add(QUrl("https://es.wikipedia.org/wiki/Wikipedia:Portada"), "Wikipedia, la enciclopedia libre", QUrl());
         QCOMPARE(model->rowCount(), 6);
         QStringList domains;
-        domains << DomainUtils::TOKEN_LOCAL << "example.com" << "example.org"
-                << "gogle.com" << "ubuntu.com" << "wikipedia.org";
+        domains << "example.org" << "gogle.com" << "example.com" << "ubuntu.com"
+                << DomainUtils::TOKEN_LOCAL << "wikipedia.org";
         for (int i = 0; i < domains.count(); ++i) {
             QModelIndex index = model->index(i, 0);
             QString domain = model->data(index, HistoryDomainListModel::Domain).toString();