[Merge] lp:~osomon/webbrowser-app/use-qml-SortFilterModel into lp:webbrowser-app

[Ugo Riboni]

Review: Needs Fixing

Apart of that code-wise all LGTM. Reviews that mostly remove code are always a pleasure. I left a few comments inline, but they are not major things. 
Performing functional testing in a minute.

Diff comments:

> 
> === modified file 'src/app/webbrowser/HistoryViewWide.qml'
> --- src/app/webbrowser/HistoryViewWide.qml	2015-09-18 20:43:26 +0000
> +++ src/app/webbrowser/HistoryViewWide.qml	2015-10-02 09:54:57 +0000
> @@ -234,13 +234,18 @@
>                  Keys.onReturnPressed: historyEntrySelected()
>                  Keys.onEnterPressed: historyEntrySelected()
>  
> -                model: HistoryLastVisitDateModel {
> +                model: SortFilterModel {
>                      id: historyLastVisitDateModel
> +                    property date lastVisitDate
> +                    filter {
> +                        property: "lastVisitDate"
> +                        pattern: new RegExp(Qt.formatDate(lastVisitDate, "yyyy-MM-dd"))

I feel a bit nervous with implicitly relying on QDate and QDateTime conversion to QString (when the data is exposed through the model data() method) and then doing our conversion to string on the QML side to match it.
 
Wouldn't it be safer and cleaner to add a role that exposes the lastVisit as a QString in ISO 8601 standard format and then use that for sorting and filtering ? You could do things like anchor the RegExp to the start for extra safety.

> +                    }
>                      // Until a valid HistoryModel is assigned the TextSearchFilterModel
> -                    // will not report role names, and the HistoryLastVisit*Models will emit warnings
> -                    // since they need a dateLastVisit role to be present.
> -                    // We avoid this by assigning the sourceModel only when HistoryModel is ready.
> -                    sourceModel: historyModel ? historySearchModel : undefined
> +                    // will not report role names, and the HistoryLastVisitDateListModel
> +                    // will emit warnings since it needs a dateLastVisit role to be
> +                    // present.
> +                    model: historyModel ? historySearchModel : null
>                  }
>  
>                  clip: true
> 
> === modified file 'src/app/webbrowser/NewTabView.qml'
> --- src/app/webbrowser/NewTabView.qml	2015-08-10 15:22:00 +0000
> +++ src/app/webbrowser/NewTabView.qml	2015-10-02 09:54:57 +0000
> @@ -33,11 +32,12 @@
>      signal bookmarkRemoved(url url)
>      signal historyEntryClicked(url url)
>  
> +    HistoryTimeframeModel {
> +        id: historyTimeframeModel
> +    }

Is there any particular reason why you prefer to declare this separately instead as directly assigned to the topSitesModel.model ? Same applies in other parts of the code. I don't mind but I would like to know if it is just a style preference you have or if there are other reasons for it.

>      TopSitesModel {
>          id: topSitesModel
> -        sourceModel: HistoryTimeframeModel {
> -            id: historyTimeframeModel
> -        }
> +        model: historyTimeframeModel
>      }
>  
>      QtObject {
> 
> === modified file 'src/app/webbrowser/history-lastvisitdatelist-model.cpp'
> --- src/app/webbrowser/history-lastvisitdatelist-model.cpp	2015-08-20 12:12:55 +0000
> +++ src/app/webbrowser/history-lastvisitdatelist-model.cpp	2015-10-02 09:54:57 +0000
> @@ -86,9 +86,9 @@
>  void HistoryLastVisitDateListModel::setSourceModel(QVariant sourceModel)
>  {
>      QAbstractItemModel* newSourceModel = qvariant_cast<QAbstractItemModel*>(sourceModel);
> -    if (sourceModel.isValid() && newSourceModel == 0) {
> -       qWarning() << "Only QAbstractItemModel-derived instances are allowed as"
> -                  << "source models";
> +    if (sourceModel.isValid() && (newSourceModel == 0) && !sourceModel.canConvert<void*>()) {
> +        qWarning() << "Only QAbstractItemModel-derived instances are allowed as"
> +                   << "source models";

Maybe in the warning mention that null is allowed too ?

>      }
>  
>      if (newSourceModel != m_sourceModel) {


-- 
https://code.launchpad.net/~osomon/webbrowser-app/use-qml-SortFilterModel/+merge/273203
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.