[Merge] lp:~uriboni/webbrowser-app/search-history into lp:webbrowser-app
Ugo Riboni
ugo.riboni at canonical.com
Wed Sep 16 17:26:08 UTC 2015
Fixed unless otherwise commented below
> 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.
Exiting select mode and entering search mode would be confusing and not symmetric, since when search mode you can't long press on items to switch to select mode.
So I am swallowing the key combo event instead.
> 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.
Done and also exited the find in page mode when settings is shown (since when transitioning back from settings I found it really jarring to see the find in page box there)
> 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.
Since the change in UITK made it to the overlay PPA already, I switched to using SortFilterModel.
I have not removed the HistoryLastVisitDateModel sources and tests, as I think it is better done in a separate MR (like one based the branch you link above)
> Is the first change in history-timeframe-model.cpp really necessary? Doesn’t
> QSortFilterProxyModel::setSourceModel() imply a model reset notification?
I thought so too, but apparently not. And the documentation does not say so either.
> In tst_HistoryViewWide.qml:
> - 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)
Setting focus:true only on the Loader is not enough. On the loader and on the item inside works.
--
https://code.launchpad.net/~uriboni/webbrowser-app/search-history/+merge/270929
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list