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

Olivier Tilloy olivier.tilloy at canonical.com
Wed Oct 7 11:13:28 UTC 2015


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

Agreed, this is safer. Done, I added a 'lastVisitDateString' role. It doesn’t include the time as there doesn’t seem to be a need for it anywhere, we can update the role to be 'lastVisitString' in the future if the need arises.


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

Yes, there is a good reason for this structure. See comments 1 and 2 on bug #1495482.


> Maybe in the warning mention that null is allowed too ?

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



More information about the Ubuntu-reviews mailing list