Unity 8

Merge lp:~diegosarmentero/unity8/unity-rates-and-scroll into lp:unity8

unity-rates-and-scroll
Merge into trunk

Proposed by Diego Sarmentero on 2013-09-17

Status:

Merged

Approved by:

Diego Sarmentero on 2013-09-26

Approved revision:

352

Merged at revision:

352

Proposed branch:

lp:~diegosarmentero/unity8/unity-rates-and-scroll

Merge into:

lp:unity8

Diff against target:

143 lines (+45/-0)

4 files modified

Dash/Apps/AppPreview.qml (+7/-0)
Dash/Apps/AppReviews.qml (+3/-0)
Dash/DashPreview.qml (+13/-0)
tests/qmltests/Dash/Apps/tst_AppPreview.qml (+22/-0)

To merge this branch:

bzr merge lp:~diegosarmentero/unity8/unity-rates-and-scroll

Related bugs:

Bug #1224717: App preview screen does not resize correctly when keyboard is shown. Potentially hides text input.	Undecided	Fix Released
Bug #1226632: Disable Rates and Comments sections from Application Preview until the feature is ready	Medium	Fix Released
Bug #1226638: Application Preview get cut off on keyboard shown	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-09-26
Michał Sawicz			Approve on 2013-09-26
Alejandro J. Cura (community)		2013-09-17	Approve on 2013-09-17
Review via email: mp+186131@code.launchpad.net

Commit message

- 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).

Description of the change

Remove the "Reviews and Comments" section for the Application Preview until the feature is complete (as requested by design).
Add the size of the keyboard to the content height of the scrollable area of any preview if the keyboard is being shown so the components at the bottom of that preview are not covered by the keyboard, and the user can keep scrolling the preview to interact with that part of the page.
Fixed and added tests related to this.

Revision history for this message

Alejandro J. Cura (alecu) wrote on 2013-09-17:

Looks good

review: Approve

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-09-17:

You should use bottomMargin for the keyboard size. [1]

You should also scroll the content when keyboard size changes, so that you're not left with the keyboard obscuring the text field you've just focused.

Also, use skip() instead of commenting out the tests. Same for the AppReviews itself - just make it invisible to reduce diff size and ease the review.

[1] http://qt-project.org/doc/qt-5.0/qtquick/qml-qtquick2-flickable.html#bottomMargin-prop

review: Needs Fixing

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-09-26:

Something's wrong here, when I enable the ratings (visible: true) and focus the text field, I get:
http://ubuntuone.com/5aRajAiMMtuiecHwESy9TY

review: Needs Fixing

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-09-26:

As discussed, let's merge as-is right now, since the issue isn't visible anyway. There will be a bug opened for the issue I've mentioned above.

review: Approve

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/1140/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

review: Needs Fixing (continuous-integration)

lp:~diegosarmentero/unity8/unity-rates-and-scroll updated on 2013-09-26

352. By Diego Sarmentero on 2013-09-26: resolve conflict

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/1150/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2013-09-26:

One more comment you should add to the bug "for later": you should use UbuntuNumberAnimations¹ and related durations².

¹ http://developer.ubuntu.com/api/ubuntu-12.10/qml/mobile/qml-ubuntu-components0-ubuntunumberanimation.html
² http://developer.ubuntu.com/api/ubuntu-12.10/qml/mobile/qml-ubuntu-components0-ubuntuanimation.html#details

Revision history for this message

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

review: Approve (continuous-integration)

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:

Diego Sarmentero

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'Dash/Apps/AppPreview.qml'
2	--- Dash/Apps/AppPreview.qml 2013-09-24 13:54:29 +0000
3	+++ Dash/Apps/AppPreview.qml 2013-09-26 18:55:40 +0000
4	@@ -191,12 +191,19 @@
5	ListItem.ThinDivider { }
6
7	AppReviews {
8	+ id: appReviews
9	objectName: "appReviews"
10	+ // TODO: This should make this visible when the feature for reviews/comments is complete.
11	+ visible: false
12	anchors { left: parent.left; right: parent.right }
13
14	model: root.previewData.infoMap["comments"] ? root.previewData.infoMap["comments"].value : undefined
15
16	onSendReview: root.sendUserReview(review);
17	+
18	+ onEditing: {
19	+ root.ensureVisible(appReviews.textArea);
20	+ }
21	}
22	}
23	}
24
25	=== modified file 'Dash/Apps/AppReviews.qml'
26	--- Dash/Apps/AppReviews.qml 2013-08-09 09:37:50 +0000
27	+++ Dash/Apps/AppReviews.qml 2013-09-26 18:55:40 +0000
28	@@ -23,8 +23,10 @@
29	id: root
30
31	property var model
32	+ property alias textArea: reviewField
33
34	signal sendReview(string review)
35	+ signal editing
36
37	spacing: units.gu(2)
38	state: ""
39	@@ -93,6 +95,7 @@
40	if(reviewField.focus){
41	root.state = "editing";
42	reviewField.selectAll();
43	+ root.editing();
44	}
45	}
46
47
48	=== modified file 'Dash/DashPreview.qml'
49	--- Dash/DashPreview.qml 2013-08-09 09:48:56 +0000
50	+++ Dash/DashPreview.qml 2013-09-26 18:55:40 +0000
51	@@ -20,6 +20,7 @@
52	Rectangle {
53	id: root
54
55	+ property int keyboardSize: Qt.inputMethod.visible ? Qt.inputMethod.keyboardRectangle.height : 0
56	property var previewData
57
58	property string title: ""
59	@@ -38,6 +39,14 @@
60	color: Qt.rgba(0, 0, 0, .3)
61	clip: true
62
63	+ function ensureVisible(item) {
64	+ var o = leftFlickable.mapFromItem(item, 0, 0, item.width, item.height);
65	+ var keyboardY = shell.height - root.keyboardSize;
66	+ if ((o.y + o.height) > keyboardY) {
67	+ leftFlickable.contentY += o.y + o.height - keyboardY;
68	+ }
69	+ }
70	+
71	Connections {
72	target: shell.applicationManager
73	onMainStageFocusedApplicationChanged: {
74	@@ -128,12 +137,16 @@
75
76	Flickable {
77	id: leftFlickable
78	+ objectName: "leftFlickable"
79	anchors.top: parent.top
80	anchors.bottom: parent.bottom
81	width: root.narrowMode ? contentRow.width : contentRow.width * root.previewWidthRatio
82	contentHeight: leftColumn.height
83	+ anchors.bottomMargin: root.keyboardSize
84	clip: true
85
86	+ Behavior on contentY { NumberAnimation { duration: 300 } }
87	+
88	Column {
89	id: leftColumn
90	objectName: "leftColumn"
91
92	=== modified file 'tests/qmltests/Dash/Apps/tst_AppPreview.qml'
93	--- tests/qmltests/Dash/Apps/tst_AppPreview.qml 2013-09-25 19:58:58 +0000
94	+++ tests/qmltests/Dash/Apps/tst_AppPreview.qml 2013-09-26 18:55:40 +0000
95	@@ -25,6 +25,9 @@
96	width: units.gu(60)
97	height: units.gu(80)
98
99	+ // Fake shell
100	+ QtObject { id: shell; property int height: appPreview.height }
101	+
102	property var calls: []
103
104	SignalSpy {
105	@@ -114,6 +117,9 @@
106	reviewField.text = "";
107	data.rating = rating.value;
108	dataProgress.infoMap["progressbar_source"].value = "service";
109	+ var appReviews = findChild(appPreview, "appReviews");
110	+ appReviews.visible = false;
111	+ appPreview.keyboardSize = 0;
112	}
113
114	function test_actions() {
115	@@ -149,6 +155,7 @@
116
117	function test_review_focus() {
118	var appReviews = findChild(appPreview, "appReviews");
119	+ appReviews.visible = true;
120	var sendButton = findChild(appReviews, "sendButton");
121	var reviewField = findChild(appReviews, "reviewField");
122
123	@@ -213,5 +220,20 @@
124	compare(root.calls[0][1], {"error": "DOWNLOAD ERROR"}, "Data of action not found.");
125	}
126
127	+ function test_automatic_scroll_on_keyboard_shown() {
128	+ waitForRendering(appPreview);
129	+ var appReviews = findChild(appPreview, "appReviews");
130	+ appReviews.visible = true;
131	+
132	+ var leftFlickable = findChild(appPreview, "leftFlickable");
133	+ leftFlickable.contentY = leftFlickable.contentHeight;
134	+ var reviewField = findChild(appReviews, "reviewField");
135	+ appPreview.keyboardSize = 400;
136	+ appReviews.editing(reviewField);
137	+ var newFlickPos = leftFlickable.contentY;
138	+ var keyboardY = shell.height - appPreview.keyboardSize;
139	+ verify(newFlickPos < keyboardY);
140	+ }
141	+
142	}
143	}