[Merge] lp:~ahayzen/webbrowser-app/dnd-tabs-001 into lp:webbrowser-app/staging

Olivier Tilloy olivier.tilloy at canonical.com
Thu Nov 3 10:30:39 UTC 2016


Review: Needs Fixing

Please fix encapsulation that’s broken in various places:

- in src/app/webbrowser/TabsBar.qml there are references to 'window', 'browser' and 'tabsModel'
- in src/app/webbrowser/Browser.qml there is a reference to 'builder(…)'

The new builder() function sets the properties on the object after instantiating it, as opposed to setting them at construction time. Can’t you pass a dict of properties to Reparenter::createObject(…) ?

Reparenter should probably be made a singleton type, and referred to as 'Reparenter' throughout QML code. I don’t think it needs to be a QQuickItem, a simple QObject would do.

In src/app/webbrowser/Browser.qml, buildContextProperties() and createTabHelper() don’t need to be public functions, please make them private by moving them inside the 'internal' QtObject.

In src/app/webbrowser/Browser.qml, dropArea.heightThreshold should be declared as real, not int.

Do we really need the console.debug() statements?

In src/app/webbrowser/drag-helper.[h|cpp] and src/app/webbrowser/reparenter.[h|cpp], please use fully-qualified include directories for consistency, e.g. <QtCore/QObject> instead of <QObject>. It makes it easier to know which Qt modules are required dependencies at a glance.

Can the 'closeMethod' parameter of 'newWindowFromTab()' be renamed 'callback' ?
-- 
https://code.launchpad.net/~ahayzen/webbrowser-app/dnd-tabs-001/+merge/308507
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app/staging.



More information about the Ubuntu-reviews mailing list