Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick into lp:ubuntu-ui-toolkit/staging

Proposed by Christian Dywan on 2015-06-17
Status: Merged
Approved by: Zsombor Egri on 2015-07-20
Approved revision: 1549
Merged at revision: 1570
Proposed branch: lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 337 lines (+173/-8)
7 files modified
modules/Ubuntu/Components/1.3/ListItemPopover.qml (+70/-0)
modules/Ubuntu/Components/Popups/1.3/Popover.qml (+2/-2)
modules/Ubuntu/Components/plugin/uclistitem.cpp (+59/-1)
modules/Ubuntu/Components/plugin/uclistitem.h (+7/-0)
modules/Ubuntu/Components/plugin/uctheme.cpp (+4/-0)
modules/Ubuntu/Components/plugin/uctheme.h (+2/-1)
tests/unit_x11/tst_components/tst_listitem.qml (+29/-4)
To merge this branch: bzr merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/listItemRightClick
Reviewer Review Type Date Requested Status
Zsombor Egri (community) Approve on 2015-07-20
PS Jenkins bot continuous-integration Approve on 2015-07-20
Tim Peeters 2015-06-17 Needs Fixing on 2015-07-01
Review via email: mp+262177@code.launchpad.net

Commit Message

Implement ListItemPopover on right-click

To post a comment you must log in.
Andrea Bernabei (faenil) wrote :

I left a few comments, just out of curiosity

Andrea Bernabei (faenil) :
Tim Peeters (tpeeters) wrote :

When I open our gallery.sh and go to the new list items and right-click them, I am getting a bunch of warnings:

file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/examples/ubuntu-ui-toolkit-gallery/ListItems.qml:244:13: QML ListView: Binding loop detected for property "height"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"

review: Needs Fixing
Tim Peeters (tpeeters) wrote :

I get the same^ warnings with tst_listitem.qml:

tim@ubuntu:~/dev/ubuntu-ui-toolkit/r/listItemRightClick/tests/unit_x11/tst_components$ ../../launcher/launcher tst_listitem.qml
file:///usr/lib/x86_64-linux-gnu/qt5/qml/QtTest/TestCase.qml:337:32: Unable to assign [undefined] to bool
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:20:1: QML ListItemPopover: Binding loop detected for property "actions"
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null
file:///home/tim/dev/ubuntu-ui-toolkit/r/listItemRightClick/modules/Ubuntu/Components/1.3/ListItemPopover.qml:38: TypeError: Cannot read property 'leadingActions' of null

Tim Peeters (tpeeters) wrote :

I agree with Andrea that searching or the path of the qml file is not very pretty.

Also, even when using Ubuntu.Components 1.2, the ListItem will show the popover for 1.3. We don't want API or visual changes in 1.2 any more, but with this approach you will get the changes in ListItem popover that are made for 1.3 even when importing 1.2.

Perhaps we should show the popover only for 1.3?

review: Needs Information
Tim Peeters (tpeeters) wrote :

A screenshot of how it looks now: https://www.dropbox.com/s/v4cxv2vaqogqvjo/Screenshot%202015-06-23%2016.04.03.png?dl=0

Design input is needed.

Christian Dywan (kalikiana) wrote :

FTR I'm going to update the design.

The searching for the QML path can't be avoided as far as I'm aware because there's no path in the component, but I will look if I can find the version that is being used.

Tim Peeters (tpeeters) wrote :

> FTR I'm going to update the design.
>
> The searching for the QML path can't be avoided as far as I'm aware because
> there's no path in the component, but I will look if I can find the version
> that is being used.

If the version is not >= 1.3, the right-click should not even open the popover.

1542. By Christian Dywan on 2015-06-24

Update components.api

Tim Peeters (tpeeters) wrote :

57 + MouseArea {
58 + anchors.fill: parent
59 + onReleased: {
60 + popover.hide();
61 + action.trigger();
62 + }
63 + }

This seems weird/hackish to me. Why do you need this?

review: Needs Information
Tim Peeters (tpeeters) wrote :

Test Result (3 failures / -1)
components.ListItemAPI::test_1_warn_missing_dragUpdated_signal_handler
components.ListItemAPI::test_dragmode_availability
components.ListItemAPI::test_warn_model

^the failures from jenkins seem related to the changes you made. Can you check them?

review: Needs Fixing
Tim Peeters (tpeeters) wrote :

I'm getting these failures locally too, see http://pastebin.ubuntu.com/11792405/

Tim Peeters (tpeeters) wrote :

Should we have autopilot CPOs specifically for using the popover to trigger an action?

Tim Peeters (tpeeters) wrote :

The list-item that was right-clicked to show the popover must be highlighted while the popover is open. I don't know if that was in the specifications, but we just agreed on it in the design sync meeting.

review: Needs Fixing
1543. By Christian Dywan on 2015-07-01

Use onClicked instead of custom MouseArea in ListItem

1544. By Christian Dywan on 2015-07-01

Highlight ListItem while context menu is open

Christian Dywan (kalikiana) wrote :

> Should we have autopilot CPOs specifically for using the popover to trigger an
> action?

All icons are available via swiping. Currently I don't see any need since it would do exactly the same.

Zsombor Egri (zsombi) wrote :

What is the reason you registered a 1.3 specific ListItem? You wanted to guard the availability of the popup menu in 1.2? You can do that by not having the Popup present in 1.2 directory, and check where the ListItem is used based on the theme.toolkitVersion used.

You can look after the QML file you need based on the plugin's baseUrl(). See UbuntuComponentsPlugin::pluginUrl().

review: Needs Fixing
Zsombor Egri (zsombi) wrote :

Another idea on resolving the context menu: Introduce the menu property in the ListItem (which will be done anyway), yet undocumented. Then set that property in the 1.3 Ambiance style with a Popover from the theme. In the code you look after the menu property, if it is set, you can open it. So no need for 1.3 specific subclassing anymore.

1545. By Christian Dywan on 2015-07-19

Use UbuntuComponentsPlugin::pluginUrl to locale ListItemPopover

1546. By Christian Dywan on 2015-07-19

Update line numbers in ignoreWarning calls in listitem test

1547. By Christian Dywan on 2015-07-19

Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/staging

1548. By Christian Dywan on 2015-07-19

Check popover.parent before adjusting y in updatePosition

Zsombor Egri (zsombi) wrote :

Few comments inline.

review: Needs Fixing
1549. By Christian Dywan on 2015-07-20

UCTheme.version method and delete QQmlComponent

Zsombor Egri (zsombi) wrote :

Good to go.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'modules/Ubuntu/Components/1.3/ListItemPopover.qml'
2--- modules/Ubuntu/Components/1.3/ListItemPopover.qml 1970-01-01 00:00:00 +0000
3+++ modules/Ubuntu/Components/1.3/ListItemPopover.qml 2015-07-20 12:57:55 +0000
4@@ -0,0 +1,70 @@
5+/*
6+ * Copyright 2015 Canonical Ltd.
7+ *
8+ * This program is free software; you can redistribute it and/or modify
9+ * it under the terms of the GNU Lesser General Public License as published by
10+ * the Free Software Foundation; version 3.
11+ *
12+ * This program is distributed in the hope that it will be useful,
13+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+ * GNU Lesser General Public License for more details.
16+ *
17+ * You should have received a copy of the GNU Lesser General Public License
18+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19+ */
20+
21+import QtQuick 2.4
22+import Ubuntu.Components 1.3
23+import Ubuntu.Components.Popups 1.3
24+
25+ActionSelectionPopover {
26+ id: popover
27+ objectName: 'listItemContextMenu'
28+ contentWidth: units.gu(25)
29+
30+ delegate: ListItem {
31+ contentItem.anchors {
32+ leftMargin: units.gu(2)
33+ rightMargin: units.gu(2)
34+ topMargin: units.gu(0.5)
35+ bottomMargin: units.gu(0.5)
36+ }
37+ divider.visible: action != actions[actions.length - 1]
38+
39+ Label {
40+ anchors.verticalCenter: parent.verticalCenter
41+ text: action.text
42+ color: '#5D5D5D'
43+ }
44+
45+ onClicked: popover.hide()
46+ }
47+
48+ function mergeActions(to, from) {
49+ if (from == null)
50+ return;
51+
52+ var actions = from.actions;
53+ for (var i in actions) {
54+ var action = actions[i];
55+ if (!action.text)
56+ action.text = action.iconName;
57+ to.push(actions[i]);
58+ }
59+ }
60+
61+ onCallerChanged: {
62+ var all = [];
63+ mergeActions(all, caller.leadingActions);
64+ mergeActions(all, caller.trailingActions);
65+ actions = all;
66+ }
67+
68+ // Hide the arrow
69+ Binding {
70+ target: __foreground
71+ property: "direction"
72+ value: "none"
73+ }
74+}
75
76=== modified file 'modules/Ubuntu/Components/Popups/1.3/Popover.qml'
77--- modules/Ubuntu/Components/Popups/1.3/Popover.qml 2015-07-02 17:39:03 +0000
78+++ modules/Ubuntu/Components/Popups/1.3/Popover.qml 2015-07-20 12:57:55 +0000
79@@ -191,7 +191,7 @@
80
81 // private
82 function updatePosition() {
83- if (pointerTarget && pointerTarget.parent)
84+ if (pointerTarget && pointerTarget.parent && popover.parent)
85 popover.y = (popover.parent.height - popover.height) / 2;
86 var pos = new InternalPopupUtils.CallerPositioning(foreground, pointer, dismissArea, caller, pointerTarget, edgeMargins, callerMargin);
87 pos.auto();
88@@ -210,7 +210,7 @@
89 objectName: "popover_foreground"
90
91 //styling properties
92- property real minimumWidth: units.gu(40)
93+ property real minimumWidth: units.gu(25)
94
95 property real maxWidth: dismissArea ? (internal.portrait ? dismissArea.width : dismissArea.width * 3/4) : 0.0
96 property real maxHeight: dismissArea ? (internal.portrait ? dismissArea.height * 3/4 : dismissArea.height) : 0.0
97
98=== modified file 'modules/Ubuntu/Components/plugin/uclistitem.cpp'
99--- modules/Ubuntu/Components/plugin/uclistitem.cpp 2015-07-13 15:01:01 +0000
100+++ modules/Ubuntu/Components/plugin/uclistitem.cpp 2015-07-20 12:57:55 +0000
101@@ -16,6 +16,7 @@
102
103 #include "ucunits.h"
104 #include "uctheme.h"
105+#include "ucnamespace.h"
106 #include "uclistitem.h"
107 #include "uclistitem_p.h"
108 #include "uclistitemactions.h"
109@@ -37,6 +38,9 @@
110 #include "uclistitemstyle.h"
111 #include <QtQuick/private/qquickbehavior_p.h>
112 #include <QtQml/QQmlEngine>
113+#include <QFileInfo>
114+#include <QLibraryInfo>
115+#include "plugin.h"
116
117 /******************************************************************************
118 * Divider
119@@ -1075,6 +1079,54 @@
120 event->setAccepted(true);
121 }
122
123+bool UCListItem13::shouldShowContextMenu(QMouseEvent *event)
124+{
125+ if (event->button() != Qt::RightButton)
126+ return false;
127+ return leadingActions() || trailingActions();
128+}
129+
130+void UCListItem13::mousePressEvent(QMouseEvent *event)
131+{
132+ UCListItem::mousePressEvent(event);
133+ if (shouldShowContextMenu(event)) {
134+ // Highlight the Item while the menu is showing
135+ setHighlighted(true);
136+
137+ Q_D(UCListItem);
138+ quint16 version(d->getTheme()->version());
139+ QString versionString(QStringLiteral("%1.%2").arg(MAJOR_VERSION(version)).arg(MINOR_VERSION(version)));
140+ QUrl url(UbuntuComponentsPlugin::pluginUrl().resolved(versionString + "/ListItemPopover.qml"));
141+
142+ // Open Popover
143+ QQmlEngine* engine = qmlEngine(this);
144+ QQmlComponent* component = new QQmlComponent(engine, url, QQmlComponent::PreferSynchronous, this);
145+ if (component->isError()) {
146+ qmlInfo(this) << component->errorString();
147+ } else {
148+ QQmlEngine::setContextForObject(component, qmlContext(this));
149+ QQuickItem* item = static_cast<QQuickItem*>(component->create(qmlContext(this)));
150+ item->setProperty("caller", QVariant::fromValue(this));
151+ item->setParentItem(QuickUtils::instance().rootItem(this));
152+ QMetaObject::invokeMethod(item, "show");
153+ connect(item, &QQuickItem::visibleChanged, this,
154+ &UCListItem13::popoverClosed, Qt::DirectConnection);
155+ }
156+ delete component;
157+ }
158+}
159+
160+void UCListItem13::popoverClosed()
161+{
162+ setHighlighted(false);
163+}
164+
165+void UCListItem::setHighlighted(bool highlighted)
166+{
167+ Q_D(UCListItem);
168+ d->setHighlighted(highlighted);
169+}
170+
171 void UCListItem::mouseReleaseEvent(QMouseEvent *event)
172 {
173 UCStyledItemBase::mouseReleaseEvent(event);
174@@ -1101,8 +1153,14 @@
175 d->swipeEvent(event->localPos(), UCSwipeEvent::Finished);
176 d->suppressClick = false;
177 }
178+ d->setHighlighted(false);
179 }
180- d->setHighlighted(false);
181+}
182+
183+void UCListItem13::mouseReleaseEvent(QMouseEvent *event)
184+{
185+ if (!shouldShowContextMenu(event))
186+ UCListItem::mouseReleaseEvent(event);
187 }
188
189 void UCListItem::mouseMoveEvent(QMouseEvent *event)
190
191=== modified file 'modules/Ubuntu/Components/plugin/uclistitem.h'
192--- modules/Ubuntu/Components/plugin/uclistitem.h 2015-07-13 08:06:24 +0000
193+++ modules/Ubuntu/Components/plugin/uclistitem.h 2015-07-20 12:57:55 +0000
194@@ -64,6 +64,7 @@
195 protected:
196 void classBegin();
197 void componentComplete();
198+ void setHighlighted(bool);
199 QSGNode *updatePaintNode(QSGNode *oldNode, UpdatePaintNodeData *data);
200 void itemChange(ItemChange change, const ItemChangeData &data);
201 void mousePressEvent(QMouseEvent *event);
202@@ -112,6 +113,12 @@
203 class UCListItem13 : public UCListItem
204 {
205 Q_OBJECT
206+protected:
207+ void mousePressEvent(QMouseEvent *event);
208+ void mouseReleaseEvent(QMouseEvent *event);
209+private:
210+ bool shouldShowContextMenu(QMouseEvent *event);
211+ void popoverClosed();
212 public:
213 explicit UCListItem13(QQuickItem *parent = 0);
214 };
215
216=== modified file 'modules/Ubuntu/Components/plugin/uctheme.cpp'
217--- modules/Ubuntu/Components/plugin/uctheme.cpp 2015-07-13 14:26:01 +0000
218+++ modules/Ubuntu/Components/plugin/uctheme.cpp 2015-07-20 12:57:55 +0000
219@@ -638,6 +638,10 @@
220 *
221 * \sa Ubuntu::toolkitVersion, Ubuntu::version, {Themes}
222 */
223+quint16 UCTheme::version()
224+{
225+ return m_version;
226+}
227 void UCTheme::setVersion(quint16 version)
228 {
229 if (m_version == version) {
230
231=== modified file 'modules/Ubuntu/Components/plugin/uctheme.h'
232--- modules/Ubuntu/Components/plugin/uctheme.h 2015-05-26 08:18:02 +0000
233+++ modules/Ubuntu/Components/plugin/uctheme.h 2015-07-20 12:57:55 +0000
234@@ -39,7 +39,7 @@
235 Q_PROPERTY(UCTheme *parentTheme READ parentTheme NOTIFY parentThemeChanged FINAL)
236 Q_PROPERTY(QString name READ name WRITE setName RESET resetName NOTIFY nameChanged FINAL)
237 Q_PROPERTY(QObject* palette READ palette WRITE setPalette RESET resetPalette NOTIFY paletteChanged FINAL)
238- Q_PROPERTY(quint16 version MEMBER m_version WRITE setVersion NOTIFY versionChanged FINAL)
239+ Q_PROPERTY(quint16 version READ version WRITE setVersion NOTIFY versionChanged FINAL)
240 public:
241 struct ThemeRecord {
242 ThemeRecord() :
243@@ -73,6 +73,7 @@
244 void resetName();
245 QObject* palette();
246 void setPalette(QObject *config);
247+ quint16 version();
248 void setVersion(quint16 version);
249
250 // internal, used by the deprecated Theme.createStyledComponent()
251
252=== modified file 'tests/unit_x11/tst_components/tst_listitem.qml'
253--- tests/unit_x11/tst_components/tst_listitem.qml 2015-04-13 11:11:59 +0000
254+++ tests/unit_x11/tst_components/tst_listitem.qml 2015-07-20 12:57:55 +0000
255@@ -28,22 +28,27 @@
256
257 Action {
258 id: stockAction
259- iconName: "starred"
260+ iconName: "torch-on"
261 objectName: "stockAction"
262+ text: 'Switch lights on'
263 }
264 ListItemActions {
265 id: leading
266 actions: [
267 Action {
268 iconName: "starred"
269+ text: 'Bookmark'
270 objectName: "leading_1"
271 },
272 Action {
273 iconName: "edit"
274+ text: 'Edit'
275 objectName: "leading_2"
276+ onTriggered: text = 'Edit Again'
277 },
278 Action {
279 iconName: "camcorder"
280+ text: 'Record'
281 objectName: "leading_3"
282 }
283 ]
284@@ -376,6 +381,26 @@
285 clickSpy.wait();
286 }
287
288+ SignalSpy {
289+ id: visibleSpy
290+ signalName: "visibleChanged"
291+ }
292+
293+ function test_context_menu() {
294+ mouseClick(testItem, testItem.width / 2, testItem.height / 2, Qt.RightButton);
295+ wait(1000);
296+ compare(testItem.highlighted, true, "List item didn't highlight on right-click");
297+ var context_menu = findChild(main, "listItemContextMenu");
298+ verify(context_menu, "Context menu didn't open on right-click");
299+ waitForRendering(context_menu);
300+ var edit = findChildWithProperty(context_menu, "text", "Edit");
301+ verify(edit, "Context menu has no 'Edit' item");
302+ visibleSpy.target = context_menu;
303+ mouseClick(edit, edit.width / 2, edit.height / 2);
304+ compare(edit.text, 'Edit Again', "Item wasn't triggered'");
305+ visibleSpy.wait()
306+ }
307+
308 function test_no_click_when_swiped() {
309 var item = findChild(listView, "listItem0");
310 clickSpy.target = item;
311@@ -976,7 +1001,7 @@
312 }
313 function test_dragmode_availability(data) {
314 if (data.xfail) {
315- ignoreWarning(warningFormat(80, 5, "QML Column: Dragging mode requires ListView"));
316+ ignoreWarning(warningFormat(85, 5, "QML Column: Dragging mode requires ListView"));
317 }
318 data.item.ViewItems.dragMode = true;
319 wait(400);
320@@ -1147,7 +1172,7 @@
321
322 // must run this immediately after the defaults are checked otherwise drag handler connected check will fail
323 function test_1_warn_missing_dragUpdated_signal_handler() {
324- ignoreWarning(warningFormat(116, 9, "QML ListView: ListView has no ViewItems.dragUpdated() signal handler implemented. No dragging will be possible."));
325+ ignoreWarning(warningFormat(121, 9, "QML ListView: ListView has no ViewItems.dragUpdated() signal handler implemented. No dragging will be possible."));
326 toggleDragMode(listView, true);
327 drag(listView, 0, 1);
328 toggleDragMode(listView, true);
329@@ -1185,7 +1210,7 @@
330 function test_warn_model(data) {
331 function dummyFunc() {}
332 if (data.warning !== "") {
333- ignoreWarning(warningFormat(116, 9, "QML ListView: " + data.warning));
334+ ignoreWarning(warningFormat(121, 9, "QML ListView: " + data.warning));
335 }
336 listView.model = data.model;
337 if (typeof data.modelModel !== "undefined") {

Subscribers

People subscribed via source and target branches