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

Arthur Mello arthur.mello at canonical.com
Tue Aug 4 04:27:26 UTC 2015


> 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.

Fixed

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

Fixed

> 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.

I added a couple of new tests but they are in a no good state so far. I am facing problem in populating the history model before running the tests. It seems that the tests are running before everything is correctly set. So unfortunately that is still WIP.

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

I am using that since something is setting the focus after the component is complete. It needs further investigation to see what is happening and how to unset that.

> 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.
>   
> 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.

Both those questions are related with how I implemented the left panel. The "All History" entry is placed in the header element of the ListView. Since that entry would not make much sense in the model itself, I decided to misuse the header to work around the feature. Because of that, we have some keyboard navigation issues since the header is not considered by the list during the navigation and while placing view correctly when scrolling the list. That was the cause of a couple of your comments and that is why I am overriding keyUp and keyDown to use the header as another delegate. And, since I use the header of the list view, I was not able to find out a way to implement the keyboard navigation using the HistoryTimeframeModel as model and the section to implement the left panel. I agree that would be a better solution since would reduce the code base in a good way, but we would need to figure out how to use implement the keyboard navigation on it.
-- 
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