Unity 8

Merge lp:~cimi/unity8/carousel-music-video into lp:unity8

carousel-music-video
Merge into trunk

Proposed by Andrea Cimitan on 2013-10-22

Status:

Rejected

Rejected by:

Michał Sawicz on 2014-04-29

Proposed branch:

lp:~cimi/unity8/carousel-music-video

Merge into:

lp:unity8

Prerequisite:

lp:~unity-team/unity8/carousel-loader

Diff against target:

216 lines (+127/-27)

4 files modified

Dash/Music/CarouselDelegateMusic.qml (+59/-13)
Dash/Video/CarouselDelegateVideo.qml (+64/-10)
Dash/Video/VideoCarousel.qml (+2/-2)
Dash/Video/VideoTileStyle.qml (+2/-2)

To merge this branch:

bzr merge lp:~cimi/unity8/carousel-music-video

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michał Sawicz		2013-10-22	Disapprove on 2014-04-29
PS Jenkins bot (community)	continuous-integration	2013-10-22	Approve on 2013-11-18
Michael Zanetti (community)			Needs Fixing on 2013-10-24
Review via email: mp+192118@code.launchpad.net

This proposal supersedes a proposal from 2013-10-15.

Commit message

Added music and video carousel using new assets

Description of the change

Added music and video carousel using new assets (music carousel asset will need future update).

This branch will conflict with https://code.launchpad.net/~unity-team/unity8/carousel-loader/+merge/190406 so the last to get approved will require a small update

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-15: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1416/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-10-21: Posted in a previous version of this proposal

What is " // XXX test on Nexus 10"?

What's the status of the incorrect shape of the overlay?

34 + borderSource: "cimi wants no borders"
Erm... Filed a bug about this?

49 + duration: 250
50 + easing.type: Easing.InOutQuad

We've got UbuntuAnimation for standard durations. Also, opacity animations are supposed to be Linear.

67 + visible: true

Is default.

review: Needs Fixing

Revision history for this message

Andrea Cimitan (cimi) wrote on 2013-10-21: Posted in a previous version of this proposal

> What is " // XXX test on Nexus 10"?
>
> What's the status of the incorrect shape of the overlay?
>
> 34 + borderSource: "cimi wants no borders"
> Erm... Filed a bug about this?
>
bordersource needs a not-empty and incorrect value to disable borders...

> 49 + duration: 250
> 50 + easing.type: Easing.InOutQuad
>
> We've got UbuntuAnimation for standard durations. Also, opacity animations are
> supposed to be Linear.
>
> 67 + visible: true
>
> Is default.
will fix those tomorrow morning

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-22: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1467/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-22:

FAILED: Continuous integration, rev:466
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~cimi/unity8/carousel-music-video/+merge/192118/+edit-commit-message

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1468/rebuild

review: Needs Fixing (continuous-integration)

lp:~cimi/unity8/carousel-music-video updated on 2013-10-22

467. By Andrea Cimitan on 2013-10-22: Update asset
468. By Andrea Cimitan on 2013-10-22: Update comments
469. By Andrea Cimitan on 2013-10-22: Changed url :)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-22:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1473/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-10-23:

Please remove the XXX comments, and just test under different grid units.

It's broken in different-than-portrait aspect ratio. Just ./run -- -fullscreen on desktop to see.

Should be left-aligned in videos.

Font looks too big in videos - is there support for the subtitle?

Still isn't aligned well in music :/ - and font looks too big.

Two-line title + subtitle in music overflows the overlay.

review: Needs Fixing

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-10-23:

Overlay in music should be 7gu-high, in video 6gu-high (just over video - disregarding the mount).

review: Needs Fixing

lp:~cimi/unity8/carousel-music-video updated on 2013-10-23

470. By Andrea Cimitan on 2013-10-23: Extra fixes and refinements for carousel music and video (align video grid tiles too)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-23:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1478/rebuild

review: Needs Fixing (continuous-integration)

lp:~cimi/unity8/carousel-music-video updated on 2013-10-23

471. By Andrea Cimitan on 2013-10-23: Updated overlay asset

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-23:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1481/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-23:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1482/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-10-24:

seems lots of new logic to me. Can we test this?

review: Needs Fixing

Revision history for this message

Andrea Cimitan (cimi) wrote on 2013-10-24:

> seems lots of new logic to me. Can we test this?

manual tests (i.e. eye...) can't really test visuals

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-10-24:

Well, we can do tests which check if the Loader creates a carousel or a grid, depending on the amount of items, no?

Revision history for this message

Andrea Cimitan (cimi) wrote on 2013-10-24:

> Well, we can do tests which check if the Loader creates a carousel or a grid,
> depending on the amount of items, no?
Not relevant to this branch, though

lp:~cimi/unity8/carousel-music-video updated on 2013-10-25

472. By Andrea Cimitan on 2013-10-25: Delete tags

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-25:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1497/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andrea Cimitan (cimi) wrote on 2013-11-18:

> Please remove the XXX comments, and just test under different grid units.
>
> It's broken in different-than-portrait aspect ratio. Just ./run -- -fullscreen
> on desktop to see.
>
> Should be left-aligned in videos.
>
> Font looks too big in videos - is there support for the subtitle?
>
> Still isn't aligned well in music :/ - and font looks too big.
>
> Two-line title + subtitle in music overflows the overlay.

Everything is fixed apart the fullscreen mode... will have a look

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-18:

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

review: Approve (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-29:

This is obsolete with new scopes.

review: Disapprove

Unmerged revisions

472. By Andrea Cimitan on 2013-10-25: Delete tags
471. By Andrea Cimitan on 2013-10-23: Updated overlay asset
470. By Andrea Cimitan on 2013-10-23: Extra fixes and refinements for carousel music and video (align video grid tiles too)
469. By Andrea Cimitan on 2013-10-22: Changed url :)
468. By Andrea Cimitan on 2013-10-22: Update comments
467. By Andrea Cimitan on 2013-10-22: Update asset
466. By Andrea Cimitan on 2013-10-22: Merged lp:~unity-team/unity8/carousel-loader
465. By Andrea Cimitan on 2013-10-22: use ubuntu animation
464. By Andrea Cimitan on 2013-10-15: Added carousel music and video renderers

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Andrea Cimitan

Mohammad Rakibul Hasan

Unity Team

 === modified file 'Dash/Music/CarouselDelegateMusic.qml'
 --- Dash/Music/CarouselDelegateMusic.qml	2013-07-09 11:04:25 +0000
 +++ Dash/Music/CarouselDelegateMusic.qml	2013-10-25 13:50:38 +0000
@@ -21,22 +21,68 @@
  BaseCarouselDelegate {
      id: item
--    UbuntuShape {
++    Image {
          anchors.fill: parent
--        radius: "medium"
--        borderSource: ""
--        image: Image {
--            asynchronous: true
--            sourceSize { width: item.width; height: item.height }
--            source: model ? model.icon : ""
++        source: "graphics/music_lens_carousel_itemholder.png"
++
++        UbuntuShape {
++            anchors.fill: parent
++            radius: "medium"
++            borderSource: "invalid url" // need an invalid / notNull url
++            image: Image {
++                asynchronous: true
++                sourceSize { width: item.width; height: item.height }
++                source: model ? model.icon : ""
++            }
++        }
++
++        Loader {
++            id: loaderFrame
++            anchors.fill: parent
++            opacity: item.explicitlyScaled ? 1.0 : 0
++
++            Behavior on opacity {
++                UbuntuNumberAnimation {
++                    easing.type: Easing.Linear
++                }
++            }
++
++            sourceComponent: opacity > 0 ? componentFrame : undefined
+         }
+     }
--    BorderImage {
--        anchors.centerIn: parent
--        opacity: 0.6
--        source: "../../Components/graphics/non-selected.sci"
--        width: parent.width + units.gu(1.5)
--        height: parent.height + units.gu(1.5)
++    Component {
++        id: componentFrame
++        Image {
++            source: "graphics/music_lens_carousel_text_overlay.png"
++
++            Column {
++                anchors {
++                    left: parent.left
++                    right: parent.right
++                    bottom: parent.bottom
++                    margins: units.gu(1)
++                    bottomMargin: units.gu(2)
++                }
++
++                Label {
++                    text: model ? model.comment : ""
++                    fontSize: "x-small"
++                    font.weight: Font.Bold
++                    elide: Text.ElideRight
++                    wrapMode: Text.Wrap
++                    maximumLineCount: 2
++                    width: parent.width
++                }
++                Label {
++                    text: model ? model.title : ""
++                    fontSize: "xx-small"
++                    elide: Text.ElideRight
++                    wrapMode: Text.Wrap
++                    maximumLineCount: 1
++                    width: parent.width
++                }
++            }
++        }
+     }
+ }
 === added directory 'Dash/Music/graphics'
 === added file 'Dash/Music/graphics/music_lens_carousel_itemholder@27.png'
 Binary files Dash/Music/graphics/music_lens_carousel_itemholder@27.png	1970-01-01 00:00:00 +0000 and Dash/Music/graphics/music_lens_carousel_itemholder@27.png	2013-10-25 13:50:38 +0000 differ
 === added file 'Dash/Music/graphics/music_lens_carousel_text_overlay@27.png'
 Binary files Dash/Music/graphics/music_lens_carousel_text_overlay@27.png	1970-01-01 00:00:00 +0000 and Dash/Music/graphics/music_lens_carousel_text_overlay@27.png	2013-10-25 13:50:38 +0000 differ
 === modified file 'Dash/Video/CarouselDelegateVideo.qml'
 --- Dash/Video/CarouselDelegateVideo.qml	2013-07-09 09:06:29 +0000
 +++ Dash/Video/CarouselDelegateVideo.qml	2013-10-25 13:50:38 +0000
@@ -21,22 +21,76 @@
  BaseCarouselDelegate {
      id: item
--    UbuntuShape {
++    Image {
          anchors.fill: parent
--        radius: "medium"
--        borderSource: ""
--        image: Image {
++        source: "graphics/movie_icon_holder_carousel.png"
++
++        Image {
++            id: thumbnail
              asynchronous: true
++            anchors {
++                fill: parent;
++                leftMargin: units.dp(2)
++                rightMargin: units.dp(2)
++                topMargin: units.gu(2) + units.dp(2)
++                bottomMargin: units.gu(2.5) - units.dp(1)
++            }
              sourceSize { width: item.width; height: item.height }
              source: model ? model.icon : ""
++            fillMode: Image.PreserveAspectCrop
++        }
++
++        Loader {
++            id: loaderFrame
++            anchors {
++                left: thumbnail.left
++                right: thumbnail.right
++                bottom: thumbnail.bottom
++            }
++            height: units.gu(6)
++            opacity: item.explicitlyScaled ? 1.0 : 0
++
++            Behavior on opacity {
++                UbuntuNumberAnimation {
++                    easing.type: Easing.Linear
++                }
++            }
++
++            sourceComponent: opacity > 0 ? componentFrame : undefined
+         }
+     }
--    BorderImage {
--        anchors.centerIn: parent
--        opacity: 0.6
--        source: "../../Components/graphics/non-selected.sci"
--        width: parent.width + units.gu(1.5)
--        height: parent.height + units.gu(1.5)
++    Component {
++        id: componentFrame
++        Rectangle {
++            color: Qt.rgba(0, 0, 0, 0.8)
++
++            Column {
++                anchors {
++                    left: parent.left
++                    right: parent.right
++                    top: parent.top
++                    margins: units.gu(1)
++                }
++
++                Label {
++                    width: parent.width
++                    text: model ? model.title : ""
++                    fontSize: "x-small"
++                    font.weight: Font.Bold
++                    elide: Text.ElideRight
++                    wrapMode: Text.Wrap
++                    maximumLineCount: 2
++                }
++                Label {
++                    width: parent.width
++                    text: model ? model.comment : ""
++                    fontSize: "xx-small"
++                    elide: Text.ElideRight
++                    wrapMode: Text.Wrap
++                    maximumLineCount: 1
++                }
++            }
++        }
+     }
+ }
 === modified file 'Dash/Video/VideoCarousel.qml'
 --- Dash/Video/VideoCarousel.qml	2013-06-05 22:03:08 +0000
 +++ Dash/Video/VideoCarousel.qml	2013-10-25 13:50:38 +0000
@@ -19,10 +19,10 @@
  Carousel {
      id: videosCarousel
--    tileAspectRatio: 198 / 288
++    tileAspectRatio: 1
      minimumTileWidth: units.gu(13)
      itemComponent: carouselDelegateVideo
--    selectedItemScaleFactor: 1.14
++    selectedItemScaleFactor: 1.2
      cacheBuffer: 1404 // 18px * 13gu * 6
      height: implicitHeight + units.gu(6)
 === modified file 'Dash/Video/VideoTileStyle.qml'
 --- Dash/Video/VideoTileStyle.qml	2013-10-14 10:23:18 +0000
 +++ Dash/Video/VideoTileStyle.qml	2013-10-25 13:50:38 +0000
@@ -37,8 +37,8 @@
              objectName: "image"
              anchors {
                  fill: parent;
--                topMargin: units.gu(2);
--                bottomMargin: units.gu(2)
++                topMargin: units.gu(2) + units.dp(1)
++                bottomMargin: units.gu(2) + units.dp(2)
+             }
              sourceSize { width: icon.width; height: icon.height }
              asynchronous: true
 === added file 'Dash/Video/graphics/movie_icon_holder_carousel@27.png'
 Binary files Dash/Video/graphics/movie_icon_holder_carousel@27.png	1970-01-01 00:00:00 +0000 and Dash/Video/graphics/movie_icon_holder_carousel@27.png	2013-10-25 13:50:38 +0000 differ