Merge lp:~cimi/unity8/previewSocialComment into lp:unity8

[Andrea Cimitan]

Add PreviewSocialComment

 * Are there any related MPs required for this MP to build/function as expected? Please list.
No, but needs backend to hook in order to show up
 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes
 * Did you make sure that your branch does not contain spurious tags?
Yes
 * 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?
yes, it's as in the specs

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1802
http://jenkins.qa.ubuntu.com/job/unity8-ci/5898/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/213/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/178
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/179
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/141/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/213
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/213/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21570

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

review: Needs Fixing (continuous-integration)

[Josh Arenson (josharenson)]

See inline comment

review: Needs Fixing

[Albert Astals Cid (aacid)]

spacing: units.gu(0.24)?

Seems a bit arbitrary-ish is that even an integer number of pixels?

review: Needs Information

[Albert Astals Cid (aacid)]

Set elide: Text.ElideRight for the date too, i don't think we'll ever need it but it doesn't hurt just in case.

review: Needs Fixing

[Albert Astals Cid (aacid)]

Why is the image anchored to the top without margins but the text actually has margins? feels a bit weird, is that what design wants?

review: Needs Information

[Albert Astals Cid (aacid)]

When there's no date the text from the comment doesn't go up, is that because the date is mandatory?

review: Needs Information

[Albert Astals Cid (aacid)]

I'm a bit concerned about
   implicitHeight: column.implicitHeight
i know that at the moment the column is always taller than the avatar, but i can see a gu change or text font size change making this not true, should it have some Math.max also considering the avatar?

review: Needs Information

[Albert Astals Cid (aacid)]

visible: source.status === Image.Ready is a bit weird since for http images (that i guess it's what we will use mainly there?) it means that first the text will be full width and then will be relayouted to include the image.

That feels a bit weird, what do you think?

To test, replace
  "source": "../../graphics/avatars/[e-mail]"
with
  "source": "https://assets.ubuntu.com/sites/ubuntu/latest/u/img/homepage/kylin/meizu-takeover.jpg"
and run
  make tryPreviewSocialComment

review: Needs Information

[Albert Astals Cid (aacid)]

[11:20:18] <patriciadavila> tsdgeos: if there's not subtitle (date & time info) available, yes, we shall move it {the comment} up :)

This probably means we really need the max calculation in the height now.

review: Needs Fixing

[lp:~cimi/unity8/previewSocialComment]

1803. By Andrea Cimitan on 2015-07-01

Subtitle is optional

1804. By Andrea Cimitan on 2015-07-01

Renames

[Andrea Cimitan (cimi)]

> visible: source.status === Image.Ready is a bit weird since for http images
> (that i guess it's what we will use mainly there?) it means that first the
> text will be full width and then will be relayouted to include the image.
>
> That feels a bit weird, what do you think?
>
> To test, replace
> "source": "../../graphics/avatars/[e-mail]"
> with
> "source":
> "https://assets.ubuntu.com/sites/ubuntu/latest/u/img/homepage/kylin/meizu-
> takeover.jpg"
> and run
> make tryPreviewSocialComment

I see... but that is what we do in CardCreator too... anything else?

[Andrea Cimitan (cimi)]

> spacing: units.gu(0.24)?
>
> Seems a bit arbitrary-ish is that even an integer number of pixels?
That's the only value that give me the text aligned like in the mockups. Might be that the ubuntu font might not be precisely 1gu

[Andrea Cimitan (cimi)]

> Why is the image anchored to the top without margins but the text actually has
> margins? feels a bit weird, is that what design wants?

From the mockups, yes

[lp:~cimi/unity8/previewSocialComment]

1805. By Andrea Cimitan on 2015-07-01

More changes for review

[Albert Astals Cid (aacid)]

> > visible: source.status === Image.Ready is a bit weird since for http images
> > (that i guess it's what we will use mainly there?) it means that first the
> > text will be full width and then will be relayouted to include the image.
> >
> > That feels a bit weird, what do you think?
> >
> > To test, replace
> > "source": "../../graphics/avatars/[e-mail]"
> > with
> > "source":
> > "https://assets.ubuntu.com/sites/ubuntu/latest/u/img/homepage/kylin/meizu-
> > takeover.jpg"
> > and run
> > make tryPreviewSocialComment
>
> I see... but that is what we do in CardCreator too... anything else?

But CardCreator works and this one doesn't have you seen any card created by the cardcreator have the text jumping because the image loads async? no, this one does.

review: Needs Fixing

[lp:~cimi/unity8/previewSocialComment]

1806. By Andrea Cimitan on 2015-07-02

Use different method for checking if source is set

[Albert Astals Cid (aacid)]

what about adding "opacity: source.status === Image.Ready ? 1 : 0 " to the ubuntushape?

review: Needs Information

[lp:~cimi/unity8/previewSocialComment]

1807. By Andrea Cimitan on 2015-07-06

Merged

[Albert Astals Cid (aacid)]

 * 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.
It's all weird :/

 * Did you make sure that the branch does not contain spurious tags?
Yes

review: Approve

[Josh Arenson (josharenson)]

 * 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.
I don't see a recent CI run

 * Did you make sure that the branch does not contain spurious tags?
Yes

Left a comment inline, but approving since this is already top level approved and the suggestion isn't a big deal.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1807
http://jenkins.qa.ubuntu.com/job/unity8-ci/6048/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/350
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/286
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/287
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/254
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/350
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/350/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21867

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

review: Needs Fixing (continuous-integration)

=== added file 'qml/Dash/Previews/PreviewComment.qml'
--- qml/Dash/Previews/PreviewComment.qml	1970-01-01 00:00:00 +0000
+++ qml/Dash/Previews/PreviewComment.qml	2015-07-06 15:33:26 +0000
@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.0
+import Ubuntu.Components 1.2
+
+/*! \brief Preview widget for comments.
+
+    This widget shows an (optional) avatar contained in widgetData["source"]
+    along with a label that comes from widgetData["author"],
+    a (optional) subtitle from widgetData["subtitle"] and the comment widgetData["comment"].
+*/
+
+PreviewWidget {
+    id: root
+    implicitHeight: Math.max(avatar.height, column.implicitHeight)
+
+    UbuntuShape {
+        id: avatar
+        objectName: "avatar"
+        anchors {
+            left: parent.left
+            top: parent.top
+        }
+        width: units.gu(6)
+        height: width
+        source: Image {
+            source: widgetData["source"]
+        }
+        radius: "medium"
+        opacity: source.status === Image.Ready ? 1 : 0
+        visible: widgetData["source"] !== ""
+    }
+
+    Column {
+        id: column
+        objectName: "column"
+        anchors {
+            left: avatar.visible ? avatar.right : parent.left
+            right: parent.right
+            top: parent.top
+            topMargin: units.gu(0.5)
+            leftMargin: avatar.visible ? units.gu(1) : 0
+        }
+        spacing: units.gu(0.24)
+
+        Label {
+            width: parent.width
+            text: widgetData["author"] || ""
+            fontSize: "small"
+            maximumLineCount: 1
+            elide: Text.ElideRight
+        }
+        Label {
+            objectName: "subtitle"
+            width: parent.width
+            visible: text !== ""
+            text: widgetData["subtitle"] || ""
+            fontSize: "xx-small"
+            maximumLineCount: 1
+            elide: Text.ElideRight
+        }
+        Label {
+            width: parent.width
+            text: widgetData["comment"] || ""
+            fontSize: "small"
+            wrapMode: Text.Wrap
+        }
+    }
+}

=== modified file 'qml/Dash/Previews/PreviewWidgetFactory.qml'
--- qml/Dash/Previews/PreviewWidgetFactory.qml	2015-07-06 15:33:26 +0000
+++ qml/Dash/Previews/PreviewWidgetFactory.qml	2015-07-06 15:33:26 +0000
@@ -50,6 +50,8 @@
         switch (widgetType) {
             case "actions": return "PreviewActions.qml";
             case "audio": return "PreviewAudioPlayback.qml";
+            case "comment": return "PreviewComment.qml";
+            case "comment-input": return "PreviewCommentInput.qml";
             case "expandable": return "PreviewExpandable.qml";
             case "gallery": return "PreviewImageGallery.qml";
             case "header": return "PreviewHeader.qml";
@@ -62,7 +64,6 @@
             case "table": return "PreviewTable.qml";
             case "text": return "PreviewTextSummary.qml";
             case "video": return "PreviewVideoPlayback.qml";
-            case "comment-input": return "PreviewCommentInput.qml";
             default: return "";
         }
     }

=== modified file 'tests/qmltests/CMakeLists.txt'
--- tests/qmltests/CMakeLists.txt	2015-07-06 15:33:26 +0000
+++ tests/qmltests/CMakeLists.txt	2015-07-06 15:33:26 +0000
@@ -27,6 +27,7 @@
 add_unity8_qmltest(Dash/Previews Preview)
 add_unity8_qmltest(Dash/Previews PreviewActions)
 add_unity8_qmltest(Dash/Previews PreviewAudioPlayback)
+add_unity8_qmltest(Dash/Previews PreviewComment)
 add_unity8_qmltest(Dash/Previews PreviewCommentInput)
 add_unity8_qmltest(Dash/Previews PreviewExpandable)
 add_unity8_qmltest(Dash/Previews PreviewHeader)

=== added file 'tests/qmltests/Dash/Previews/tst_PreviewComment.qml'
--- tests/qmltests/Dash/Previews/tst_PreviewComment.qml	1970-01-01 00:00:00 +0000
+++ tests/qmltests/Dash/Previews/tst_PreviewComment.qml	2015-07-06 15:33:26 +0000
@@ -0,0 +1,84 @@
+/*
+ * Copyright 2015 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.0
+import QtTest 1.0
+import Ubuntu.Components 1.1
+import "../../../../qml/Dash/Previews"
+import Unity.Test 0.1 as UT
+
+Rectangle {
+    id: root
+    width: units.gu(40)
+    height: units.gu(80)
+    color: Theme.palette.selected.background
+
+    property var comment: {
+        "author": "Claire Thompson",
+        "subtitle": "28/04/2015 3:48pm",
+        "comment": "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.",
+        "source": "../../graphics/avatars/[e-mail]"
+    }
+
+    property var commentNoImage: {
+        "author": "Claire Thompson",
+        "subtitle": "28/04/2015 3:48pm",
+        "comment": "Been quite a while since I really liked one.",
+        "source": ""
+    }
+
+    property var commentNoSubtitle: {
+        "author": "Claire Thompson",
+        "comment": "Been quite a while since I really liked one.",
+        "source": "../../graphics/avatars/[e-mail]"
+    }
+
+    PreviewComment {
+        id: previewComment
+        anchors.left: parent.left
+        anchors.right: parent.right
+        widgetData: comment
+        widgetId: "previewComment"
+    }
+
+    UT.UnityTestCase {
+        name: "PreviewCommentTest"
+        when: windowShown
+
+        function init() {
+            previewComment.widgetData = comment;
+        }
+
+        function test_AnchorsNoAvatar() {
+            var column = findChild(previewComment, "column");
+            var avatar = findChild(previewComment, "avatar");
+
+            compare(previewComment.widgetData, comment);
+            compare(avatar.visible, true);
+            compare(column.anchors.left, avatar.anchors.right);
+
+            previewComment.widgetData = commentNoImage;
+            tryCompare(avatar, "visible", false);
+            compare(column.anchors.left, previewComment.anchors.left);
+        }
+
+        function test_OptionalSubtitle() {
+            var subtitle = findChild(previewComment, "subtitle");
+            previewComment.widgetData = commentNoSubtitle;
+            tryCompare(subtitle, "visible", false);
+        }
+    }
+}

=== modified file 'tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml'
--- tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml	2015-07-06 15:33:26 +0000
+++ tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml	2015-07-06 15:33:26 +0000
@@ -77,6 +77,7 @@
             return [
                 { tag: "Actions", type: "actions", source: "PreviewActions.qml" },
                 { tag: "Audio", type: "audio", source: "PreviewAudioPlayback.qml" },
+                { tag: "Comment", type: "comment", source: "PreviewComment.qml" },
                 { tag: "Comment Input", type: "comment-input", source: "PreviewCommentInput.qml" },
                 { tag: "Expandable", type: "expandable", source: "PreviewExpandable.qml" },
                 { tag: "Gallery", type: "gallery", source: "PreviewImageGallery.qml" },