Code review comment for lp:~rpadovani/webbrowser-app/settings-page

Revision history for this message
Olivier Tilloy (osomon) wrote :

In src/app/webbrowser/SettingsPage.qml:

 - Instead of having searchEngineContainer and privacyContainer, we could have one single subpageContainer, as we will never have two subpages instantiated at the same time anyway.

 - Most ListItems have an action defined with onTriggered, but the one for "Reset browser settings" only has onClicked. The result is the same, but can the code be made consistent (I don’t have any preference either way, other than the use of onClicked results in less code :))

 - Instead of directly setting the opacity on the list item for "Clear Browsing History", you should set the 'enabled' property to (historyModel.count > 0)

 - The code would be more readable if the header came before the flickable (just set 'clip' to true on the flickable, I’m told that rectangular clipping is cheap these days, and as a bonus you won’t need a coloured rectangle inside the divider in the header).

 - Just a personal preference: I would instantiate the Bindings next to the switches their refer to, for improved readability.

review: Needs Fixing

« Back to merge proposal