webbrowser-app

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

webbrowser-app-fix_1484562
Merge into trunk

Proposed by Arthur Mello on 2015-08-15

Status:

Merged

Approved by:

Olivier Tilloy on 2015-08-18

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

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-08-18
Olivier Tilloy		2015-08-15	Approve on 2015-08-18
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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2101/rebuild

review: Needs Fixing (continuous-integration)

lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-16

1141. By Arthur Mello on 2015-08-16: Improve qml tests
1142. By Arthur Mello on 2015-08-16: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2102/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-08-17:

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 on 2015-08-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2107/rebuild

review: Needs Fixing (continuous-integration)

lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-17

1143. By Arthur Mello on 2015-08-17: Set left panel to "All History" when the last entry is deleted
1144. By Arthur Mello on 2015-08-17: Merge with trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2110/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-08-17:

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

lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-17

1145. By Arthur Mello on 2015-08-17: Remove unnecessary checks
1146. By Arthur Mello on 2015-08-17: Use clearAll when deleting all entries from history
1147. By Arthur Mello on 2015-08-17: Add a removeEntriesByDate to the HistoryModel
1148. By Arthur Mello on 2015-08-17: Add unittests for the HistoryModel new method
1149. By Arthur Mello on 2015-08-17: Use signals to call changes on the history model to keep consistency
1150. By Arthur Mello on 2015-08-17: Add qml tests to HistoryViewWide

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2114/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-08-18:

> 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 on 2015-08-18:

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

The time should also include milliseconds (999).

review: Needs Fixing

lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-18

1151. By Arthur Mello on 2015-08-18: Time used to rmeove entries from database should also include milliseconds
1152. By Arthur Mello on 2015-08-18: Merge with trunk
1153. By Arthur Mello on 2015-08-18: Stop using signals for changes on the HistoryModel
1154. By Arthur Mello on 2015-08-18: Update QML tests

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-08-18:

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.

lp:~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-18

1155. By Arthur Mello on 2015-08-18: Clean all history entries in the cleanup method

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-08-18:

Thanks, that looks good to me now.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2121/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Arthur Mello

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === 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 @@
                      browser.openUrlInNewTab(url, true)
                      done()
+                 }
--                onHistoryEntryRemoved: browser.historyModel.removeEntryByUrl(url)
                  onNewTabRequested: browser.openUrlInNewTab("", true)
                  onDone: historyViewLoader.active = false
+             }
 === 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 @@
      signal done()
      signal historyEntryClicked(url url)
--    signal historyEntryRemoved(url url)
      signal newTabRequested()
      Keys.onLeftPressed: lastVisitDateListView.forceActiveFocus()
      Keys.onRightPressed: urlsListView.forceActiveFocus()
++    Keys.onDeletePressed: {
++        if (urlsListView.ViewItems.selectMode) {
++            internal.removeSelected()
++        } else {
++            if (urlsListView.activeFocus) {
++                historyViewWide.historyModel.removeEntryByUrl(urlsListView.currentItem.url)
++                if (urlsListView.count == 0) {
++                    lastVisitDateListView.currentIndex = 0
++                }
++            } else {
++                if (lastVisitDateListView.currentIndex == 0) {
++                    historyViewWide.historyModel.clearAll()
++                } else {
++                    historyViewWide.historyModel.removeEntriesByDate(lastVisitDateListView.currentItem.lastVisitDate)
++                    lastVisitDateListView.currentIndex = 0
++                }
++            }
++        }
++    }
      onActiveFocusChanged: {
          if (activeFocus) {
@@ -227,11 +245,9 @@
+                     }
                      onRemoved: {
--                        if (urlsListView.count == 1) {
--                            historyViewWide.historyEntryRemoved(model.url)
++                        historyViewWide.historyModel.removeEntryByUrl(model.url)
++                        if (urlsListView.count == 0) {
                              lastVisitDateListView.currentIndex = 0
--                        } else {
--                            historyViewWide.historyEntryRemoved(model.url)
+                         }
+                     }
@@ -311,19 +327,7 @@
              iconName: "select"
              text: i18n.tr("Select all")
--            onClicked: {
--                if (urlsListView.ViewItems.selectedIndices.length === urlsListView.count) {
--                    urlsListView.ViewItems.selectedIndices = []
--                } else {
--                    var indices = []
--                    for (var i = 0; i < urlsListView.count; ++i) {
--                        indices.push(i)
--                    }
--                    urlsListView.ViewItems.selectedIndices = indices
--                }
--
--                urlsListView.forceActiveFocus()
--            }
++            onClicked: internal.toggleSelectAll()
+         }
          ToolbarAction {
@@ -342,24 +346,7 @@
              iconName: "delete"
              text: i18n.tr("Delete")
              enabled: urlsListView.ViewItems.selectedIndices.length > 0
--            onClicked: {
--                var indices = urlsListView.ViewItems.selectedIndices
--                var urls = []
--                for (var i in indices) {
--                    urls.push(urlsListView.model.get(indices[i])["url"])
--                }
--
--                if (urlsListView.count == urls.length) {
--                    lastVisitDateListView.currentIndex = 0
--                }
--
--                urlsListView.ViewItems.selectMode = false
--                for (var j in urls) {
--                    historyViewWide.historyEntryRemoved(urls[j])
--                }
--
--                lastVisitDateListView.forceActiveFocus()
--            }
++            onClicked: internal.removeSelected()
+         }
          ListItems.ThinDivider {
@@ -415,4 +402,41 @@
+             }
+         }
+     }
++
++    QtObject {
++        id: internal
++
++        function toggleSelectAll() {
++            if (urlsListView.ViewItems.selectedIndices.length === urlsListView.count) {
++                urlsListView.ViewItems.selectedIndices = []
++            } else {
++                var indices = []
++                for (var i = 0; i < urlsListView.count; ++i) {
++                    indices.push(i)
++                }
++                urlsListView.ViewItems.selectedIndices = indices
++            }
++
++            urlsListView.forceActiveFocus()
++        }
++
++        function removeSelected() {
++            var indices = urlsListView.ViewItems.selectedIndices
++            var urls = []
++            for (var i in indices) {
++                urls.push(urlsListView.model.get(indices[i])["url"])
++            }
++
++            if (urlsListView.count == urls.length) {
++                lastVisitDateListView.currentIndex = 0
++            }
++
++            urlsListView.ViewItems.selectMode = false
++            for (var j in urls) {
++                historyViewWide.historyModel.removeEntryByUrl(urls[j])
++            }
++
++            lastVisitDateListView.forceActiveFocus()
++        }
++    }
+ }
 === 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 @@
+ }
  /*!
++    Remove all urls last visited in a given DATE from the history model.
++*/
++void HistoryModel::removeEntriesByDate(const QDate& date)
++{
++    if (!date.isValid()) {
++        return;
++    }
++
++    for (int i = m_entries.count() - 1; i >= 0; --i) {
++        if (m_entries.at(i).lastVisit.toLocalTime().date() == date) {
++            removeByIndex(i);
++        }
++    }
++    removeEntriesFromDatabaseByDate(date);
++    Q_EMIT rowCountChanged();
++}
++
++/*!
      Remove all urls from a given DOMAIN from the history model.
  */
  void HistoryModel::removeEntriesByDomain(const QString& domain)
@@ -396,6 +414,19 @@
      query.exec();
+ }
++void HistoryModel::removeEntriesFromDatabaseByDate(const QDate& date)
++{
++    QMutexLocker ml(&m_dbMutex);
++    QSqlQuery query(m_database);
++    static QString deleteStatement = QLatin1String("DELETE FROM history WHERE lastVisit BETWEEN ? AND ?;");
++    query.prepare(deleteStatement);
++    QDateTime dateTime = QDateTime(date);
++    query.addBindValue(dateTime.toTime_t());
++    dateTime.setTime(QTime(23, 59, 59, 999));
++    query.addBindValue(dateTime.toTime_t());
++    query.exec();
++}
++
  void HistoryModel::removeEntriesFromDatabaseByDomain(const QString& domain)
+ {
      QMutexLocker ml(&m_dbMutex);
 === 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 @@
      Q_INVOKABLE int add(const QUrl& url, const QString& title, const QUrl& icon);
      Q_INVOKABLE void removeEntryByUrl(const QUrl& url);
++    Q_INVOKABLE void removeEntriesByDate(const QDate& date);
      Q_INVOKABLE void removeEntriesByDomain(const QString& domain);
      Q_INVOKABLE void clearAll();
      Q_INVOKABLE void hide(const QUrl& url);
@@ -99,6 +100,7 @@
      void updateExistingEntryInDatabase(const HistoryEntry& entry);
      void removeEntryFromDatabaseByUrl(const QUrl& url);
      void removeEntryFromHiddenDatabaseByUrl(const QUrl& url);
++    void removeEntriesFromDatabaseByDate(const QDate& date);
      void removeEntriesFromDatabaseByDomain(const QString& domain);
      void clearDatabase();
  };
 === 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 @@
          QCOMPARE(model->rowCount(), 0);
+     }
++    void shouldRemoveByDate()
++    {
++        QCOMPARE(model->add(QUrl("http://example.org/"), "Example Domain", QUrl()), 1);
++        QCOMPARE(model->rowCount(), 1);
++        QCOMPARE(model->add(QUrl("http://example.com/"), "Example Domain", QUrl()), 1);
++        QCOMPARE(model->rowCount(), 2);
++
++        model->removeEntriesByDate(QDate::currentDate());
++        QCOMPARE(model->rowCount(), 0);
++    }
++
      void shouldRemoveByDomain()
+     {
          QCOMPARE(model->add(QUrl("http://example.org/page1"), "Example Domain Page 1", QUrl()), 1);
 === 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 @@
          signalName: "historyEntryClicked"
+     }
--    SignalSpy {
--        id: historyEntryRemovedSpy
--        target: historyViewWide
--        signalName: "historyEntryRemoved"
--    }
--
      UbuntuTestCase {
          name: "HistoryViewWide"
          when: windowShown
@@ -83,7 +77,7 @@
              mouseRelease(item, center.x + 100, center.y, Qt.LeftButton, Qt.NoModifier, 2000)
+         }
--        function initTestCase() {
++        function init() {
              for (var i = 0; i < 3; ++i) {
                  historyMockModel.add("http://example.org/" + i, "Example Domain " + i, "")
+             }
@@ -92,11 +86,14 @@
              waitForRendering(urlsList)
+         }
++        function cleanup() {
++            historyMockModel.clearAll()
++        }
++
          function test_done_button() {
              var doneButton = findChild(historyViewWide, "doneButton")
              verify(doneButton != null)
              doneSpy.clear()
--            compare(doneSpy.count, 0)
              clickItem(doneButton)
              compare(doneSpy.count, 1)
+         }
@@ -106,8 +103,6 @@
              verify(newTabButton != null)
              doneSpy.clear()
              newTabRequestedSpy.clear()
--            compare(doneSpy.count, 0)
--            compare(newTabRequestedSpy.count, 0)
              clickItem(newTabButton)
              compare(newTabRequestedSpy.count, 1)
              compare(doneSpy.count, 1)
@@ -116,7 +111,7 @@
          function test_history_entry_clicked() {
              var urlsList = findChild(historyViewWide, "urlsListView")
              compare(urlsList.count, 3)
--            compare(historyEntryClickedSpy.count, 0)
++            historyEntryClickedSpy.clear()
              clickItem(urlsList.children[0])
              compare(historyEntryClickedSpy.count, 1)
              var args = historyEntryClickedSpy.signalArguments[0]
@@ -160,14 +155,14 @@
          function test_delete_button() {
              var urlsList = findChild(historyViewWide, "urlsListView")
              compare(urlsList.count, 3)
++            var deletedUrl = urlsList.model.get(0).url
              longPressItem(urlsList.children[0])
              var deleteButton = findChild(historyViewWide, "deleteButton")
--            compare(historyEntryRemovedSpy.count, 0)
              clickItem(deleteButton)
--            compare(historyEntryRemovedSpy.count, 1)
--            var args = historyEntryRemovedSpy.signalArguments[0]
--            var entry = urlsList.model.get(0)
--            compare(args[0], entry.url)
++            compare(urlsList.count, 2)
++            for (var i = 0; i < urlsList.count; ++i) {
++                verify(urlsList.model.get(i).url != deletedUrl)
++            }
+         }
          function test_keyboard_navigation_between_lists() {
@@ -180,5 +175,38 @@
              keyClick(Qt.Key_Right)
              verify(urlsList.activeFocus)
+         }
++
++        function test_delete_key_at_urls_list_view() {
++            var urlsList = findChild(historyViewWide, "urlsListView")
++            keyClick(Qt.Key_Right)
++            verify(urlsList.activeFocus)
++            compare(urlsList.count, 3)
++            keyClick(Qt.Key_Delete)
++            compare(urlsList.count, 2)
++        }
++
++        function test_delete_key_at_last_visit_date() {
++            var lastVisitDateList = findChild(historyViewWide, "lastVisitDateListView")
++            var urlsList = findChild(historyViewWide, "urlsListView")
++            keyClick(Qt.Key_Left)
++            verify(lastVisitDateList.activeFocus)
++            compare(lastVisitDateList.currentIndex, 0)
++            keyClick(Qt.Key_Down)
++            compare(lastVisitDateList.currentIndex, 1)
++            compare(urlsList.count, 3)
++            keyClick(Qt.Key_Delete)
++            compare(urlsList.count, 0)
++        }
++
++        function test_delete_key_at_all_history() {
++            var lastVisitDateList = findChild(historyViewWide, "lastVisitDateListView")
++            var urlsList = findChild(historyViewWide, "urlsListView")
++            keyClick(Qt.Key_Left)
++            verify(lastVisitDateList.activeFocus)
++            compare(lastVisitDateList.currentIndex, 0)
++            compare(urlsList.count, 3)
++            keyClick(Qt.Key_Delete)
++            compare(urlsList.count, 0)
++        }
+     }
+ }