[Merge] lp:~artmello/webbrowser-app/webbrowser-app-history_view_convergence into lp:webbrowser-app

Olivier Tilloy olivier.tilloy at canonical.com
Fri Jul 31 08:53:52 UTC 2015


Review: Needs Fixing

If I scroll down the left panel so that the first entry (All History) is out of view, and then delete all the entries for one given day, All History becomes the current item, but it is not scrolled into view in the left panel.

There is no need to override Keys.onUpPressed and Keys.onDownPressed for ListViews, as the default implementation has keyboard navigation built in. All that’s needed is for the list view to have focus.

tst_HistoryViewWide.qml is a good start, but this is not enough. Think of it as a self contained replacement for autopilot tests for the component. All important use cases need to be tested.
historyEntryClicked and historyEntryRemoved should be tested, as well as general keyboard navigation and mouse/touch interaction with the component.

In HistoryViewWide.qml, why is there a timer that sets focus on urlsListView after 1 millisecond? Can’t this be done in Component.onCompleted?

history-lastvisitdate-model.cpp has a test coverage of only 69.7% (because HistoryLastVisitDateModel::get(…) is not tested), this needs to be improved.

I’m wondering whether HistoryLastVisitDateListModel is really necessary. What we need is a list view that displays all the unique dates in a history model. Can’t this be achieved with the HistoryTimeframeModel, with empty delegates and section headers for each unique visit date? I know this sounds like a hack, but if functional and explained with comments it should be clean enough, and would avoid a lot of custom C++ code. In any case I don’t think HistoryLastVisitDateListModel needs an "entries" role, all that’s needed is to instantiate a HistoryLastVisitDateModel in QML and set its 'lastVisitDate' property dynamically to the date of the currently selected entry in the left panel.

-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-history_view_convergence/+merge/263052
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list