Code review comment for lp:~kalikiana/ubuntu-ui-toolkit/appsettings

> Can we have more setting groups defined in an application? how do we select between those?

Yes, by using the "group" property which is a string.

> Will the Settings.gropup property change cause settings reload?

No and I don't see a use case for changing groups at runtime, so I added a warning that's shown if you try.

> 735 +Item {
> 736 + id: settings
> Do we need an Item for this? In that way we can put visual items onto it, but that does not make any sense... Shouldn't we use Object instead?

Yes and no. It must be an Item so that you can put it into another Item. It doesn't say anything about what you can put inside.

> 753 + Component.onCompleted: {
> What if children get changed after this point? Do we exclude them from state saving?

I changed it, this didn't work before I ported to a custom default property.

> default property list<Option> options
> So we make sure we cannot add anything else into the Settings blob

I failed to make this work before, I gave it another try. I managed to get it to work by using an alias on a second property which is the real list. If I declare it as one I get syntax errors. And indeed with this I could drop the hasOwnProperty checks. Though the error message generated by QML is not really great, it gets us an early failure.

> Please close each JS line with semicolon.

Done.