Merge lp:~tpeeters/ubuntu-ui-toolkit/ToolbarButton-disable into lp:ubuntu-ui-toolkit

Proposed by Tim Peeters on 2013-07-23
Status: Merged
Approved by: Tim Peeters on 2013-08-22
Approved revision: 661
Merged at revision: 707
Proposed branch: lp:~tpeeters/ubuntu-ui-toolkit/ToolbarButton-disable
Merge into: lp:ubuntu-ui-toolkit
Diff against target: 169 lines (+36/-24)
7 files modified
components.api (+2/-1)
modules/Ubuntu/Components/AbstractButton.qml (+1/-1)
modules/Ubuntu/Components/ActionItem.qml (+11/-3)
modules/Ubuntu/Components/Panel.qml (+5/-5)
modules/Ubuntu/Components/Popups/ActionSelectionPopover.qml (+2/-0)
modules/Ubuntu/Components/TextInputPopover.qml (+14/-14)
tests/resources/toolbar/toolbar.qml (+1/-0)
To merge this branch: bzr merge lp:~tpeeters/ubuntu-ui-toolkit/ToolbarButton-disable
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2013-08-22
Christian Dywan (community) 2013-07-23 Approve on 2013-08-16
Review via email: mp+176395@code.launchpad.net

Commit message

Do not trigger a ToolbarButton that is not enabled.

To post a comment you must log in.
Christian Dywan (kalikiana) wrote :

I'm thinking we should have a "trigger()" method that does this check. The AbstractButton.clicked also doesn't check enabled. And adding this in apps is even more error-prone.

review: Needs Fixing
Tim Peeters (tpeeters) wrote :

the trigger() of the action is defined by the developers, so we cannot force that they check it

Tim Peeters (tpeeters) wrote :

correction: triggered() is defined by the developers.

Unity.Action has a trigger() that does the check, need to change the UITK code to call trigger() instead of triggered().

Christian Dywan (kalikiana) wrote :

Nice stuff, thanks for making the changes. And good to have the value argument retained: this came up only in IRC, so I'm mentioning it for clarity, I was running into issues in another place because the value was "hidden".

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'components.api'
2--- components.api 2013-08-16 10:16:48 +0000
3+++ components.api 2013-08-21 17:07:26 +0000
4@@ -16,7 +16,8 @@
5 property Action action
6 property string text
7 property url iconSource
8- signal triggered(var caller)
9+ signal triggered(var value)
10+ function trigger(value)
11 modules/Ubuntu/Components/ActionList.qml
12 QtObject
13 default property list<Action> children
14
15=== modified file 'modules/Ubuntu/Components/AbstractButton.qml'
16--- modules/Ubuntu/Components/AbstractButton.qml 2013-06-12 18:22:07 +0000
17+++ modules/Ubuntu/Components/AbstractButton.qml 2013-08-21 17:07:26 +0000
18@@ -47,7 +47,7 @@
19 /*!
20 If a button is clicked, its triggered() signal will automatically be called.
21 */
22- onClicked: button.triggered(button)
23+ onClicked: button.trigger()
24
25 Keys.onEnterPressed: clicked()
26 Keys.onReturnPressed: clicked()
27
28=== modified file 'modules/Ubuntu/Components/ActionItem.qml'
29--- modules/Ubuntu/Components/ActionItem.qml 2013-07-01 22:13:31 +0000
30+++ modules/Ubuntu/Components/ActionItem.qml 2013-08-21 17:07:26 +0000
31@@ -59,10 +59,18 @@
32 /*!
33 Called when the actionItem is triggered.
34 */
35- signal triggered(var caller)
36+ signal triggered(var value)
37
38 /*!
39- If \l action is set, this will call action.trigger(caller).
40+ If \l action is set, this will trigger it.
41 */
42- onTriggered: if (action) action.triggered(caller)
43+ onTriggered: if (action) action.trigger(value)
44+
45+ /*!
46+ Trigger this action item if it is enabled.
47+ */
48+ function trigger(value) {
49+ var passingValue = value ? value : null
50+ if (actionItem.enabled) actionItem.triggered(passingValue);
51+ }
52 }
53
54=== modified file 'modules/Ubuntu/Components/Panel.qml'
55--- modules/Ubuntu/Components/Panel.qml 2013-08-15 10:59:06 +0000
56+++ modules/Ubuntu/Components/Panel.qml 2013-08-21 17:07:26 +0000
57@@ -416,17 +416,17 @@
58 // find the first child with a triggered property:
59 function getTriggerableItem(mouse) {
60 var item = bar; // contains the children
61- while (item && !item.hasOwnProperty("triggered")) {
62+ while (item && !item.hasOwnProperty("trigger")) {
63 var coords = mapToItem(item, mouse.x, mouse.y);
64 // FIXME: When using a ListView the highlight may be
65 // returned instead of the Item that you are looking for
66 item = item.childAt(coords.x, coords.y);
67 }
68- return item; // will be null if no item has triggered() signal.
69+ return item; // will be null if no item has trigger() function.
70 }
71
72- // forward clicked and triggered events to any child Item with a
73- // clicked() or triggered() signal, not
74+ // forward clicked() and trigger() events to any child Item with a
75+ // clicked() or trigger() function, not
76 // just MouseAreas since MouseAreas would block swiping of the panel.
77 // This must also happen when the panel is locked, so the DraggingArea is
78 // never disabled, and other signal handlers will return when panel.locked is true.
79@@ -437,7 +437,7 @@
80 pressedItem.clicked();
81 } else if (pressedItem && pressedItem === getTriggerableItem(mouse)) {
82 // Click event positioned at the Item where the user first pressed
83- pressedItem.triggered(pressedItem);
84+ pressedItem.trigger();
85 }
86 }
87
88
89=== modified file 'modules/Ubuntu/Components/Popups/ActionSelectionPopover.qml'
90--- modules/Ubuntu/Components/Popups/ActionSelectionPopover.qml 2013-07-04 21:36:23 +0000
91+++ modules/Ubuntu/Components/Popups/ActionSelectionPopover.qml 2013-08-21 17:07:26 +0000
92@@ -120,6 +120,8 @@
93 }
94 /*! \internal */
95 onTriggered: popover.hide()
96+ visible: enabled
97+ height: visible ? implicitHeight : 0
98 }
99
100 grabDismissAreaEvents: false
101
102=== modified file 'modules/Ubuntu/Components/TextInputPopover.qml'
103--- modules/Ubuntu/Components/TextInputPopover.qml 2013-02-13 09:36:45 +0000
104+++ modules/Ubuntu/Components/TextInputPopover.qml 2013-08-21 17:07:26 +0000
105@@ -22,38 +22,38 @@
106 actions: ActionList {
107 Action {
108 text: i18n.tr("Select All")
109- visible: target && target.text !== "" && target.text !== target.selectedText
110- onTriggered: caller.selectAll()
111+ enabled: target && target.text !== "" && target.text !== target.selectedText
112+ onTriggered: target.selectAll()
113 }
114 Action {
115 text: i18n.tr("Select Word")
116- visible: target && target.text !== "" && target.selectedText === ""
117- onTriggered: caller.selectWord()
118+ enabled: target && target.text !== "" && target.selectedText === ""
119+ onTriggered: target.selectWord()
120 }
121 Action {
122 text: i18n.tr("Cut")
123- visible: target && target.selectedText !== ""
124- onTriggered: caller.cut()
125+ enabled: target && target.selectedText !== ""
126+ onTriggered: target.cut()
127 }
128 Action {
129 text: i18n.tr("Copy")
130- visible: target && target.selectedText !== ""
131- onTriggered: caller.copy()
132+ enabled: target && target.selectedText !== ""
133+ onTriggered: target.copy()
134 }
135 Action {
136 text: i18n.tr("Paste")
137- visible: target && target.canPaste
138- onTriggered: caller.paste()
139+ enabled: target && target.canPaste
140+ onTriggered: target.paste()
141 }
142 Action {
143 text: i18n.tr("Undo")
144- visible: target && target.canUndo
145- onTriggered: caller.undo()
146+ enabled: target && target.canUndo
147+ onTriggered: target.undo()
148 }
149 Action {
150 text: i18n.tr("Redo")
151- visible: target && target.canRedo
152- onTriggered: caller.redo()
153+ enabled: target && target.canRedo
154+ onTriggered: target.redo()
155 }
156 }
157 }
158
159=== modified file 'tests/resources/toolbar/toolbar.qml'
160--- tests/resources/toolbar/toolbar.qml 2013-06-14 16:28:04 +0000
161+++ tests/resources/toolbar/toolbar.qml 2013-08-21 17:07:26 +0000
162@@ -47,6 +47,7 @@
163 iconSource: Qt.resolvedUrl("../../../examples/ubuntu-ui-toolkit-gallery/small_avatar.png")
164 text: "oh"
165 onTriggered: print("lala")
166+ enabled: false
167 }
168 ToolbarButton {
169 action: action1

Subscribers

People subscribed via source and target branches

to status/vote changes: