[Merge] lp:~uriboni/webbrowser-app/qml-tabs-model into lp:webbrowser-app

[Olivier Tilloy]

Review: Needs Fixing

Overall this looks good to me, getting rid of custom C++ code in favour of QML with (supposedly) no performance hit is always good, thanks for working on this.

I have a few minor comments:

56  -            return tabComponent.createObject(tabContainer, properties)
57	+            var tab = tabComponent.createObject(tabContainer, properties)
58	+            return tab

that change is unnecessary, can it be reverted?


Is QTBUG-26357 the correct bug report for the lack of a way to override methods and call their base implementation? It doesn’t look like it from the description.


TabsModel could be a QtObject instead of an Item (you could move the updateCurrentTab() function to inside the ListModel).


There’s a couple of lines of JS code in TabsModel.qml that end with a semicolon (and one line in test_shouldReturnTabWhenRemoving), can you please remove them?


What is the point of allowing currentIndex to be invalid? Shouldn’t it be reset to a valid value?


Related to the above, what is the point of allowing the second parameter of insert() to be invalid, and have the model clamp it? Wouldn’t it be better to spit out an error if the index is not valid?


Because these tests don’t exercise user input, the width and height of the top-level item in tst_TabsModel.qml don’t need to be that large (they can be set to 1px).


Camel-casing on test_shouldUpdateCurrentTabWhenSettingcurrentIndex is not correct.


The first two compare() in test_shouldSetCurrentTabWhenAddingFirstTab and in test_shouldSetCurrentTabWhenInsertingFirstTab are useless, as this is already being tested by test_shouldBeInitiallyEmpty.
-- 
https://code.launchpad.net/~uriboni/webbrowser-app/qml-tabs-model/+merge/270408
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.