Merge lp:~unity-team/unity8/unity8-fix-mascot into lp:unity8

Proposed by Michał Karnicki
Status: Merged
Approved by: Michał Sawicz
Approved revision: 677
Merged at revision: 678
Proposed branch: lp:~unity-team/unity8/unity8-fix-mascot
Merge into: lp:unity8
Diff against target: 25 lines (+9/-3)
1 file modified
qml/Dash/CardHeader.qml (+9/-3)
To merge this branch: bzr merge lp:~unity-team/unity8/unity8-fix-mascot
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Michał Sawicz Approve
Review via email: mp+203967@code.launchpad.net

Commit message

CardHeader mascot improvements.

Description of the change

Change CardHeader mascot size and image properties.

http://ubuntuone.com/5SsrLHHadzoZrYb7o8Uz7g

Default Image fillMode is Strech.
Img1: Mascot looks really bad.
Img2: Adds PreserveAspectCrop
Img3: Change size and aspect ratio along the 'future dash' spec.
Blur effect will be fixed when Saviq backports this:
https://bugs.launchpad.net/unity-lens-applications/+bug/1271158

Are there any related MPs required for this MP to build/function as expected? Please list.
 * No.
Did you perform an exploratory manual test run of your code change and any related functionality?
 * Yes, as well as run Card and CardHeader tests.
If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
 * N/A
If you changed the UI, has there been a design review?
 * Changed UI, based on feedback from Katie. Reviewed by Katie.

To post a comment you must log in.
674. By Michał Karnicki

Merged trunk.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
675. By Michał Karnicki

Fix whitespace.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:675
http://jenkins.qa.ubuntu.com/job/unity8-ci/2179/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2765
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/2526/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1052
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/701
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/703
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/703/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/701
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2767
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2767/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2527
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2527/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/4956/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3492

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2179/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

21 + sourceSize {
22 + width: mascotShape.maxSize
23 + height: mascotShape.maxSize
24 + }

Flatten, please - does not really help readability, IMO.

=====

14 + readonly property int maxSize: Math.max(width, height)

I wonder if it's more expensive to keep this property than to just use Math.max() directly in sourceSize... But let's leave it be.

=====

26 + horizontalAlignment: Image.AlignHCenter

verticalAlignment: Image.AlignVCenter, too

=====

Truth be told I wouldn't add those tests... They're really just testing visual properties - whether you bound the right value in the right place... And what's more we might allow theming that, at which point the test would become invalid.

Only the sourceSize tests make sense, but I want to replace the Image + Shape with LazyImage, which has that built in, so those would go away.

review: Needs Fixing
676. By Michał Karnicki

Remove tests, per discussion on IRC with Saviq.

677. By Michał Karnicki

Typo.

Revision history for this message
Michał Sawicz (saviq) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Yes.

 * Did CI run pass? If not, please explain why.
No. Still last two tests failing in otto.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qml/Dash/CardHeader.qml'
2--- qml/Dash/CardHeader.qml 2014-01-27 21:23:05 +0000
3+++ qml/Dash/CardHeader.qml 2014-01-30 22:10:52 +0000
4@@ -40,13 +40,19 @@
5 id: mascotShape
6 objectName: "mascotShape"
7
8- width: units.gu(8)
9- height: units.gu(8)
10+ // TODO karni: Icon aspect-ratio is 8:7.5. Revisit these values to avoid fraction of pixels.
11+ width: units.gu(6)
12+ height: units.gu(5.625)
13 visible: image.status === Image.Ready
14+ readonly property int maxSize: Math.max(width, height)
15
16 image: Image {
17 id: mascotImage
18- sourceSize { width: mascotShape.width; height: mascotShape.height }
19+
20+ sourceSize { width: mascotShape.maxSize; height: mascotShape.maxSize }
21+ fillMode: Image.PreserveAspectCrop
22+ horizontalAlignment: Image.AlignHCenter
23+ verticalAlignment: Image.AlignVCenter
24 }
25 }
26

Subscribers

People subscribed via source and target branches