[Merge] lp:~uriboni/webbrowser-app/search-history into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Tue Sep 15 11:33:45 UTC 2015
Review: Needs Fixing
> When a cetain date is selected and the current search returns no
> results for that date, move back to the "all dates" block
This doesn’t seem to always work. In some cases, a more recent date than the one I initially selected, which has matching results, gets selected (just tested with 1st of sept, no matches for a given search string, but 2nd of sept gets selected instead of all history).
I think monitoring onCountChanged on the list in the right panel is not reliable, you should probably record which date was selected, and if it disappears from the left panel, then reset the current index.
In the visual spec (https://docs.google.com/presentation/d/1P6A7ZsI03sPfuI9vzPeC0VcUfIgugxnU4QyObH6UNTs/edit#slide=id.g75d5dd944_091), the search field has a magnifier icon always displayed on the left, and the field spans the width of the right panel.
In the history view, if I long-press on an item to enter multi-selection mode, then press Ctrl+F, nothing seemingly happens, but after closing the history view find-in-page is activated on the current tab. Ctrl+F events should be swallowed when in multi-selection mode. Or maybe they should exit multi-selection mode and activate search in history. In any case they shouldn’t have an effect on a view that’s not currently visible.
Related to the above comment (and not specific to this MR, but could be fixed together with it): if find in page was activated on the current page, I think it should be turned off when opening the history view.
In Highlight.js, only the highlightTerms function should be public, the rest (global vars and helper functions) shouldn’t be exposed, can’t they be defined in the body of highlightTerms?
In HistoryViewWide.qml, there is a typo in a comment: s/TestSearchFilterModel/TextSearchFilterModel/.
A note on the modifications you made to HistoryLastVisitDateModel: if it wasn’t for bug #1495641, this model could entirely be replaced by a simple SortFilterModel, which is what I did here: http://bazaar.launchpad.net/~osomon/webbrowser-app/use-qml-SortFilterModel/revision/1144. Until my fix for the bug in the UITK lands, your changes look good.
In history-lastvisitdatelist-model.h, includes are not ordered alphabetically.
Is the first change in history-timeframe-model.cpp really necessary? Doesn’t QSortFilterProxyModel::setSourceModel() imply a model reset notification?
In tst_HistoryLastVisitDateListModelTests.cpp, in shouldUpdateWhenChangingSourceModel(), timeframe2 is never deleted (the issue was there already, not introduced by your changes). Can it be instantiated on the stack, or deleted at the end of the method?
Can HistoryModelTest be renamed 'HistoryModelMock' ?
In HistoryModelTest::addByDate(), when inserting a new entry, the number of visits is initially set to 1 (with the call to add(…)), and then bumped to 2. Incrementing entry.visits should be done only when it’s not a new entry.
"efficiency is not important": I would re-word that to "efficiency is not critical". Efficiency is always important :)
In tests/unittests/qml/tst_QmlTests.cpp, the 'HistoryModelTest' type should be registered under "webbrowsertest.private", not under "webbrowserapp.private".
In tst_HistoryViewWide.qml:
- "id: historyViewWideComponent" is unused and can be removed
- "anchors.fill: parent" inside the HistoryViewWide instance is useless, as the Loader will resize its content item to fill itself already
- "id: historyMockModel" seems to be unused
- in init(), I don’t think a call to historyViewWide.forceActiveFocus() is needed, instead you can just set "focus: true" on the Loader (which will forward focus as a Loader is a FocusScope)
- in test_search_button, it might be useful to verify that the text field is initially invisible, before clicking the search button
--
https://code.launchpad.net/~uriboni/webbrowser-app/search-history/+merge/268610
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list