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

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

Visually, this looks good except for the separation between the header and the list items. I think you should add a Divider there (https://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.ListItems.Divider/).

A few comments on the code itself:

 - In SettingsPage.qml there are a bunch of references to 'browser', which breaks encapsulation: the SettingsPage component isn’t aware of its parent context. To avoid this, you should add top-level properties to the component for each setting, and in Browser.qml where the component is instantiated, listen to changes on those properties and update the corresponding properties on the Browser instance.

 - Instead of a historyRemoved() signal, I would add a top-level 'historyModel' property, and have the corresponding action call clearAll() on the model. Also, the opacity shouldn’t be set to 0.5 after clearing. Instead, we need to add a 'count' property to the history model, and bind the opacity to its value being ≠ 0.

 - The back button should probably be implemented using an AbstractButton (and it should be highlighted when pressed, to match the standard header).

 - "Restore old session on startup" should probably be "Restore previous session at startup"

 - "Open new tab in background" should probably be "Allow opening new tabs in background"

 - The Column needs to be inside a Flickable, in case all entries don’t fit on the screen

review: Needs Fixing

« Back to merge proposal