Unity 8

Merge lp:~paulliu/unity8/unity8-fixgenericpreview into lp:unity8

unity8-fixgenericpreview
Merge into trunk

Proposed by Ying-Chun Liu on 2013-09-26

Status:	Rejected
Rejected by:	Michael Zanetti on 2013-10-01
Proposed branch:	lp:~paulliu/unity8/unity8-fixgenericpreview
Merge into:	lp:unity8
Diff against target:	375 lines (+157/-95) 4 files modified Dash/Apps/AppPreview.qml (+16/-13) Dash/DashPreview.qml (+33/-62) Dash/Generic/GenericPreview.qml (+90/-19) Dash/Video/VideoPreview.qml (+18/-1)
To merge this branch:	bzr merge lp:~paulliu/unity8/unity8-fixgenericpreview
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2013-09-26	Disapprove on 2013-10-01
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2013-09-30
Review via email: mp+187762@code.launchpad.net

Commit message

Rearrange the previews to comply the design.

Description of the change

Rearrange the previews to comply the design.

Revision history for this message

Ying-Chun Liu (paulliu) wrote on 2013-09-26:

https://bugs.launchpad.net/unity8/+bug/1224555

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-09-26:

Videos:

What it is: http://wstaw.org/m/2013/09/26/plasma-desktopS30440.png
What it should be: http://wstaw.org/m/2013/09/26/plasma-desktopB30440.png

* It seems the text is too large. Also I guess it shouldn't wrap but elide.
* Lacks rating stars

Apps:
What it is: http://wstaw.org/m/2013/09/26/plasma-desktopC30440.png
What it should be: http://wstaw.org/m/2013/09/26/plasma-desktopy30440.png

* The title is double here. The one at the top should look exactly like the one at the bottom. The bottom one needs to go away

Overall some margins seem to be wrong. I.e. in the code the stuff is clipped 2 grid units from the bottom while the design spec doesn't do that clipping there.

review: Needs Fixing

lp:~paulliu/unity8/unity8-fixgenericpreview updated on 2013-09-26

349. By Albert Astals Cid on 2013-09-26

Remember the expanded categoryId and not the expanded index

The index can change on search, and we still want to maintain it expanded in that case. Fixes: https://bugs.launchpad.net/bugs/1230216.

Approved by PS Jenkins bot, Michael Zanetti.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-26:

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

review: Needs Fixing (continuous-integration)

lp:~paulliu/unity8/unity8-fixgenericpreview updated on 2013-09-27

350. By Michał Sawicz on 2013-09-26

Make SHOW_DASH and HIDE_DASH close the current preview. Fixes: https://bugs.launchpad.net/bugs/1231404.

Approved by PS Jenkins bot, Michał Sawicz, Diego Sarmentero.

351. By Diego Sarmentero on 2013-09-26

- Handling error signal from the DownloadTracker plugin (BUG: #1229744). Fixes: https://bugs.launchpad.net/bugs/1229744.

Approved by PS Jenkins bot, Michał Sawicz, Roberto Alsina.

352. By Diego Sarmentero on 2013-09-26

- Remove "Reviews and Comments" section from Application Preview until the feature is ready (BUG: #1226632)
- Detect when the keyboard is being shown to allow the user to scroll the Preview even more if necessary to interact with the components at the bottom of that preview, and don't leave those components obscured behind the keyboard (BUG: #1226638). Fixes: https://bugs.launchpad.net/bugs/1224717, https://bugs.launchpad.net/bugs/1226632, https://bugs.launchpad.net/bugs/1226638.

Approved by PS Jenkins bot, Michał Sawicz, Alejandro J. Cura.

353. By Nick Dedekind on 2013-09-26

Brought messaging indicator inline with UnityMenuModel & UnityMenuAction. Fixes: https://bugs.launchpad.net/bugs/1217676, https://bugs.launchpad.net/bugs/1217678.

Approved by PS Jenkins bot, Michael Zanetti.

354. By Albert Astals Cid on 2013-09-26

Fix showHeader in an edge case of notShownByItsOwn

Not all the tests i've added fail without the code fix, but i've added them just to be more covered
. Fixes: https://bugs.launchpad.net/bugs/1230187.

Approved by PS Jenkins bot, Michał Sawicz.

355. By Launchpad Translations on behalf of unity-team on 2013-09-27

Launchpad automatic translations update.

356. By Michał Sawicz on 2013-09-27

Add a LazyImage component that shows an activity spinner for long-loading images and handles aspect ratio properly.

Approved by PS Jenkins bot, Michael Zanetti.

357. By Michał Sawicz on 2013-09-27

Fix Qt 5.1 FTBFS and suppress some build warnings.

Approved by PS Jenkins bot, Michael Zanetti.

Revision history for this message

Ying-Chun Liu (paulliu) wrote on 2013-09-27:

Hi, I'll fix the video preview in another branch. lp:~paulliu/unity8/movie-preview

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-27:

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

review: Needs Fixing (continuous-integration)

lp:~paulliu/unity8/unity8-fixgenericpreview updated on 2013-09-27

358. By Paweł Stołowski on 2013-09-27

Cancel previous actions and previews on new activation / preview. Expose previewed data row in Preview object.

Approved by PS Jenkins bot, Michal Hruby, Michał Sawicz.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-27:

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

review: Needs Fixing (continuous-integration)

lp:~paulliu/unity8/unity8-fixgenericpreview updated on 2013-09-30

359. By PS Jenkins bot on 2013-09-27

Releasing 7.81.3+13.10.20130927.3-0ubuntu1 (revision 358 from lp:unity8).

Approved by PS Jenkins bot.

360. By Launchpad Translations on behalf of unity-team on 2013-09-29

Launchpad automatic translations update.

361. By Launchpad Translations on behalf of unity-team on 2013-09-30

Launchpad automatic translations update.

364. By Ying-Chun Liu on 2013-09-27

Fix app preview.

365. By Ying-Chun Liu on 2013-09-27

Add Displaying the infoHints.

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-09-30:

From the design spec: http://i.imgur.com/up8dETX.png

I see we have Preview, Header, Actions, Description and Rating as elements on the preview. Our code is still structured for the old previews and the new elements somehow squeezed into the old structure. I think in the long run this will cause continuous headaches and would vote for properly doing the rework, splitting up DashPreview.qml into exactly those defined fields. Another thing is that the current code still holds id's etc referring to the old structure. For example the id for the area that holds the screenshots (in the design spec called "Preview") is still named headerIcon in the code. This is quite a mess imho.

Also,
This is how it looks in landscape mode http://i.imgur.com/nAknW9T.jpg , while this is how it should look like http://i.imgur.com/uwHyln6.jpg

I guess that's a side effect of not having it structured properly.

review: Needs Fixing

Revision history for this message

Michal Hruby (mhr3) wrote on 2013-09-30:

@mzanetti: Isn't that issue with the AppPreview? This branch says it fixes the GenericPreview, not music, not apps.

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-09-30:

No... its really about the structure of the whole thing. The underlaying DashPreview is still the old structure and the new look is squeezed in there somehow.

Revision history for this message

Ying-Chun Liu (paulliu) wrote on 2013-09-30:

ok.. I'll modify it now. But because generic preview doesn't provide any ratings, I'll call it leftPane, middlePane, and rightPanel. And then I'll use Grid for arranging these panes according to the narrowMode.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-30:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Rejecting this because of the complete preview rework in here: https://code.launchpad.net/~unity-team/unity8/fix-genericpreview/+merge/188355

review: Disapprove

Unmerged revisions

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:

Mohammad Rakibul Hasan

Unity Team

Ying-Chun Liu

 === modified file 'Dash/Apps/AppPreview.qml'
 --- Dash/Apps/AppPreview.qml	2013-09-26 18:55:06 +0000
 +++ Dash/Apps/AppPreview.qml	2013-09-30 08:54:15 +0000
@@ -26,8 +26,6 @@
      signal sendUserReview(string review)
--    title: root.previewData.title
--
      header: ListView {
          spacing: units.gu(1)
          orientation: ListView.Horizontal
@@ -57,6 +55,22 @@
+         }
+     }
++    title: AppInfo {
++        objectName: "appInfo"
++        anchors {
++            top: parent.top
++            topMargin: units.gu(2)
++            left: parent.left
++            right: parent.right
++        }
++
++        appName: root.previewData.title
++        icon: root.previewData.appIcon
++        rating: Math.round(root.previewData.rating * 10)
++        reviews: root.previewData.numRatings
++        rated: root.previewData.infoMap["rated"] ? root.previewData.infoMap["rated"].value : 0
++    }
++
      Component {
          id: buttonsComponent
@@ -135,17 +149,6 @@
      body: Column {
          spacing: units.gu(1)
--        AppInfo {
--            objectName: "appInfo"
--            anchors { left: parent.left; right: parent.right }
--
--            appName: root.previewData.title
--            icon: root.previewData.appIcon
--            rating: Math.round(root.previewData.rating * 10)
--            reviews: root.previewData.numRatings
--            rated: root.previewData.infoMap["rated"] ? root.previewData.infoMap["rated"].value : 0
--        }
--
          Label {
              anchors { left: parent.left; right: parent.right }
              text: root.previewData.description
 === modified file 'Dash/DashPreview.qml'
 --- Dash/DashPreview.qml	2013-09-23 12:24:42 +0000
 +++ Dash/DashPreview.qml	2013-09-30 08:54:15 +0000
@@ -23,10 +23,10 @@
      property int keyboardSize: Qt.inputMethod.visible ? Qt.inputMethod.keyboardRectangle.height : 0
      property var previewData
--    property string title: ""
      property real previewWidthRatio: 0.5
      property Component header
++    property Component title
      property Component buttons
      property Component body
@@ -62,64 +62,39 @@
+     }
      Item {
++        id: headerIcon
++        height: childrenRect.height
++        anchors {
++            left: parent.left
++            right: parent.right
++            top: parent.top
++            leftMargin: root.contentSpacing
++            topMargin: root.contentSpacing
++            rightMargin: root.contentSpacing
++        }
++        Loader {
++            id: headerLoader
++            anchors.left: parent.left
++            anchors.right: parent.right
++            sourceComponent: root.header
++        }
++    }
++
++    Item {
          id: headerRow
--        height: units.gu(4)
++        height: childrenRect.height
          anchors {
--            top: parent.top
++            top: headerIcon.bottom
              left: parent.left
              right: parent.right
--            margins: root.contentSpacing
--        }
--
--        MouseArea {
--            anchors {
--                fill: parent
--                leftMargin: -root.contentSpacing
--                rightMargin: -root.contentSpacing
--                topMargin: -root.contentSpacing
--            }
--
--            onClicked: root.close();
--        }
--
--        Item {
--            id: labelItem
--            anchors {
--                fill: parent
--                rightMargin: closePreviewImage.width + spacing
--            }
--
--            property int spacing: units.gu(2)
--
--            Label {
--                id: title
--                objectName: "titleLabel"
--                anchors {
--                    left: parent.left
--                    right: parent.right
--                }
--
--                elide: Text.ElideRight
--                fontSize: "x-large"
--                font.weight: Font.Light
--                color: Theme.palette.selected.backgroundText
--                opacity: 0.9
--                text: root.title
--                style: Text.Raised
--                styleColor: "black"
--            }
--            Image {
--                id: closePreviewImage
--                source: "graphics/tablet/icon_close_preview.png"
--                width: units.gu(4)
--                height: units.gu(1.5)
--                anchors {
--                    bottom: title.bottom
--                    bottomMargin: units.dp(7)
--                    left: parent.left
--                    leftMargin: title.paintedWidth + labelItem.spacing
--                }
--            }
++            leftMargin: root.contentSpacing
++            rightMargin: root.contentSpacing
++        }
++        Loader {
++            id: titleLoader
++            anchors.left: parent.left
++            anchors.right: parent.right
++            sourceComponent: root.title
+         }
+     }
@@ -130,7 +105,9 @@
              right: parent.right
              top: headerRow.bottom
              bottom: parent.bottom
--            margins: root.contentSpacing
++            topMargin: root.contentSpacing
++            leftMargin: root.contentSpacing
++            rightMargin: root.contentSpacing
+         }
          spacing: units.gu(2)
@@ -158,12 +135,6 @@
                  spacing: units.gu(1)
                  Loader {
--                    id: headerLoader
--                    anchors.left: parent.left
--                    anchors.right: parent.right
--                    sourceComponent: root.header
--                }
--                Loader {
                      id: buttonLoader
                      anchors.left: parent.left
                      anchors.right: parent.right
 === modified file 'Dash/Generic/GenericPreview.qml'
 --- Dash/Generic/GenericPreview.qml	2013-09-05 10:02:00 +0000
 +++ Dash/Generic/GenericPreview.qml	2013-09-30 08:54:15 +0000
@@ -23,7 +23,6 @@
  DashPreview {
      id: genericPreview
--    title: previewData.title
      previewWidthRatio: 0.6
      property url url: previewData.image
@@ -42,17 +41,60 @@
+         }
+     }
++    title: Column {
++        anchors {
++            top: parent.top
++            topMargin: units.gu(2)
++            left: parent.left
++            right: parent.right
++        }
++
++        Label {
++            id: title
++            objectName: "titleLabel"
++            anchors { left: parent.left; right: parent.right }
++
++            elide: Text.ElideRight
++            fontSize: "x-large"
++            font.weight: Font.Light
++            color: Theme.palette.selected.backgroundText
++            opacity: 0.9
++            text: previewData.title
++            style: Text.Raised
++            styleColor: "black"
++            maximumLineCount: 2
++            wrapMode: Text.WordWrap
++        }
++
++        Label {
++            anchors { left: parent.left; right: parent.right }
++            visible: text != ""
++            fontSize: "medium"
++            opacity: 0.6
++            color: "white"
++            text: previewData.subtitle.replace(/[\r\n]+/g, "<br />")
++            style: Text.Raised
++            styleColor: "black"
++            wrapMode: Text.WordWrap
++            textFormat: Text.RichText
++            maximumLineCount: 1
++            // FIXME: workaround for https://bugreports.qt-project.org/browse/QTBUG-33020
++            onWidthChanged: { wrapMode = Text.NoWrap; wrapMode = Text.WordWrap }
++        }
++    }
      buttons: GridView {
          id: buttons
          model: genericPreview.previewData.actions
--        property int numOfRows: (count + 1) / 2
++        property int buttonMaxWidth: units.gu(34)
++        property int numOfColumns: Math.ceil((width + spacing) / (buttonMaxWidth + spacing))
++        property int numOfRows: Math.ceil(count / numOfColumns)
          property int spacing: units.gu(1)
          height: Math.max(units.gu(4), units.gu(4)*numOfRows + spacing*(numOfRows - 1))
--        cellWidth: Math.max(units.gu(9), width / 2)
++        cellWidth: width / numOfColumns
          cellHeight: buttonHeight + spacing
--        property int buttonWidth: Math.max(0, width / 2 - spacing)
++        property int buttonWidth: cellWidth - spacing / 2
          property int buttonHeight: units.gu(4)
          delegate: Button {
@@ -69,25 +111,12 @@
+         }
          focus: false
+     }
++
      body: Column {
          spacing: units.gu(2)
          Label {
--            anchors { left: parent.left; right: parent.right }
--            visible: text != ""
--            fontSize: "medium"
--            opacity: 0.6
--            color: "white"
--            text: previewData.subtitle.replace(/[\r\n]+/g, "<br />")
--            style: Text.Raised
--            styleColor: "black"
--            wrapMode: Text.WordWrap
--            textFormat: Text.RichText
--            // FIXME: workaround for https://bugreports.qt-project.org/browse/QTBUG-33020
--            onWidthChanged: { wrapMode = Text.NoWrap; wrapMode = Text.WordWrap }
--        }
--
--        Label {
++            id: descriptionLabel
              anchors { left: parent.left; right: parent.right }
              visible: text != ""
              fontSize: "small"
@@ -101,5 +130,47 @@
              // FIXME: workaround for https://bugreports.qt-project.org/browse/QTBUG-33020
              onWidthChanged: { wrapMode = Text.NoWrap; wrapMode = Text.WordWrap }
+         }
++
++        Column {
++            id: infoItem
++            anchors {
++                left: parent.left
++                right: parent.right
++            }
++            Repeater {
++                model: previewData.infoHints
++
++                delegate: Grid {
++                    columns: 2
++                    width: parent.width
++                    spacing: units.gu(1)
++
++                    Label {
++                        visible: directedLabel.visible
++                        fontSize: "small"
++                        opacity: 0.9
++                        color: "white"
++                        horizontalAlignment: Text.AlignLeft
++                        width: infoItem.width / 2
++                        text: modelData.displayName
++                        style: Text.Raised
++                        styleColor: "black"
++                    }
++                    Label {
++                        id: directedLabel
++                        visible: modelData.value != ""
++                        fontSize: "small"
++                        opacity: 0.6
++                        color: "white"
++                        horizontalAlignment: Text.AlignRight
++                        width: infoItem.width / 2
++                        text: modelData.value ? modelData.value : ""
++                        style: Text.Raised
++                        styleColor: "black"
++                        wrapMode: Text.WordWrap
++                    }
++                }
++            }
++        }
+     }
+ }
 === modified file 'Dash/Video/VideoPreview.qml'
 --- Dash/Video/VideoPreview.qml	2013-08-09 10:12:19 +0000
 +++ Dash/Video/VideoPreview.qml	2013-09-30 08:54:15 +0000
@@ -33,7 +33,6 @@
          source: item ? item.nfoUri : ""
+     }
--    title: nfo.ready ? nfo.video.title : ""
      previewWidthRatio: 0.6
      onPreviewImageClicked: {
@@ -74,6 +73,24 @@
+         }
+     }
++    title: Label {
++        id: title
++        objectName: "titleLabel"
++        anchors {
++            left: parent.left
++            right: parent.right
++        }
++
++        elide: Text.ElideRight
++        fontSize: "x-large"
++        font.weight: Font.Light
++        color: Theme.palette.selected.backgroundText
++        opacity: 0.9
++        text: nfo.ready ? nfo.video.title : ""
++        style: Text.Raised
++        styleColor: "black"
++    }
++
      buttons: Row {
          spacing: units.gu(2)