Merge lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 into lp:webbrowser-app

Proposed by Arthur Mello
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 1155
Merged at revision: 1148
Proposed branch: lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562
Merge into: lp:webbrowser-app
Diff against target: 359 lines (+148/-53)
6 files modified
src/app/webbrowser/Browser.qml (+0/-1)
src/app/webbrowser/HistoryViewWide.qml (+60/-36)
src/app/webbrowser/history-model.cpp (+31/-0)
src/app/webbrowser/history-model.h (+2/-0)
tests/unittests/history-model/tst_HistoryModelTests.cpp (+11/-0)
tests/unittests/qml/tst_HistoryViewWide.qml (+44/-16)
To merge this branch: bzr merge lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Olivier Tilloy Approve
Review via email: mp+268165@code.launchpad.net

Commit message

Add support for removing history entries with delete key in HistoryViewWide

Description of the change

Add support for removing history entries with delete key in HistoryViewWide

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1141. By Arthur Mello

Improve qml tests

1142. By Arthur Mello

Merge with trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

If I press Delete repeatedly to manually delete all history entries for a given day (just tried with Today), the corresponding date entry in the left panel disappears as expected, however the right panel isn’t updated, it remains empty, until I focus the left panel with the left arrow and move up/down.
When the last entry is deleted, the selection should probably be reset to All History.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1143. By Arthur Mello

Set left panel to "All History" when the last entry is deleted

1144. By Arthur Mello

Merge with trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

In the two new test functions, checking that the 'count' property of a spy is 0 right after calling clear() on that spy is useless. This is guaranteed to be true, by definition.

In onDeletePressed, if the left panel has active focus and the current index is 0 ("All History"), you could call historyModel.clearAll() instead of iterating through all the entries to delete them one by one.

Similarly, it might be interesting to add a 'clearByDate()' method to the history model to allow deleting all the entries for one given date, instead of iterating. This would be faster and cleaner.

review: Needs Fixing
1145. By Arthur Mello

Remove unnecessary checks

1146. By Arthur Mello

Use clearAll when deleting all entries from history

1147. By Arthur Mello

Add a removeEntriesByDate to the HistoryModel

1148. By Arthur Mello

Add unittests for the HistoryModel new method

1149. By Arthur Mello

Use signals to call changes on the history model to keep consistency

1150. By Arthur Mello

Add qml tests to HistoryViewWide

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

> 1149. By Arthur Mello 12 hours ago
> Use signals to call changes on the history model to keep consistency

Actually, I would have done the opposite: the history model is already exposed to the component, so there is no need to add signals (and the existing ones could be removed): calling methods on the model directly is absolutely fine, and makes the code easier to read. Unit tests won’t be able to monitor signal emissions, but they can still check that after deleting entries the count of the listviews is correctly updated.

Revision history for this message
Olivier Tilloy (osomon) wrote :

193 + dateTime.setTime(QTime(23, 59, 59));

The time should also include milliseconds (999).

review: Needs Fixing
1151. By Arthur Mello

Time used to rmeove entries from database should also include milliseconds

1152. By Arthur Mello

Merge with trunk

1153. By Arthur Mello

Stop using signals for changes on the HistoryModel

1154. By Arthur Mello

Update QML tests

Revision history for this message
Olivier Tilloy (osomon) wrote :

Not strictly necessary for tests to pass, but it would be better for the sake of completeness to re-implement the cleanup() method and have it clear historyMockModel.

1155. By Arthur Mello

Clean all history entries in the cleanup method

Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks, that looks good to me now.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/app/webbrowser/Browser.qml'
--- src/app/webbrowser/Browser.qml 2015-08-16 01:37:08 +0000
+++ src/app/webbrowser/Browser.qml 2015-08-18 13:59:06 +0000
@@ -830,7 +830,6 @@
830 browser.openUrlInNewTab(url, true)830 browser.openUrlInNewTab(url, true)
831 done()831 done()
832 }832 }
833 onHistoryEntryRemoved: browser.historyModel.removeEntryByUrl(url)
834 onNewTabRequested: browser.openUrlInNewTab("", true)833 onNewTabRequested: browser.openUrlInNewTab("", true)
835 onDone: historyViewLoader.active = false834 onDone: historyViewLoader.active = false
836 }835 }
837836
=== modified file 'src/app/webbrowser/HistoryViewWide.qml'
--- src/app/webbrowser/HistoryViewWide.qml 2015-08-11 21:05:38 +0000
+++ src/app/webbrowser/HistoryViewWide.qml 2015-08-18 13:59:06 +0000
@@ -28,11 +28,29 @@
2828
29 signal done()29 signal done()
30 signal historyEntryClicked(url url)30 signal historyEntryClicked(url url)
31 signal historyEntryRemoved(url url)
32 signal newTabRequested()31 signal newTabRequested()
3332
34 Keys.onLeftPressed: lastVisitDateListView.forceActiveFocus()33 Keys.onLeftPressed: lastVisitDateListView.forceActiveFocus()
35 Keys.onRightPressed: urlsListView.forceActiveFocus()34 Keys.onRightPressed: urlsListView.forceActiveFocus()
35 Keys.onDeletePressed: {
36 if (urlsListView.ViewItems.selectMode) {
37 internal.removeSelected()
38 } else {
39 if (urlsListView.activeFocus) {
40 historyViewWide.historyModel.removeEntryByUrl(urlsListView.currentItem.url)
41 if (urlsListView.count == 0) {
42 lastVisitDateListView.currentIndex = 0
43 }
44 } else {
45 if (lastVisitDateListView.currentIndex == 0) {
46 historyViewWide.historyModel.clearAll()
47 } else {
48 historyViewWide.historyModel.removeEntriesByDate(lastVisitDateListView.currentItem.lastVisitDate)
49 lastVisitDateListView.currentIndex = 0
50 }
51 }
52 }
53 }
3654
37 onActiveFocusChanged: {55 onActiveFocusChanged: {
38 if (activeFocus) {56 if (activeFocus) {
@@ -227,11 +245,9 @@
227 }245 }
228 246
229 onRemoved: {247 onRemoved: {
230 if (urlsListView.count == 1) {248 historyViewWide.historyModel.removeEntryByUrl(model.url)
231 historyViewWide.historyEntryRemoved(model.url)249 if (urlsListView.count == 0) {
232 lastVisitDateListView.currentIndex = 0250 lastVisitDateListView.currentIndex = 0
233 } else {
234 historyViewWide.historyEntryRemoved(model.url)
235 }251 }
236 }252 }
237253
@@ -311,19 +327,7 @@
311 iconName: "select"327 iconName: "select"
312 text: i18n.tr("Select all")328 text: i18n.tr("Select all")
313329
314 onClicked: {330 onClicked: internal.toggleSelectAll()
315 if (urlsListView.ViewItems.selectedIndices.length === urlsListView.count) {
316 urlsListView.ViewItems.selectedIndices = []
317 } else {
318 var indices = []
319 for (var i = 0; i < urlsListView.count; ++i) {
320 indices.push(i)
321 }
322 urlsListView.ViewItems.selectedIndices = indices
323 }
324
325 urlsListView.forceActiveFocus()
326 }
327 }331 }
328332
329 ToolbarAction {333 ToolbarAction {
@@ -342,24 +346,7 @@
342 iconName: "delete"346 iconName: "delete"
343 text: i18n.tr("Delete")347 text: i18n.tr("Delete")
344 enabled: urlsListView.ViewItems.selectedIndices.length > 0348 enabled: urlsListView.ViewItems.selectedIndices.length > 0
345 onClicked: {349 onClicked: internal.removeSelected()
346 var indices = urlsListView.ViewItems.selectedIndices
347 var urls = []
348 for (var i in indices) {
349 urls.push(urlsListView.model.get(indices[i])["url"])
350 }
351
352 if (urlsListView.count == urls.length) {
353 lastVisitDateListView.currentIndex = 0
354 }
355
356 urlsListView.ViewItems.selectMode = false
357 for (var j in urls) {
358 historyViewWide.historyEntryRemoved(urls[j])
359 }
360
361 lastVisitDateListView.forceActiveFocus()
362 }
363 }350 }
364351
365 ListItems.ThinDivider {352 ListItems.ThinDivider {
@@ -415,4 +402,41 @@
415 }402 }
416 }403 }
417 }404 }
405
406 QtObject {
407 id: internal
408
409 function toggleSelectAll() {
410 if (urlsListView.ViewItems.selectedIndices.length === urlsListView.count) {
411 urlsListView.ViewItems.selectedIndices = []
412 } else {
413 var indices = []
414 for (var i = 0; i < urlsListView.count; ++i) {
415 indices.push(i)
416 }
417 urlsListView.ViewItems.selectedIndices = indices
418 }
419
420 urlsListView.forceActiveFocus()
421 }
422
423 function removeSelected() {
424 var indices = urlsListView.ViewItems.selectedIndices
425 var urls = []
426 for (var i in indices) {
427 urls.push(urlsListView.model.get(indices[i])["url"])
428 }
429
430 if (urlsListView.count == urls.length) {
431 lastVisitDateListView.currentIndex = 0
432 }
433
434 urlsListView.ViewItems.selectMode = false
435 for (var j in urls) {
436 historyViewWide.historyModel.removeEntryByUrl(urls[j])
437 }
438
439 lastVisitDateListView.forceActiveFocus()
440 }
441 }
418}442}
419443
=== modified file 'src/app/webbrowser/history-model.cpp'
--- src/app/webbrowser/history-model.cpp 2015-07-14 22:16:41 +0000
+++ src/app/webbrowser/history-model.cpp 2015-08-18 13:59:06 +0000
@@ -309,6 +309,24 @@
309}309}
310310
311/*!311/*!
312 Remove all urls last visited in a given DATE from the history model.
313*/
314void HistoryModel::removeEntriesByDate(const QDate& date)
315{
316 if (!date.isValid()) {
317 return;
318 }
319
320 for (int i = m_entries.count() - 1; i >= 0; --i) {
321 if (m_entries.at(i).lastVisit.toLocalTime().date() == date) {
322 removeByIndex(i);
323 }
324 }
325 removeEntriesFromDatabaseByDate(date);
326 Q_EMIT rowCountChanged();
327}
328
329/*!
312 Remove all urls from a given DOMAIN from the history model.330 Remove all urls from a given DOMAIN from the history model.
313*/331*/
314void HistoryModel::removeEntriesByDomain(const QString& domain)332void HistoryModel::removeEntriesByDomain(const QString& domain)
@@ -396,6 +414,19 @@
396 query.exec();414 query.exec();
397}415}
398416
417void HistoryModel::removeEntriesFromDatabaseByDate(const QDate& date)
418{
419 QMutexLocker ml(&m_dbMutex);
420 QSqlQuery query(m_database);
421 static QString deleteStatement = QLatin1String("DELETE FROM history WHERE lastVisit BETWEEN ? AND ?;");
422 query.prepare(deleteStatement);
423 QDateTime dateTime = QDateTime(date);
424 query.addBindValue(dateTime.toTime_t());
425 dateTime.setTime(QTime(23, 59, 59, 999));
426 query.addBindValue(dateTime.toTime_t());
427 query.exec();
428}
429
399void HistoryModel::removeEntriesFromDatabaseByDomain(const QString& domain)430void HistoryModel::removeEntriesFromDatabaseByDomain(const QString& domain)
400{431{
401 QMutexLocker ml(&m_dbMutex);432 QMutexLocker ml(&m_dbMutex);
402433
=== modified file 'src/app/webbrowser/history-model.h'
--- src/app/webbrowser/history-model.h 2015-07-14 22:16:41 +0000
+++ src/app/webbrowser/history-model.h 2015-08-18 13:59:06 +0000
@@ -62,6 +62,7 @@
6262
63 Q_INVOKABLE int add(const QUrl& url, const QString& title, const QUrl& icon);63 Q_INVOKABLE int add(const QUrl& url, const QString& title, const QUrl& icon);
64 Q_INVOKABLE void removeEntryByUrl(const QUrl& url);64 Q_INVOKABLE void removeEntryByUrl(const QUrl& url);
65 Q_INVOKABLE void removeEntriesByDate(const QDate& date);
65 Q_INVOKABLE void removeEntriesByDomain(const QString& domain);66 Q_INVOKABLE void removeEntriesByDomain(const QString& domain);
66 Q_INVOKABLE void clearAll();67 Q_INVOKABLE void clearAll();
67 Q_INVOKABLE void hide(const QUrl& url);68 Q_INVOKABLE void hide(const QUrl& url);
@@ -99,6 +100,7 @@
99 void updateExistingEntryInDatabase(const HistoryEntry& entry);100 void updateExistingEntryInDatabase(const HistoryEntry& entry);
100 void removeEntryFromDatabaseByUrl(const QUrl& url);101 void removeEntryFromDatabaseByUrl(const QUrl& url);
101 void removeEntryFromHiddenDatabaseByUrl(const QUrl& url);102 void removeEntryFromHiddenDatabaseByUrl(const QUrl& url);
103 void removeEntriesFromDatabaseByDate(const QDate& date);
102 void removeEntriesFromDatabaseByDomain(const QString& domain);104 void removeEntriesFromDatabaseByDomain(const QString& domain);
103 void clearDatabase();105 void clearDatabase();
104};106};
105107
=== modified file 'tests/unittests/history-model/tst_HistoryModelTests.cpp'
--- tests/unittests/history-model/tst_HistoryModelTests.cpp 2015-04-15 13:27:23 +0000
+++ tests/unittests/history-model/tst_HistoryModelTests.cpp 2015-08-18 13:59:06 +0000
@@ -256,6 +256,17 @@
256 QCOMPARE(model->rowCount(), 0);256 QCOMPARE(model->rowCount(), 0);
257 }257 }
258258
259 void shouldRemoveByDate()
260 {
261 QCOMPARE(model->add(QUrl("http://example.org/"), "Example Domain", QUrl()), 1);
262 QCOMPARE(model->rowCount(), 1);
263 QCOMPARE(model->add(QUrl("http://example.com/"), "Example Domain", QUrl()), 1);
264 QCOMPARE(model->rowCount(), 2);
265
266 model->removeEntriesByDate(QDate::currentDate());
267 QCOMPARE(model->rowCount(), 0);
268 }
269
259 void shouldRemoveByDomain()270 void shouldRemoveByDomain()
260 {271 {
261 QCOMPARE(model->add(QUrl("http://example.org/page1"), "Example Domain Page 1", QUrl()), 1);272 QCOMPARE(model->add(QUrl("http://example.org/page1"), "Example Domain Page 1", QUrl()), 1);
262273
=== modified file 'tests/unittests/qml/tst_HistoryViewWide.qml'
--- tests/unittests/qml/tst_HistoryViewWide.qml 2015-08-11 16:16:07 +0000
+++ tests/unittests/qml/tst_HistoryViewWide.qml 2015-08-18 13:59:06 +0000
@@ -56,12 +56,6 @@
56 signalName: "historyEntryClicked"56 signalName: "historyEntryClicked"
57 }57 }
5858
59 SignalSpy {
60 id: historyEntryRemovedSpy
61 target: historyViewWide
62 signalName: "historyEntryRemoved"
63 }
64
65 UbuntuTestCase {59 UbuntuTestCase {
66 name: "HistoryViewWide"60 name: "HistoryViewWide"
67 when: windowShown61 when: windowShown
@@ -83,7 +77,7 @@
83 mouseRelease(item, center.x + 100, center.y, Qt.LeftButton, Qt.NoModifier, 2000)77 mouseRelease(item, center.x + 100, center.y, Qt.LeftButton, Qt.NoModifier, 2000)
84 }78 }
8579
86 function initTestCase() {80 function init() {
87 for (var i = 0; i < 3; ++i) {81 for (var i = 0; i < 3; ++i) {
88 historyMockModel.add("http://example.org/" + i, "Example Domain " + i, "")82 historyMockModel.add("http://example.org/" + i, "Example Domain " + i, "")
89 }83 }
@@ -92,11 +86,14 @@
92 waitForRendering(urlsList)86 waitForRendering(urlsList)
93 }87 }
9488
89 function cleanup() {
90 historyMockModel.clearAll()
91 }
92
95 function test_done_button() {93 function test_done_button() {
96 var doneButton = findChild(historyViewWide, "doneButton")94 var doneButton = findChild(historyViewWide, "doneButton")
97 verify(doneButton != null)95 verify(doneButton != null)
98 doneSpy.clear()96 doneSpy.clear()
99 compare(doneSpy.count, 0)
100 clickItem(doneButton)97 clickItem(doneButton)
101 compare(doneSpy.count, 1)98 compare(doneSpy.count, 1)
102 }99 }
@@ -106,8 +103,6 @@
106 verify(newTabButton != null)103 verify(newTabButton != null)
107 doneSpy.clear()104 doneSpy.clear()
108 newTabRequestedSpy.clear()105 newTabRequestedSpy.clear()
109 compare(doneSpy.count, 0)
110 compare(newTabRequestedSpy.count, 0)
111 clickItem(newTabButton)106 clickItem(newTabButton)
112 compare(newTabRequestedSpy.count, 1)107 compare(newTabRequestedSpy.count, 1)
113 compare(doneSpy.count, 1)108 compare(doneSpy.count, 1)
@@ -116,7 +111,7 @@
116 function test_history_entry_clicked() {111 function test_history_entry_clicked() {
117 var urlsList = findChild(historyViewWide, "urlsListView")112 var urlsList = findChild(historyViewWide, "urlsListView")
118 compare(urlsList.count, 3)113 compare(urlsList.count, 3)
119 compare(historyEntryClickedSpy.count, 0)114 historyEntryClickedSpy.clear()
120 clickItem(urlsList.children[0])115 clickItem(urlsList.children[0])
121 compare(historyEntryClickedSpy.count, 1)116 compare(historyEntryClickedSpy.count, 1)
122 var args = historyEntryClickedSpy.signalArguments[0]117 var args = historyEntryClickedSpy.signalArguments[0]
@@ -160,14 +155,14 @@
160 function test_delete_button() {155 function test_delete_button() {
161 var urlsList = findChild(historyViewWide, "urlsListView")156 var urlsList = findChild(historyViewWide, "urlsListView")
162 compare(urlsList.count, 3)157 compare(urlsList.count, 3)
158 var deletedUrl = urlsList.model.get(0).url
163 longPressItem(urlsList.children[0])159 longPressItem(urlsList.children[0])
164 var deleteButton = findChild(historyViewWide, "deleteButton")160 var deleteButton = findChild(historyViewWide, "deleteButton")
165 compare(historyEntryRemovedSpy.count, 0)
166 clickItem(deleteButton)161 clickItem(deleteButton)
167 compare(historyEntryRemovedSpy.count, 1)162 compare(urlsList.count, 2)
168 var args = historyEntryRemovedSpy.signalArguments[0]163 for (var i = 0; i < urlsList.count; ++i) {
169 var entry = urlsList.model.get(0)164 verify(urlsList.model.get(i).url != deletedUrl)
170 compare(args[0], entry.url)165 }
171 }166 }
172167
173 function test_keyboard_navigation_between_lists() {168 function test_keyboard_navigation_between_lists() {
@@ -180,5 +175,38 @@
180 keyClick(Qt.Key_Right)175 keyClick(Qt.Key_Right)
181 verify(urlsList.activeFocus) 176 verify(urlsList.activeFocus)
182 }177 }
178
179 function test_delete_key_at_urls_list_view() {
180 var urlsList = findChild(historyViewWide, "urlsListView")
181 keyClick(Qt.Key_Right)
182 verify(urlsList.activeFocus)
183 compare(urlsList.count, 3)
184 keyClick(Qt.Key_Delete)
185 compare(urlsList.count, 2)
186 }
187
188 function test_delete_key_at_last_visit_date() {
189 var lastVisitDateList = findChild(historyViewWide, "lastVisitDateListView")
190 var urlsList = findChild(historyViewWide, "urlsListView")
191 keyClick(Qt.Key_Left)
192 verify(lastVisitDateList.activeFocus)
193 compare(lastVisitDateList.currentIndex, 0)
194 keyClick(Qt.Key_Down)
195 compare(lastVisitDateList.currentIndex, 1)
196 compare(urlsList.count, 3)
197 keyClick(Qt.Key_Delete)
198 compare(urlsList.count, 0)
199 }
200
201 function test_delete_key_at_all_history() {
202 var lastVisitDateList = findChild(historyViewWide, "lastVisitDateListView")
203 var urlsList = findChild(historyViewWide, "urlsListView")
204 keyClick(Qt.Key_Left)
205 verify(lastVisitDateList.activeFocus)
206 compare(lastVisitDateList.currentIndex, 0)
207 compare(urlsList.count, 3)
208 keyClick(Qt.Key_Delete)
209 compare(urlsList.count, 0)
210 }
183 }211 }
184}212}

Subscribers

People subscribed via source and target branches

to status/vote changes: