Code review comment for lp:~gerboland/unity8/orientationLock

Revision history for this message
Gerry Boland (gerboland) wrote :

> In tests/qmltests/Stages/tst_SessionContainer.qml
>
> """
> tryCompare(sessionContainer, "orientation",
> sessionContainer.orientation);
> """
>
> Huh!? You're comparing sessionContainer.orientation to itself?
> You probably meant:
> tryCompare(childContainer, "orientation", sessionContainer.orientation);

Yep silly me. Thanks for catching that.

> Calling it rootSessionContainer instead of just sessionContainer would also
> help to clarify things. But that's just a suggestion.

Done. And used childSessionContainer while I was at it.

> -----------------------------------------------------
>
> In tests/qmltests/Stages/tst_SessionContainer.qml
>
> """
> /* Text orientation changes are propagated to all children immediately */
> """
>
> typo. s/Text/Test

Done

> ------------------------------------------------------
>
> """
> --- qml/Stages/ApplicationWindow.qml 2014-09-03 16:38:10 +0000
> +++ qml/Stages/ApplicationWindow.qml 2014-09-11 10:41:11 +0000
> @@ -27,6 +27,7 @@ Item {
>
> // to be set from outside
> property QtObject application
> + property int orientation
>
> QtObject {
> id: d
> @@ -111,6 +112,7 @@ Item {
> id: sessionContainer
> session: application ? application.session : null
> anchors.fill: parent
> + orientation: root.orientation
>
> onSurfaceChanged: {
> if (sessionContainer.surface) {
> """
>
> Just a suggestion (feel free to change it or not):
> I think it's better to use property aliases in such cases as they seem to be
> more efficient (besides being more concise) than property bindings. But who
> knows, maybe they boil down to the same in the qml engine...

I've no idea how aliases work. I suppose they must be more efficient, but I also heard from a Qt developer (working on the QtQuickComponents) that they're expensive. So I don't know! I tend not to use them as (in this instance) it's harder to see immediately that SessionContainer has an orientation property, and you need to scroll around to figure out what sets that property. Personal taste I guess :) Mind if I leave it alone? If we learn it is indeed an optimization, I'll switch it.

« Back to merge proposal