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

Proposed by Andrea Bernabei on 2015-05-27
Status: Merged
Approved by: Tim Peeters on 2015-05-27
Approved revision: 1523
Merged at revision: 1520
Proposed branch: lp:~faenil/ubuntu-ui-toolkit/oldcaption_alignment_fix
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 59 lines (+18/-6)
2 files modified
modules/Ubuntu/Components/ListItems/1.2/Caption.qml (+9/-3)
modules/Ubuntu/Components/ListItems/1.3/Caption.qml (+9/-3)
To merge this branch: bzr merge lp:~faenil/ubuntu-ui-toolkit/oldcaption_alignment_fix
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-05-27
Tim Peeters 2015-05-27 Approve on 2015-05-27
Review via email: mp+260278@code.launchpad.net

Commit message

Modify ListItem.Caption label margins

Description of the change

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

To post a comment you must log in.
Tim Peeters (tpeeters) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Tim Peeters (tpeeters) wrote :

> 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) wrote :

not really, I edited both?

Tim Peeters (tpeeters) wrote :

Indeed, I am not reading it well.

Andrea Bernabei (faenil) wrote :

no problem :D

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

Tim Peeters (tpeeters) wrote :

> 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) wrote :

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

Tim Peeters (tpeeters) wrote :

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) wrote :

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) wrote :

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

Tim Peeters (tpeeters) wrote :

Ok, then. Approving.

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'modules/Ubuntu/Components/ListItems/1.2/Caption.qml'
2--- modules/Ubuntu/Components/ListItems/1.2/Caption.qml 2015-04-30 08:32:44 +0000
3+++ modules/Ubuntu/Components/ListItems/1.2/Caption.qml 2015-05-27 14:15:27 +0000
4@@ -38,7 +38,7 @@
5 \endqml
6 */
7 Item {
8- height: captionText.height + units.gu(1)
9+ height: captionText.contentHeight + units.gu(2) * 2
10 width: parent ? parent.width : units.gu(31)
11
12 /*!
13@@ -49,8 +49,14 @@
14
15 Label {
16 id: captionText
17- anchors.centerIn: parent
18- width: parent.width - units.gu(1)
19+ anchors {
20+ verticalCenter: parent.verticalCenter
21+ left: parent.left
22+ right: parent.right
23+ //Margins are currently hardcoded to avoid adding new API
24+ leftMargin: units.gu(2)
25+ rightMargin: units.gu(2)
26+ }
27 wrapMode: Text.Wrap
28 color: Theme.palette.normal.backgroundText
29 horizontalAlignment: Text.AlignLeft
30
31=== modified file 'modules/Ubuntu/Components/ListItems/1.3/Caption.qml'
32--- modules/Ubuntu/Components/ListItems/1.3/Caption.qml 2015-04-29 07:21:29 +0000
33+++ modules/Ubuntu/Components/ListItems/1.3/Caption.qml 2015-05-27 14:15:27 +0000
34@@ -38,7 +38,7 @@
35 \endqml
36 */
37 Item {
38- height: captionText.height + units.gu(1)
39+ height: captionText.contentHeight + units.gu(2) * 2
40 width: parent ? parent.width : units.gu(31)
41
42 /*!
43@@ -49,8 +49,14 @@
44
45 Label {
46 id: captionText
47- anchors.centerIn: parent
48- width: parent.width - units.gu(1)
49+ anchors {
50+ verticalCenter: parent.verticalCenter
51+ left: parent.left
52+ right: parent.right
53+ //Margins are currently hardcoded to avoid adding new API
54+ leftMargin: units.gu(2)
55+ rightMargin: units.gu(2)
56+ }
57 wrapMode: Text.Wrap
58 color: theme.palette.normal.backgroundText
59 horizontalAlignment: Text.AlignLeft

Subscribers

People subscribed via source and target branches