Merge lp:~unity-team/unity8/previewcommentinput into lp:unity8

[Andrea Cimitan]

Implements a comment input widget

 * 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

 * 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?

added a new component as per visual spec.

 * Did you have a look at the warnings when running tests? Can they be reduced?

yes. only the old Theme.palette thing.

[Andrea Cimitan (cimi)]

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1795
http://jenkins.qa.ubuntu.com/job/unity8-ci/5788/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/67
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/66
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/66
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/67
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/67/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20869

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

review: Approve (continuous-integration)

[Michael Zanetti (mzanetti)]

Addressed all comments. Thanks.

[Lukáš Tinkl (lukas-kde)]

Minor optimizations/coding style issues, see inline comments

[Michael Zanetti (mzanetti)]

> Minor optimizations/coding style issues, see inline comments

fixed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1798
http://jenkins.qa.ubuntu.com/job/unity8-ci/5790/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/69
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/54
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/69
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/69/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20876

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

review: Approve (continuous-integration)

[Andrea Cimitan (cimi)]

Add this file to the test in previewwidgetfactory

review: Needs Fixing

[Michael Zanetti (mzanetti)]

> Add this file to the test in previewwidgetfactory

done

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1799
http://jenkins.qa.ubuntu.com/job/unity8-ci/5791/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/70
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/69
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/69
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/55
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/70
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/70/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

Text conflict in tests/qmltests/CMakeLists.txt
1 conflicts encountered.

[Michael Zanetti (mzanetti)]

> Text conflict in tests/qmltests/CMakeLists.txt
> 1 conflicts encountered.

merged

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1800
http://jenkins.qa.ubuntu.com/job/unity8-ci/5861/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/163
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/141
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/142
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/110
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/162
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/162/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21405

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

review: Needs Fixing (continuous-integration)

[Andrea Cimitan (cimi)]

Moved from TextField to TextArea

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1801
http://jenkins.qa.ubuntu.com/job/unity8-ci/5886/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/166
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/167
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/130
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/196
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/196/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21475

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

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

AFAICS we don't need the emitted field in the test_submit_data dictionary?

review: Needs Fixing

[Albert Astals Cid (aacid)]

We don't seem to need
    property real validInputRating: 1
    property real invalidInputRating: -1
either

review: Needs Fixing

[Albert Astals Cid (aacid)]

Can you please make test_submit actually check that the successeful submit emits triggered(widgetId, "commented", data), with data being {"comment": comment}?

review: Needs Fixing

[Albert Astals Cid (aacid)]

property alias commentText: commentTextArea.text
do we actually need/use this alias for anything? Maybe we can remove it?

review: Needs Information

[Albert Astals Cid (aacid)]

   implicitHeight: commentSubmitContainer.implicitHeight + anchors.topMargin
looks a bit weird given we don't specify any topMargin at all so i guess we should either remove anchors.topMargin from the equation or add anchors.bottomMargin ?

review: Needs Fixing

[Albert Astals Cid (aacid)]

Not asking you to change it but having "commentContainer" as a sole child of "root" makes me feel as if we could just kill it and move things up. Same for "commentSubmitContainer". This way we'd save a few binding evaluations and the code would be probably more flat/easy to understand. Any particular reason the code is like that?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1804
http://jenkins.qa.ubuntu.com/job/unity8-ci/5916/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/197
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/159
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/237
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21602

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

review: Needs Fixing (continuous-integration)

[lp:~unity-team/unity8/previewcommentinput]

1802. By Andrea Cimitan on 2015-07-06

Fixes as review

1803. By Andrea Cimitan on 2015-07-06

As albert suggests

[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?
Unrelated reasons

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

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1803
http://jenkins.qa.ubuntu.com/job/unity8-ci/6039/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/341/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/69
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/277
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/278
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/245/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/341
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/341/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21858

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

review: Needs Fixing (continuous-integration)

=== added file 'qml/Dash/Previews/PreviewCommentInput.qml'
--- qml/Dash/Previews/PreviewCommentInput.qml	1970-01-01 00:00:00 +0000
+++ qml/Dash/Previews/PreviewCommentInput.qml	2015-07-06 13:14:55 +0000
@@ -0,0 +1,66 @@
+/*
+ * 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.1
+import "../../Components"
+
+/*! \brief Preview widget for commenting.
+
+    The widget can show a field to enter a comment.
+    It is possible to customise the submit button's label by setting widgetData["submit-label"]
+    The successeful submit emits triggered(widgetId, "commented", data),
+    with data being {"comment": comment}.
+*/
+
+PreviewWidget {
+    id: root
+    implicitHeight: Math.max(commentTextArea.implicitHeight, submitButton.implicitHeight)
+
+    function submit() {
+        var data = { "comment": commentTextArea.text };
+        triggered(root.widgetId, "commented", data);
+    }
+
+    TextArea {
+        id: commentTextArea
+        objectName: "commentTextArea"
+        anchors {
+            top: parent.top
+            left: parent.left
+            right: submitButton.left
+            rightMargin: units.gu(1)
+        }
+        autoSize: true
+    }
+
+    Button {
+        id: submitButton
+        objectName: "submitButton"
+
+        readonly property bool readyToSubmit: commentTextArea.text.trim().length > 0
+
+        anchors {
+            top: parent.top
+            right: parent.right
+        }
+        color: readyToSubmit ? Theme.palette.selected.base : Theme.palette.normal.base
+        text: widgetData["submit-label"] || i18n.tr("Send")
+        onClicked: {
+            if (readyToSubmit) root.submit()
+        }
+    }
+}

=== modified file 'qml/Dash/Previews/PreviewWidgetFactory.qml'
--- qml/Dash/Previews/PreviewWidgetFactory.qml	2015-05-08 13:12:02 +0000
+++ qml/Dash/Previews/PreviewWidgetFactory.qml	2015-07-06 13:14:55 +0000
@@ -62,6 +62,7 @@
             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-06-18 19:23:08 +0000
+++ tests/qmltests/CMakeLists.txt	2015-07-06 13:14:55 +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 PreviewCommentInput)
 add_unity8_qmltest(Dash/Previews PreviewExpandable)
 add_unity8_qmltest(Dash/Previews PreviewHeader)
 add_unity8_qmltest(Dash/Previews PreviewImageGallery)

=== added file 'tests/qmltests/Dash/Previews/tst_PreviewCommentInput.qml'
--- tests/qmltests/Dash/Previews/tst_PreviewCommentInput.qml	1970-01-01 00:00:00 +0000
+++ tests/qmltests/Dash/Previews/tst_PreviewCommentInput.qml	2015-07-06 13:14:55 +0000
@@ -0,0 +1,85 @@
+/*
+ * 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 string validInputText: "Great!"
+    property string invalidInputText: ""
+
+    property var widgetDataNewLabels: { "submit-label": "TestSubmitLabel" }
+
+    PreviewCommentInput {
+        id: previewCommentInput
+        anchors.left: parent.left
+        anchors.right: parent.right
+        widgetData: widgetDataNewLabels
+        widgetId: "previewCommentInput"
+    }
+
+    SignalSpy {
+        id: spy
+        signalName: "triggered"
+    }
+
+    UT.UnityTestCase {
+        name: "PreviewCommentInputTest"
+        when: windowShown
+
+        property var commentTextArea: findChild(previewCommentInput, "commentTextArea")
+        property var submitButton: findChild(previewCommentInput, "submitButton")
+
+        function init() {
+            commentTextArea.text = "";
+        }
+
+        function test_labels() {
+            previewCommentInput.widgetData = widgetDataNewLabels;
+            compare(submitButton.text, widgetDataNewLabels["submit-label"]);
+        }
+
+        function test_submit_data() {
+            return [
+                {tag: "empty review", inputText: invalidInputText, emitted: false},
+                {tag: "filled review", inputText: validInputText, emitted: true},
+            ];
+        }
+
+        function test_submit(data) {
+            spy.clear();
+            spy.target = previewCommentInput;
+
+            commentTextArea.text = data.inputText;
+            mouseClick(submitButton);
+            if (!data.emitted) {
+                compare(spy.count, 0);
+            } else {
+                compare(spy.count, 1);
+                var args = spy.signalArguments[0];
+                compare(args[2]["comment"], data.inputText);
+            }
+        }
+    }
+}

=== modified file 'tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml'
--- tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml	2014-08-26 19:21:31 +0000
+++ tests/qmltests/Dash/Previews/tst_PreviewWidgetFactory.qml	2015-07-06 13:14:55 +0000
@@ -77,6 +77,7 @@
             return [
                 { tag: "Actions", type: "actions", source: "PreviewActions.qml" },
                 { tag: "Audio", type: "audio", source: "PreviewAudioPlayback.qml" },
+                { tag: "Comment Input", type: "comment-input", source: "PreviewCommentInput.qml" },
                 { tag: "Expandable", type: "expandable", source: "PreviewExpandable.qml" },
                 { tag: "Gallery", type: "gallery", source: "PreviewImageGallery.qml" },
                 { tag: "Header", type: "header", source: "PreviewHeader.qml" },