webbrowser-app

Merge lp://qastaging/~artmello/webbrowser-app/webbrowser-app-fix_1484562 into lp://qastaging/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://qastaging/~artmello/webbrowser-app/webbrowser-app-fix_1484562

Merge into:

lp://qastaging/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://qastaging/~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.qastaging.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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~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://qastaging/~artmello/webbrowser-app/webbrowser-app-fix_1484562 updated on 2015-08-18