Merge lp:~faenil/ubuntu-ui-toolkit/oldcaption_alignment_fix into lp:ubuntu-ui-toolkit/staging

[Andrea Bernabei]

Modify ListItem.Caption label margins

This commit adds margins to ListItem.Caption.

Since we can't access ListItem.Empty's __contentsMargins from here, I created a local variable with the hardcoded margin value. This is not optimal, suggestions welcome.

I verified this fixes the misalignment in the captions of ubuntu-system-settings on the BQ phone

[Tim Peeters (tpeeters)]

I'm not sure this is the place where the bug should be fixed. I commented about that in the bug report.

Please check out the QML coding conventions on http://doc.qt.io/qt-4.8/qml-coding-conventions.html

26 + anchors.verticalCenter: parent.verticalCenter
27 + anchors.left: parent.left
28 + anchors.right: parent.right
29 + anchors.leftMargin: __contentMargin
30 + anchors.rightMargin: __contentMargin

These should be grouped in
anchors {
...
}

review: Needs Fixing

[Tim Peeters (tpeeters)]

17 + /*! \internal
18 + The spacing inside the list item. This is already defined in ListItem.Empty but we can't access that from here.
19 + */
20 + property real __contentMargin: units.gu(2)

You can access all properties from the component that you inherit from, even \internal __prefixed properties. In this case you cannot access Empty's property, because Caption derives from Item, not from Empty. So here we are introducing new API in 1.2, which is not good.

Changes in 1.2 should also be done in 1.3. But in this case I'm not sure if any changes should be made here, or in system settings.

review: Needs Information

[Andrea Bernabei (faenil)]

of course, it derives from Item, that's why I added an explicit description, but it seems I should have been more explicit ;)

[Andrea Bernabei (faenil)]

also, the changes *are* done in 1.3 as well, aren't they?

[Tim Peeters (tpeeters)]

> also, the changes *are* done in 1.3 as well, aren't they?

check modules/Ubuntu/Components/ListItems/qmldir. It has different files listed for Caption versions 1.3 and <=1.2. For 1.3 it is in ListItems/1.3/Caption.qml and you only edited ListItems/1.2/Caption.qml

[Andrea Bernabei (faenil)]

not really, I edited both?

[Tim Peeters (tpeeters)]

Indeed, I am not reading it well.

[Andrea Bernabei (faenil)]

no problem :D

if the __contentMargin inside the Caption itself bothers you, I can move that inside the label itself...

[Tim Peeters (tpeeters)]

> no problem :D
>
> if the __contentMargin inside the Caption itself bothers you, I can move that
> inside the label itself...

Yes, especially because you are adding API to a 1.2 component. We don't add API to 1.2 any more because that would mean that on a newer UITK version something might work, and then on another device which has (supposedly the same) 1.2, the app won't work because some properties are missing that the app might be using.

[Andrea Bernabei (faenil)]

margins hardcoded in order to avoid a new internal property (and avoid having a QtObject just for that as well)

[Tim Peeters (tpeeters)]

9 + height: captionText.contentHeight + units.gu(2) * 2

units.gu(4)

23 + //Margins are currently hardcoded to avoid adding new API
24 + leftMargin: units.gu(2)
25 + rightMargin: units.gu(2)

margins: units.gu(2) should work fine because the Caption height is label height + units.gu(4)

[Andrea Bernabei (faenil)]

I kept units.gu(2)*2 on purpose, so that if someone modifies the component it's easy to spot that they refer to the same value

[Andrea Bernabei (faenil)]

I'd rather keep left and rightMargin to avoid side effects if someone else defines top/bottom anchors as well

[Tim Peeters (tpeeters)]

Ok, then. Approving.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-autolanding/949/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-autolanding/257
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-autolanding/256
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-autolanding/256/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-autolanding/256

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'modules/Ubuntu/Components/ListItems/1.2/Caption.qml'
--- modules/Ubuntu/Components/ListItems/1.2/Caption.qml	2015-04-30 08:32:44 +0000
+++ modules/Ubuntu/Components/ListItems/1.2/Caption.qml	2015-05-27 14:15:27 +0000
@@ -38,7 +38,7 @@
     \endqml
 */
 Item {
-    height: captionText.height + units.gu(1)
+    height: captionText.contentHeight + units.gu(2) * 2
     width: parent ? parent.width : units.gu(31)
 
     /*!
@@ -49,8 +49,14 @@
 
     Label {
         id: captionText
-        anchors.centerIn: parent
-        width: parent.width - units.gu(1)
+        anchors {
+            verticalCenter: parent.verticalCenter
+            left: parent.left
+            right: parent.right
+            //Margins are currently hardcoded to avoid adding new API
+            leftMargin: units.gu(2)
+            rightMargin: units.gu(2)
+        }
         wrapMode: Text.Wrap
         color: Theme.palette.normal.backgroundText
         horizontalAlignment: Text.AlignLeft

=== modified file 'modules/Ubuntu/Components/ListItems/1.3/Caption.qml'
--- modules/Ubuntu/Components/ListItems/1.3/Caption.qml	2015-04-29 07:21:29 +0000
+++ modules/Ubuntu/Components/ListItems/1.3/Caption.qml	2015-05-27 14:15:27 +0000
@@ -38,7 +38,7 @@
     \endqml
 */
 Item {
-    height: captionText.height + units.gu(1)
+    height: captionText.contentHeight + units.gu(2) * 2
     width: parent ? parent.width : units.gu(31)
 
     /*!
@@ -49,8 +49,14 @@
 
     Label {
         id: captionText
-        anchors.centerIn: parent
-        width: parent.width - units.gu(1)
+        anchors {
+            verticalCenter: parent.verticalCenter
+            left: parent.left
+            right: parent.right
+            //Margins are currently hardcoded to avoid adding new API
+            leftMargin: units.gu(2)
+            rightMargin: units.gu(2)
+        }
         wrapMode: Text.Wrap
         color: theme.palette.normal.backgroundText
         horizontalAlignment: Text.AlignLeft