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

Proposed by Florian Boucault on 2017-03-07
Status: Approved
Approved by: Andrew Hayzen on 2017-03-22
Approved revision: 1629
Proposed branch: lp:~fboucault/webbrowser-app/history_domain_no_extra_sorting
Merge into: lp:webbrowser-app/staging
Diff against target: 165 lines (+38/-16)
4 files modified
src/app/webbrowser/HistoryView.qml (+2/-6)
src/app/webbrowser/history-domainlist-model.cpp (+27/-4)
src/app/webbrowser/history-domainlist-model.h (+3/-0)
tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp (+6/-6)
To merge this branch: bzr merge lp:~fboucault/webbrowser-app/history_domain_no_extra_sorting
Reviewer Review Type Date Requested Status
Andrew Hayzen (community) 2017-03-07 Approve on 2017-03-08
Review via email: mp+319199@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Andrew Hayzen (ahayzen) wrote :

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
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) wrote :

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) wrote :

> 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) wrote :

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

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/HistoryView.qml'
2--- src/app/webbrowser/HistoryView.qml 2016-06-06 09:09:47 +0000
3+++ src/app/webbrowser/HistoryView.qml 2017-03-08 12:26:13 +0000
4@@ -57,12 +57,8 @@
5 bottom: toolbar.top
6 }
7
8- model: SortFilterModel {
9- model: HistoryDomainListModel {
10- id: historyDomainListModel
11- }
12- sort.property: "lastVisit"
13- sort.order: Qt.DescendingOrder
14+ model: HistoryDomainListModel {
15+ id: historyDomainListModel
16 }
17
18 section.property: "lastVisitDate"
19
20=== modified file 'src/app/webbrowser/history-domainlist-model.cpp'
21--- src/app/webbrowser/history-domainlist-model.cpp 2015-10-28 21:50:08 +0000
22+++ src/app/webbrowser/history-domainlist-model.cpp 2017-03-08 12:26:13 +0000
23@@ -69,7 +69,7 @@
24 if (!index.isValid()) {
25 return QVariant();
26 }
27- const QString domain = m_domains.keys().at(index.row());
28+ const QString domain = m_domainsPerLastVisit.at(index.row());
29 HistoryDomainModel* entries = m_domains.value(domain);
30
31 switch (role) {
32@@ -117,11 +117,32 @@
33 }
34 }
35
36+QVariantMap HistoryDomainListModel::get(int row) const
37+{
38+ if (row < 0 || row >= rowCount()) {
39+ return QVariantMap();
40+ }
41+
42+ QVariantMap res;
43+ QHash<int,QByteArray> names = roleNames();
44+ QHashIterator<int, QByteArray> i(names);
45+
46+ while (i.hasNext()) {
47+ i.next();
48+ QModelIndex idx = index(row, 0);
49+ QVariant data = idx.data(i.key());
50+ res[i.value()] = data;
51+ }
52+
53+ return res;
54+}
55+
56 void HistoryDomainListModel::clearDomains()
57 {
58 Q_FOREACH(const QString& domain, m_domains.keys()) {
59 delete m_domains.take(domain);
60 }
61+ m_domainsPerLastVisit.clear();
62 }
63
64 void HistoryDomainListModel::populateModel()
65@@ -142,7 +163,7 @@
66 for (int i = start; i <= end; ++i) {
67 QString domain = getDomainFromSourceModel(m_sourceModel->index(i, 0, parent));
68 if (!m_domains.contains(domain)) {
69- QStringList domains = m_domains.keys();
70+ QStringList domains = m_domainsPerLastVisit;
71 int insertAt = 0;
72 while (insertAt < domains.count()) {
73 if (domain.compare(domains.at(insertAt)) < 0) {
74@@ -178,6 +199,7 @@
75 connect(model, SIGNAL(modelReset()), SLOT(onDomainDataChanged()));
76 connect(model, SIGNAL(lastVisitChanged()), SLOT(onDomainDataChanged()));
77 m_domains.insert(domain, model);
78+ m_domainsPerLastVisit.append(domain);
79 }
80
81 QString HistoryDomainListModel::getDomainFromSourceModel(const QModelIndex& index) const
82@@ -194,9 +216,10 @@
83 if (model != 0) {
84 const QString& domain = model->domain();
85 if (model->rowCount() == 0) {
86- int removeAt = m_domains.keys().indexOf(domain);
87+ int removeAt = m_domainsPerLastVisit.indexOf(domain);
88 beginRemoveRows(QModelIndex(), removeAt, removeAt);
89 delete m_domains.take(domain);
90+ m_domainsPerLastVisit.removeAt(removeAt);
91 endRemoveRows();
92 } else {
93 emitDataChanged(domain);
94@@ -214,7 +237,7 @@
95
96 void HistoryDomainListModel::emitDataChanged(const QString& domain)
97 {
98- int i = m_domains.keys().indexOf(domain);
99+ int i = m_domainsPerLastVisit.indexOf(domain);
100 if (i != -1) {
101 QModelIndex index = this->index(i, 0);
102 Q_EMIT dataChanged(index, index, QVector<int>() << LastVisit << LastVisitDate << LastVisitedTitle << LastVisitedIcon << Entries);
103
104=== modified file 'src/app/webbrowser/history-domainlist-model.h'
105--- src/app/webbrowser/history-domainlist-model.h 2015-10-28 21:50:08 +0000
106+++ src/app/webbrowser/history-domainlist-model.h 2017-03-08 12:26:13 +0000
107@@ -56,6 +56,8 @@
108 HistoryModel* sourceModel() const;
109 void setSourceModel(HistoryModel* sourceModel);
110
111+ Q_INVOKABLE QVariantMap get(int row) const;
112+
113 Q_SIGNALS:
114 void sourceModelChanged() const;
115
116@@ -69,6 +71,7 @@
117 private:
118 HistoryModel* m_sourceModel;
119 QMap<QString, HistoryDomainModel*> m_domains;
120+ QStringList m_domainsPerLastVisit;
121
122 void clearDomains();
123 void populateModel();
124
125=== modified file 'tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp'
126--- tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp 2015-10-28 21:50:08 +0000
127+++ tests/unittests/history-domainlist-model/tst_HistoryDomainListModelTests.cpp 2017-03-08 12:26:13 +0000
128@@ -89,14 +89,14 @@
129 QCOMPARE(args.at(1).toInt(), 0);
130 QCOMPARE(args.at(2).toInt(), 0);
131 QCOMPARE(model->rowCount(), 2);
132- QCOMPARE(model->data(model->index(0, 0), HistoryDomainListModel::Domain).toString(), QString("example.com"));
133+ QCOMPARE(model->data(model->index(1, 0), HistoryDomainListModel::Domain).toString(), QString("example.com"));
134
135 history->add(QUrl("http://example.org/test.html"), "Test page", QUrl());
136 QVERIFY(spyRowsInserted.isEmpty());
137 QVERIFY(!spyDataChanged.isEmpty());
138 args = spyDataChanged.takeFirst();
139- QCOMPARE(args.at(0).toModelIndex().row(), 1);
140- QCOMPARE(args.at(1).toModelIndex().row(), 1);
141+ QCOMPARE(args.at(0).toModelIndex().row(), 0);
142+ QCOMPARE(args.at(1).toModelIndex().row(), 0);
143 QCOMPARE(model->rowCount(), 2);
144 }
145
146@@ -171,7 +171,7 @@
147 QCOMPARE(model->rowCount(), 0);
148 }
149
150- void shouldKeepDomainsSorted()
151+ void shouldKeepDomainsSortedInsertionOrder()
152 {
153 history->add(QUrl("http://example.org/"), "Example Domain", QUrl());
154 history->add(QUrl("http://www.gogle.com/lawnmower"), "Gogle Lawn Mower", QUrl());
155@@ -183,8 +183,8 @@
156 history->add(QUrl("https://es.wikipedia.org/wiki/Wikipedia:Portada"), "Wikipedia, la enciclopedia libre", QUrl());
157 QCOMPARE(model->rowCount(), 6);
158 QStringList domains;
159- domains << DomainUtils::TOKEN_LOCAL << "example.com" << "example.org"
160- << "gogle.com" << "ubuntu.com" << "wikipedia.org";
161+ domains << "example.org" << "gogle.com" << "example.com" << "ubuntu.com"
162+ << DomainUtils::TOKEN_LOCAL << "wikipedia.org";
163 for (int i = 0; i < domains.count(); ++i) {
164 QModelIndex index = model->index(i, 0);
165 QString domain = model->data(index, HistoryDomainListModel::Domain).toString();

Subscribers

People subscribed via source and target branches