Ubuntu UI Toolkit

Merge lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex into lp:ubuntu-ui-toolkit/staging

  1. pageHead-sectionsIndex
  2. Merge into staging
Proposed by Tim Peeters on 2015-12-07
Status: Merged
Approved by: Christian Dywan on 2015-12-10
Approved revision: 1753
Merged at revision: 1751
Proposed branch: lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 287 lines (+163/-10)
7 files modified
src/Ubuntu/Components/1.3/Sections.qml (+38/-5)
src/Ubuntu/Components/Themes/Ambiance/1.3/PageHeadStyle.qml (+1/-1)
src/Ubuntu/Components/Themes/Ambiance/1.3/SectionsForPageHead.qml (+46/-0)
src/Ubuntu/Components/Themes/Ambiance/Ambiance.pro (+1/-0)
src/Ubuntu/Components/Themes/Ambiance/qmldir (+1/-0)
tests/unit_x11/tst_components/tst_pagehead_sections.qml (+8/-2)
tests/unit_x11/tst_components/tst_sections.qml (+68/-2)
To merge this branch: bzr merge lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex
Reviewer Review Type Date Requested Status
Christian Dywan 2015-12-07 Approve on 2015-12-10
PS Jenkins bot continuous-integration Approve on 2015-12-09
Review via email: mp+279834@code.launchpad.net

Commit Message

To prevent an invalid sectionIndex, reset the value of sectionIndex to -1 when the model of Sections is changed.

To post a comment you must log in.
Tim Peeters (tpeeters) wrote :

Here https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1511839 it is requested NOT to change the sectionIndex when it was -1. This MR would break that, so we need a more complex solution.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex updated on 2015-12-08
1746. By Tim Peeters on 2015-12-08

documentation

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex updated on 2015-12-08
1747. By Tim Peeters on 2015-12-08

update unit test

1748. By Tim Peeters on 2015-12-08

clean

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex updated on 2015-12-08
1749. By Tim Peeters on 2015-12-08

select index 0 by default, and trigger selected action.

1750. By Tim Peeters on 2015-12-08

deal with invalid model

1751. By Tim Peeters on 2015-12-08

don't regress bug 1511839

Tim Peeters (tpeeters) wrote :

Automatically setting the index when the model changes regresses bug 1511839. To work around that, I use the older copy of Sections inside the AppHeader that is configured using Page.head.sections. That means to get the automatic updates to selectedIndex when changing the model, the app must update to use the new PageHeader instead of the old Page.head.sections. See also comment #4 on bug 1513933.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Christian Dywan (kalikiana) wrote :

> Automatically setting the index when the model changes regresses bug 1511839.
> To work around that, I use the older copy of Sections inside the AppHeader
> that is configured using Page.head.sections. That means to get the automatic
> updates to selectedIndex when changing the model, the app must update to use
> the new PageHeader instead of the old Page.head.sections. See also comment #4
> on bug 1513933.

Will setting -1 be explicitly supported with PageHeader then? I feel having different behavior between these might be both hard to support and use..

review: Needs Information
lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex updated on 2015-12-09
1752. By Tim Peeters on 2015-12-09

kick jenkins

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Tim Peeters (tpeeters) wrote :

To summarize: The desired behavior is implemented in Sections (and can be used in the new PageTreeNode), and if you use Page.head.sections, you still get the old behavior because changing that would break the fix for this bug: https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1511839

I discussed this with Zsombor and Christian and we agree that this is the best solution. Apps will have to update to use the new PageHeader if they require the new automatic behavior.

lp:~tpeeters/ubuntu-ui-toolkit/pageHead-sectionsIndex updated on 2015-12-09
1753. By Tim Peeters on 2015-12-09

kick jenkins again

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Christian Dywan (kalikiana) wrote :

Looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Ubuntu/Components/1.3/Sections.qml'
2--- src/Ubuntu/Components/1.3/Sections.qml 2015-09-28 14:36:54 +0000
3+++ src/Ubuntu/Components/1.3/Sections.qml 2015-12-09 15:25:30 +0000
4@@ -84,18 +84,51 @@
5 // FIXME: Make the Sections scrollable for more than 3 sections.
6 console.warn("It is not YET recommended or supported to use more than three sections.");
7 }
8+ if (internal.done) {
9+ if (!model || model.length === 0) {
10+ selectedIndex = -1;
11+ } else if (selectedIndex === 0) {
12+ // selectedIndex does not change, but action 0 should be triggered.
13+ internal.triggerAction(0);
14+ } else {
15+ // change selectedIndex, which will trigger action 0.
16+ selectedIndex = 0;
17+ }
18+ }
19+ }
20+
21+ Component.onCompleted: {
22+ internal.done = true;
23+ internal.triggerAction(selectedIndex);
24+ }
25+ QtObject {
26+ id: internal
27+ property bool done: false
28+
29+ /*!
30+ Triggers the action associated with the given
31+ index, if that action exists.
32+ */
33+ function triggerAction(index) {
34+ if ((index >= 0) && (index < model.length)) {
35+ if (model[index].hasOwnProperty("trigger")) {
36+ model[index].trigger();
37+ }
38+ }
39+ }
40 }
41
42 /*!
43 The index of the currently selected section in \l model.
44+ The default value is 0 if there is at least 1 section, or -1 for no sections.
45+ When the model is changed, selectedIndex is reset to 0 and the first action
46+ is triggered.
47+ Upon completion of the Sections component, if there is an Action associated
48+ with the selected index, that Action will be triggered.
49 */
50 property int selectedIndex: model ? 0 : -1
51
52 onSelectedIndexChanged: {
53- if ((selectedIndex >= 0) && (selectedIndex < model.length)) {
54- if (model[selectedIndex].hasOwnProperty("trigger")) {
55- model[selectedIndex].trigger();
56- }
57- }
58+ internal.triggerAction(selectedIndex);
59 }
60 }
61
62=== modified file 'src/Ubuntu/Components/Themes/Ambiance/1.3/PageHeadStyle.qml'
63--- src/Ubuntu/Components/Themes/Ambiance/1.3/PageHeadStyle.qml 2015-12-05 05:59:07 +0000
64+++ src/Ubuntu/Components/Themes/Ambiance/1.3/PageHeadStyle.qml 2015-12-09 15:25:30 +0000
65@@ -102,7 +102,7 @@
66 color: styledItem.dividerColor
67 }
68
69- Sections {
70+ SectionsForPageHead {
71 id: sectionsItem
72 objectName: "headerSectionsItem"
73 anchors {
74
75=== added file 'src/Ubuntu/Components/Themes/Ambiance/1.3/SectionsForPageHead.qml'
76--- src/Ubuntu/Components/Themes/Ambiance/1.3/SectionsForPageHead.qml 1970-01-01 00:00:00 +0000
77+++ src/Ubuntu/Components/Themes/Ambiance/1.3/SectionsForPageHead.qml 2015-12-09 15:25:30 +0000
78@@ -0,0 +1,46 @@
79+/*
80+ * Copyright 2015 Canonical Ltd.
81+ *
82+ * This program is free software; you can redistribute it and/or modify
83+ * it under the terms of the GNU Lesser General Public License as published by
84+ * the Free Software Foundation; version 3.
85+ *
86+ * This program is distributed in the hope that it will be useful,
87+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
88+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
89+ * GNU Lesser General Public License for more details.
90+ *
91+ * You should have received a copy of the GNU Lesser General Public License
92+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
93+ */
94+
95+import QtQuick 2.4
96+import Ubuntu.Components 1.3
97+
98+/*!
99+ \internal
100+ \deprecated
101+ Older version of Sections to work around bug 1511839
102+ (only for controlling sections via PageHeadSections).
103+ */
104+StyledItem {
105+ id: sections
106+ styleName: "SectionsStyle"
107+
108+ property list<Action> actions
109+ property var model: actions
110+ onModelChanged: {
111+ if (model && model.length > 3) {
112+ // FIXME: Make the Sections scrollable for more than 3 sections.
113+ console.warn("It is not YET recommended or supported to use more than three sections.");
114+ }
115+ }
116+ property int selectedIndex: model ? 0 : -1
117+ onSelectedIndexChanged: {
118+ if ((selectedIndex >= 0) && (selectedIndex < model.length)) {
119+ if (model[selectedIndex].hasOwnProperty("trigger")) {
120+ model[selectedIndex].trigger();
121+ }
122+ }
123+ }
124+}
125
126=== modified file 'src/Ubuntu/Components/Themes/Ambiance/Ambiance.pro'
127--- src/Ubuntu/Components/Themes/Ambiance/Ambiance.pro 2015-11-12 15:13:57 +0000
128+++ src/Ubuntu/Components/Themes/Ambiance/Ambiance.pro 2015-12-09 15:25:30 +0000
129@@ -115,6 +115,7 @@
130 1.3/PageHeaderStyle.qml \
131 1.3/BottomEdgeHintStyle.qml \
132 1.3/BottomEdgeStyle.qml \
133+ 1.3/SectionsForPageHead.qml \
134 $$ARTWORK_FILES
135
136 load(ubuntu_qml_module)
137
138=== modified file 'src/Ubuntu/Components/Themes/Ambiance/qmldir'
139--- src/Ubuntu/Components/Themes/Ambiance/qmldir 2015-11-23 15:14:16 +0000
140+++ src/Ubuntu/Components/Themes/Ambiance/qmldir 2015-12-09 15:25:30 +0000
141@@ -86,3 +86,4 @@
142 PageHeaderStyle 1.3 ./1.3/PageHeaderStyle.qml
143 BottomEdgeHintStyle 1.3 ./1.3/BottomEdgeHintStyle.qml
144 BottomEdgeStyle 1.3 ./1.3/BottomEdgeStyle.qml
145+internal SectionsForPageHead ./1.3/SectionsForPageHead.qml
146
147=== modified file 'tests/unit_x11/tst_components/tst_pagehead_sections.qml'
148--- tests/unit_x11/tst_components/tst_pagehead_sections.qml 2015-11-02 15:20:42 +0000
149+++ tests/unit_x11/tst_components/tst_pagehead_sections.qml 2015-12-09 15:25:30 +0000
150@@ -71,7 +71,10 @@
151 margins: units.gu(1)
152 }
153 width: height
154- color: page.head.sections.actions[page.head.sections.selectedIndex].text
155+ property int index: page.head.sections.selectedIndex
156+ backgroundColor: index >= 0
157+ ? page.head.sections.actions[index].text
158+ : "black"
159 }
160
161 Label {
162@@ -99,6 +102,9 @@
163 compare(page.active, true, "Single page is active in MainView.");
164 testCase.sectionsItem = findChild(mainView, "headerSectionsItem");
165 }
166+ function init() {
167+ page.head.sections.selectedIndex = 0;
168+ }
169
170 function test_number_of_sections() {
171 compare(sectionsItem.model.length, 3, "Number of sections initialization failed.");
172@@ -111,7 +117,7 @@
173 }
174
175 function check_selected_button(selectedButtonIndex) {
176- compare(selectedButtonIndex, sectionsItem.selectedIndex,
177+ compare(sectionsItem.selectedIndex, selectedButtonIndex,
178 "Incorrect button selected.");
179 var button = findChild(sectionsItem, "section_button_"+selectedButtonIndex);
180 verify(button);
181
182=== modified file 'tests/unit_x11/tst_components/tst_sections.qml'
183--- tests/unit_x11/tst_components/tst_sections.qml 2015-09-21 14:44:13 +0000
184+++ tests/unit_x11/tst_components/tst_sections.qml 2015-12-09 15:25:30 +0000
185@@ -127,6 +127,23 @@
186 model: root.stringList
187 enabled: false
188 }
189+ Sections {
190+ id: selectedIndexSections
191+ property bool action0Triggered: false;
192+ property bool action1Triggered: false;
193+ property bool action2Triggered: false;
194+ property Action action0: Action {
195+ onTriggered: selectedIndexSections.action0Triggered = true;
196+ }
197+ property Action action1: Action {
198+ onTriggered: selectedIndexSections.action1Triggered = true;
199+ }
200+ property Action action2: Action {
201+ onTriggered: selectedIndexSections.action2Triggered = true;
202+ }
203+ actions: [action0, action1, action2]
204+ selectedIndex: 1
205+ }
206 }
207
208 UbuntuTestCase {
209@@ -135,14 +152,23 @@
210 when: windowShown
211
212 function initTestCase() {
213- compare(label.text, "No action triggered.", "An action was triggered initially.");
214+ // The initially selected actions must be triggered.
215+ compare(label.text, "First action triggered.",
216+ "The first action was not triggered automatically.");
217+ compare(selectedIndexSections.action0Triggered, false,
218+ "Action 0 was triggered with selectedIndex: 1.");
219+ compare(selectedIndexSections.action1Triggered, true,
220+ "Action 1 was not automatically triggered with selectedIndex: 1.");
221 }
222
223- function cleanup() {
224+ function init() {
225+ enabledSections.actions = root.actionList;
226+ enabledSections.model = root.actionList;
227 enabledSections.selectedIndex = 0;
228 disabledSections.selectedIndex = 0;
229 enabledStringSections.selectedIndex = 0;
230 disabledStringSections.selectedIndex = 0;
231+ label.text = "No action triggered."
232 }
233
234 function wait_for_animation(sections) {
235@@ -201,6 +227,7 @@
236 compare(v, index, "selectedIndex "+v+" does not match "+index);
237 v = get_selected_section_button_index(sections);
238 compare(v, index, "selected button index "+v+" does not match "+index);
239+ if (v === -1) return;
240 var w = get_selected_section_button_text(sections);
241 compare(w, name, "selected button text \'"+w+"\' does not match \'"+name+"\'");
242 }
243@@ -305,5 +332,44 @@
244 disabledStringSections.selectedIndex = index;
245 check_selected_section(disabledStringSections, index, name);
246 }
247+
248+ function test_selectedIndex_when_model_changes_bug1513933() {
249+ enabledSections.model = ["1", "2", "3"];
250+ enabledSections.selectedIndex = 2;
251+ enabledSections.model = ["1", "2"];
252+ wait_for_animation(enabledSections);
253+ compare(enabledSections.selectedIndex, 0,
254+ "Changing the model does not set the selected index to 0.");
255+ check_selected_section(enabledSections, 0, "1");
256+ enabledSections.model = [];
257+ wait_for_animation(enabledSections);
258+ compare(enabledSections.selectedIndex, -1,
259+ "Setting an empty model does not set the selected index to -1.");
260+ enabledSections.model = ["1", "2", "3"];
261+ wait_for_animation(enabledSections);
262+ compare(enabledSections.selectedIndex, 0,
263+ "Setting a non-empty model does not set the selected index to 0.");
264+ check_selected_section(enabledSections, 0, "1");
265+ }
266+
267+ function test_model_changes_triggers_action_0() {
268+ selectedIndexSections.action0Triggered = false;
269+ selectedIndexSections.action1Triggered = false;
270+ selectedIndexSections.action2Triggered = false;
271+ var originalActions = selectedIndexSections.actions;
272+ selectedIndexSections.actions = [
273+ selectedIndexSections.action0,
274+ selectedIndexSections.action2
275+ ]
276+ wait_for_animation(selectedIndexSections);
277+ compare(selectedIndexSections.action0Triggered, true,
278+ "Changing the model does not trigger the first action.");
279+ compare(selectedIndexSections.action1Triggered, false,
280+ "Changing the model triggers the second action.");
281+ compare(selectedIndexSections.action2Triggered, false,
282+ "Changing the model triggers the third action.");
283+ selectedIndexSections.actions = originalActions;
284+ wait_for_animation(selectedIndexSections);
285+ }
286 }
287 }

Subscribers

People subscribed via source and target branches