Merge lp:~fboucault/ubuntu-ui-toolkit/fix_tabs_ordering into lp:ubuntu-ui-toolkit

Proposed by Florian Boucault on 2013-11-25
Status: Merged
Approved by: Robert Bruce Park on 2013-11-26
Approved revision: 866
Merged at revision: 859
Proposed branch: lp:~fboucault/ubuntu-ui-toolkit/fix_tabs_ordering
Merge into: lp:ubuntu-ui-toolkit
Diff against target: 546 lines (+80/-361)
5 files modified
components.api (+0/-4)
modules/Ubuntu/Components/Tabs.qml (+51/-136)
tests/resources/navigation/Tabs.qml (+0/-44)
tests/unit_x11/tst_components/ExternalTab.qml (+0/-21)
tests/unit_x11/tst_components/tst_tabs.qml (+29/-156)
To merge this branch: bzr merge lp:~fboucault/ubuntu-ui-toolkit/fix_tabs_ordering
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing on 2013-11-26
Tim Peeters 2013-11-25 Approve on 2013-11-26
Review via email: mp+196619@code.launchpad.net

Commit message

[Tabs] Fix critical regression where tabs are badly ordered when defined with a Repeater.

Make sure the ordering of the tabs is identical to the ordering of Tabs' children.
Reverted recently introduced Tabs APIs: addTab, insertTab, moveTab, removeTab.
Added workaround for use of Tabs with a Repeater.

To post a comment you must log in.
Florian Boucault (fboucault) wrote :

Unit tests are being added as we speak. Hold off on merging.

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

missing semicolons ;)

199 + tabsModel.updateTabList(children)

239 + return (item && item.hasOwnProperty("itemAdded"))

Tim Peeters (tpeeters) wrote :

224 + for (var i = 0; i <= children.length; i++) {

i < children.length

Tim Peeters (tpeeters) wrote :

Unit tests pass on my laptop, except for TextFieldAPI which always fails for me.

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

Loading tests from: /home/tim/dev/ubuntu-ui-toolkit/fix_tabs_ordering/tests/autopilot

Tests running...

Ran 103 tests in 390.700s
OK

Tim Peeters (tpeeters) wrote :

None of the core apps use the removed API

tim@ideapad:~/dev/coreapps$ for qmlfile in $(find . -name "*.qml"); do grep Tab\( $qmlfile; done
            refreshSavedTab()
    function refreshSavedTab() {

Tim Peeters (tpeeters) wrote :

All UITK autopilot tests passed on maguro with trusty-proposed #29: https://pastebin.canonical.com/101018/

review: Approve
Tim Peeters (tpeeters) wrote :

CI test failed in an unrelated test that doesn't have any tabs in it.

PS Jenkins bot (ps-jenkins) wrote :
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-11-23 00:11:29 +0000
3+++ components.api 2013-11-26 00:06:03 +0000
4@@ -225,10 +225,6 @@
5 default property list<Item> tabChildren
6 readonly property int count
7 signal modelChanged()
8- function addTab(title, component, params)
9- function insertTab(index, title, component, params)
10- function moveTab(from, to)
11- function removeTab(index)
12 modules/Ubuntu/Components/TextArea.qml
13 StyledItem
14 property bool highlighted
15
16=== modified file 'modules/Ubuntu/Components/Tabs.qml'
17--- modules/Ubuntu/Components/Tabs.qml 2013-11-22 14:49:12 +0000
18+++ modules/Ubuntu/Components/Tabs.qml 2013-11-26 00:06:03 +0000
19@@ -200,139 +200,6 @@
20 signal modelChanged()
21
22 /*!
23- Appends a Tab dynamically to the list of tabs. The \a title specifies the
24- title of the Tab. The \a component can be either a Component, a URL to
25- the Tab component to be loaded or an instance of a pre-declared tab that
26- has been previously removed. The Tab's title will be replaced with the given
27- \a title, unless if the given value is empty string or undefined. The optional
28- \a params defines parameters passed to the Tab.
29- Returns the instance of the added Tab.
30- */
31- function addTab(title, component, params) {
32- return insertTab(count, title, component, params);
33- }
34-
35- /*!
36- Inserts a Tab at the given index. If the \a index is less or equal than 0,
37- the Tab will be added to the front, and to the end of the tab stack if the
38- \a index is greater than \l count. \a title, \a component and \a params
39- are used in the same way as in \l addTab(). Returns the instance of the
40- inserted Tab.
41- */
42- function insertTab(index, title, component, params) {
43- // check if the given component is a Tab instance
44- var tab = null;
45- if (component && component.hasOwnProperty("page") && component.hasOwnProperty("__protected")) {
46- // dynamically added components are destroyed upon removal, so
47- // in case we get a Tab as parameter, we can only have a predeclared one
48- // therefore we simply restore the default state of the removedFromTabs property
49- // and return the instance
50- if (!component.__protected.removedFromTabs) {
51- // exit if the Tab is not removed
52- return null;
53- }
54-
55- component.__protected.removedFromTabs = false;
56- tab = component;
57- } else {
58- var tabComponent = null;
59- if (typeof component === "string") {
60- tabComponent = Qt.createComponent(component);
61- } else {
62- tabComponent = component;
63- }
64- if (tabComponent.status === Component.Error) {
65- console.error(tabComponent.errorString());
66- return null;
67- }
68- tab = tabComponent.createObject();
69- tab.__protected.dynamic = true;
70- }
71-
72- // fix title
73- if (title !== undefined && title !== "") {
74- tab.title = title;
75- }
76-
77- // insert the created tab into the model
78- index = MathUtils.clamp(index, 0, count);
79- tab.__protected.inserted = true;
80- tab.__protected.index = index;
81- tabsModel.insert(index, tabsModel.listModel(tab));
82- tabsModel.reindex(index);
83- tab.parent = tabStack;
84- if (tabs.selectedTabIndex >= index) {
85- // move the selected index to the next index
86- tabs.selectedTabIndex += 1;
87- } else {
88- internal.sync();
89- }
90- return tab;
91- }
92-
93- /*!
94- Moves the tab from the given \a from position to the position given in \a to.
95- Returns true if the indexes were in 0..\l count - 1 boundary and if the operation
96- succeeds, and false otherwise. The \l selectedTabIndex is updated if it is
97- affected by the move (it is equal with \a from or falls between \a from and
98- \a to indexes).
99- */
100- function moveTab(from, to) {
101- if (from < 0 || from >= count || to < 0 || to >= count || from === to) return false;
102- var tabFrom = tabsModel.get(from).tab;
103- var tabTo = tabsModel.get(to).tab;
104-
105- // move tab
106- tabsModel.move(from, to, 1);
107- tabsModel.reindex();
108-
109- // fix selected tab
110- if (selectedTabIndex === from) {
111- selectedTabIndex = to;
112- } else if (selectedTabIndex <= to && selectedTabIndex >= from) {
113- selectedTabIndex -= 1;
114- } else {
115- internal.sync();
116- }
117-
118- return true;
119- }
120-
121- /*!
122- Removes the Tab from the given \a index. Returns true if the \a index falls
123- into 0..\l count - 1 boundary and the operation succeeds, and false on error.
124- The function removes also the pre-declared tabs. These can be added back using
125- \l addTab or \l insertTab by specifying the instance of the Tab to be added as
126- component. The \l selectedTabIndex is updated if is affected by the removal
127- (it is identical or greater than the tab index to be removed).
128- */
129- function removeTab(index) {
130- if (index < 0 || index >= count) return false;
131- var tab = tabsModel.get(index).tab;
132- var activeIndex = (selectedTabIndex >= index) ? MathUtils.clamp(selectedTabIndex, 0, count - 2) : -1;
133-
134- tabsModel.remove(index);
135- tabsModel.reindex();
136- // move active tab if needed
137- if (activeIndex >= 0) {
138- selectedTabIndex = activeIndex;
139- }
140-
141- if (tab.__protected.dynamic) {
142- tab.destroy();
143- } else {
144- // pre-declared tab, mark it as removed, so we don't update it next time
145- // the tabs stack children is updated
146- tab.__protected.removedFromTabs = true;
147- }
148-
149- if (activeIndex < 0) {
150- internal.sync();
151- }
152- return true;
153- }
154-
155- /*!
156 \internal
157 required by TabsStyle
158 */
159@@ -344,19 +211,30 @@
160 }
161
162 function updateTabList(tabsList) {
163+ var offset = 0;
164+ var tabIndex;
165 for (var i in tabsList) {
166 var tab = tabsList[i];
167 if (internal.isTab(tab)) {
168+ tabIndex = i - offset;
169 // make sure we have the right parent
170 tab.parent = tabStack;
171
172 if (!tab.__protected.inserted) {
173- tab.__protected.index = count;
174+ tab.__protected.index = tabIndex;
175 tab.__protected.inserted = true;
176- append(listModel(tab));
177+ insert(tabIndex, listModel(tab));
178 } else if (!tab.__protected.removedFromTabs && tabsModel.count > tab.index) {
179 get(tab.index).title = tab.title;
180 }
181+
182+ // always makes sure that tabsModel has the same order as tabsList
183+ move(tab.__protected.index, tabIndex, 1);
184+ reindex();
185+ } else {
186+ // keep track of children that are not tabs so that we compute
187+ // the right index for actual tabs
188+ offset += 1;
189 }
190 }
191 internal.sync();
192@@ -379,7 +257,40 @@
193 anchors.fill: parent
194 id: tabStack
195
196- onChildrenChanged: tabsModel.updateTabList(children)
197+ onChildrenChanged: {
198+ connectToRepeaters();
199+ tabsModel.updateTabList(children)
200+ }
201+
202+ /* When inserting a delegate into its parent the Repeater does it in 3
203+ steps:
204+ 1) sets the parent of the delegate thus inserting it in the list of
205+ children in a position that does not correspond to the position of
206+ the corresponding item in the model. At that point the
207+ childrenChanged() signal is emitted.
208+ 2) reorder the delegate to match the position of the corresponding item
209+ in the model.
210+ 3) emits the itemAdded() signal.
211+
212+ We need to update the list of tabs (tabsModel) when the children are in the
213+ adequate order hence the workaround below. It connects to the itemAdded()
214+ signal of any repeater it finds and triggers an update of the tabsModel.
215+
216+ Somewhat related Qt bug report:
217+ https://bugreports.qt-project.org/browse/QTBUG-32438
218+ */
219+ function updateTabsModel() {
220+ tabsModel.updateTabList(children);
221+ }
222+
223+ function connectToRepeaters() {
224+ for (var i = 0; i < children.length; i++) {
225+ var child = children[i];
226+ if (internal.isRepeater(child)) {
227+ child.itemAdded.connect(tabStack.updateTabsModel);
228+ }
229+ }
230+ }
231 }
232
233 QtObject {
234@@ -396,6 +307,10 @@
235 }
236 }
237
238+ function isRepeater(item) {
239+ return (item && item.hasOwnProperty("itemAdded"));
240+ }
241+
242 function sync() {
243 if (tabBar && tabBar.__styleInstance && tabBar.__styleInstance.hasOwnProperty("sync")) {
244 tabBar.__styleInstance.sync();
245
246=== modified file 'tests/resources/navigation/Tabs.qml'
247--- tests/resources/navigation/Tabs.qml 2013-11-22 14:49:12 +0000
248+++ tests/resources/navigation/Tabs.qml 2013-11-26 00:06:03 +0000
249@@ -22,27 +22,6 @@
250 width: 800
251 height: 600
252
253- Component {
254- id: dynamicTab
255- Tab {
256- title: "Original Title #" + index
257- page: Page {
258- Row {
259- anchors.centerIn: parent
260- spacing: 5
261- Button {
262- text: "Delete"
263- onClicked: tabs.removeTab(index)
264- }
265- Button {
266- text: "Move to 0"
267- onClicked: tabs.moveTab(index, 0)
268- }
269- }
270- }
271- }
272- }
273-
274 Tabs {
275 id: tabs
276 selectedTabIndex: 0
277@@ -77,29 +56,6 @@
278 iconSource: "call_icon.png"
279 onTriggered: print("action triggered")
280 }
281- ToolbarButton {
282- text: "ADD"
283- iconSource: "call_icon.png"
284- onTriggered: tabs.addTab("", dynamicTab)
285- }
286- ToolbarButton {
287- text: "INSERT"
288- iconSource: "call_icon.png"
289- onTriggered: tabs.insertTab(1, "INSERTED", dynamicTab)
290- }
291- ToolbarButton {
292- text: "move me next"
293- iconSource: "call_icon.png"
294- onTriggered: {
295- var i = simpleTab.index;
296- tabs.moveTab(i, i + 1);
297- }
298- }
299- ToolbarButton {
300- text: "delete 0"
301- iconSource: "call_icon.png"
302- onTriggered: tabs.removeTab(0)
303- }
304 }
305 }
306 }
307
308=== removed file 'tests/unit_x11/tst_components/ExternalTab.qml'
309--- tests/unit_x11/tst_components/ExternalTab.qml 2013-11-13 10:18:28 +0000
310+++ tests/unit_x11/tst_components/ExternalTab.qml 1970-01-01 00:00:00 +0000
311@@ -1,21 +0,0 @@
312-/*
313- * Copyright 2012 Canonical Ltd.
314- *
315- * This program is free software; you can redistribute it and/or modify
316- * it under the terms of the GNU Lesser General Public License as published by
317- * the Free Software Foundation; version 3.
318- *
319- * This program is distributed in the hope that it will be useful,
320- * but WITHOUT ANY WARRANTY; without even the implied warranty of
321- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
322- * GNU Lesser General Public License for more details.
323- *
324- * You should have received a copy of the GNU Lesser General Public License
325- * along with this program. If not, see <http://www.gnu.org/licenses/>.
326- */
327-
328-import QtQuick 2.0
329-import Ubuntu.Components 0.1
330-
331-Tab {
332-}
333
334=== modified file 'tests/unit_x11/tst_components/tst_tabs.qml'
335--- tests/unit_x11/tst_components/tst_tabs.qml 2013-11-13 10:18:28 +0000
336+++ tests/unit_x11/tst_components/tst_tabs.qml 2013-11-26 00:06:03 +0000
337@@ -26,13 +26,6 @@
338 id: emptyTabs
339 }
340
341- Component {
342- id: dynamicTab
343- Tab{
344- title: "OriginalTitle"
345- }
346- }
347-
348 MainView {
349 id: mainView
350 anchors.fill: parent
351@@ -107,12 +100,40 @@
352 }
353 }
354
355-
356+ Tabs {
357+ id: tabsWithRepeater
358+ ListModel {
359+ id: inputModel
360+ Component.onCompleted: {
361+ append({ "name": "tab 1" });
362+ insert(0, { "name": "tab 0" });
363+ append({ "name": "tab 3" });
364+ insert(2, { "name": "tab 2" });
365+ }
366+ }
367+ Repeater {
368+ id: tabsRepeater
369+ model: inputModel
370+ Tab {
371+ title: name
372+ }
373+ }
374+ }
375
376 TestCase {
377 name: "TabsAPI"
378 when: windowShown
379
380+ function test_tabOrder_bug1253804() {
381+ var tabsModel = tabsWithRepeater.tabBar.model;
382+
383+ compare(tabsRepeater.count, inputModel.count, "Incorrect number of tabs in Tabs");
384+ compare(tabsModel.count, tabsRepeater.count, "Incorrect number of tabs in TabBar");
385+ for (var i=0; i < tabsModel.count; i++) {
386+ compare(tabsModel.get(i).title, inputModel.get(i).name, "Tab titles don't match for index "+i);
387+ }
388+ }
389+
390 function test_emptyTabs() {
391 compare(emptyTabs.selectedTabIndex, -1, "The default value for selectedTabIndex is -1 when there are no tabs");
392 compare(emptyTabs.selectedTab, null, "The default selected tab is null when there are no tabs");
393@@ -191,153 +212,5 @@
394 mouseClick(button, units.gu(1), units.gu(1), Qt.LeftButton);
395 compare(tabs.tabBar.selectionMode, false, "Tab bar deactivated by interacting with the page contents");
396 }
397-
398- function test_z_addTab() {
399- var newTab = tabs.addTab("Dynamic Tab", dynamicTab);
400- compare((newTab !== null), true, "tab added");
401- compare(newTab.active, false, "the inserted tab is inactive");
402- compare(newTab.index, tabs.count - 1, "the tab is the last one");
403- }
404-
405- function test_z_addExternalTab() {
406- var newTab = tabs.addTab("External Tab", Qt.resolvedUrl("ExternalTab.qml"));
407- compare((newTab !== null), true, "tab added");
408- compare(newTab.active, false, "the inserted tab is inactive");
409- compare(newTab.index, tabs.count - 1, "the tab is the last one");
410- }
411-
412- function test_z_addTabWithDefaultTitle() {
413- var newTab = tabs.addTab("", dynamicTab);
414- compare((newTab !== null), true, "tab added");
415- compare(newTab.title, "OriginalTitle", "tab created with original title");
416- }
417-
418- function test_z_insertTab() {
419- var tabIndex = Math.ceil(tabs.count / 2);
420- var newTab = tabs.insertTab(tabIndex, "Inserted tab", dynamicTab);
421- compare((newTab !== null), true, "tab inserted");
422- compare(newTab.index, tabIndex, "this is the first tab");
423- compare(tabs.selectedTab !== newTab, true, "the new tab is not the active one");
424- }
425-
426- function test_z_insertExternalTab() {
427- var tabIndex = Math.ceil(tabs.count / 2);
428- var newTab = tabs.insertTab(tabIndex, "Inserted External tab", Qt.resolvedUrl("ExternalTab.qml"));
429- compare((newTab !== null), true, "tab inserted");
430- compare(newTab.index, tabIndex, "this is the first tab");
431- compare(tabs.selectedTab !== newTab, true, "the new tab is not the active one");
432- }
433-
434- function test_z_insertTabAtSelectedIndex() {
435- tabs.selectedTabIndex = 1;
436- var tabIndex = tabs.selectedTabIndex - 1;
437- var newTab = tabs.insertTab(tabIndex, "InsertedAtSelected tab", dynamicTab);
438- compare((newTab !== null), true, "tab inserted");
439- compare(newTab.index, tabIndex, "inserted at selected tab");
440- compare(tabs.selectedTabIndex != (tabIndex + 1), true, "it is not the selected tab");
441- }
442-
443- function test_z_insertTabFront() {
444- var newTab = tabs.insertTab(-1, "PreTab", dynamicTab);
445- compare(newTab !== null, true, "pre-tab inserted");
446- compare(newTab.index, 0, "this is the new first tab");
447- compare(tabs.selectedTab !== newTab, true, "the new tab is not the active one");
448- }
449-
450- function test_z_insertTabEnd() {
451- var newTab = tabs.insertTab(tabs.count, "PostTab", dynamicTab);
452- compare(newTab !== null, true, "post-tab inserted");
453- compare(newTab.index, tabs.count - 1, "thsi is the new last tab");
454- compare(tabs.selectedTab !== newTab, true, "the new tab is not the active one");
455- }
456-
457- function test_z_insertTabAndActivate() {
458- var newTab = tabs.addTab("Inserted tab", dynamicTab);
459- compare((newTab !== null), true, "tab inserted");
460- compare(newTab.index, tabs.count - 1, "the tab is the last one");
461- tabs.selectedTabIndex = newTab.index;
462- compare(tabs.selectedTab, newTab, "the inserted tab is selected");
463- compare(newTab.active, true, "the new tab is active");
464- }
465-
466- function test_z_moveTab() {
467- var selectedIndex = tabs.count - 1;
468- tabs.selectedTabIndex = selectedIndex;
469- compare(tabs.moveTab(0, selectedIndex), true, "first tab moved to last");
470- compare(tabs.selectedTabIndex, selectedIndex - 1, "the selected index moved backwards");
471- tabs.selectedTabIndex = selectedIndex = 0;
472- compare(tabs.moveTab(selectedIndex, selectedIndex + 1), true, "selected tab moved as next");
473- compare(tabs.selectedTabIndex, selectedIndex + 1, "the selected index moved forewards");
474- }
475-
476- function test_z_moveSelectedTab() {
477- tabs.selectedTabIndex = 0;
478- tabs.moveTab(0, 1);
479- compare(tabs.selectedTabIndex, 1, "selected tab moved");
480- }
481-
482- function test_z_moveTabFail() {
483- compare(tabs.moveTab(-1, tabs.count - 1), false, "from-parameter out of range");
484- compare(tabs.moveTab(0, tabs.count), false, "to-parameter out of range");
485- }
486-
487- function test_z_removeTab() {
488- compare(tabs.removeTab(tabs.count - 1), true, "last tab removed");
489- tabs.selectedTabIndex = 0;
490- compare(tabs.removeTab(0), true, "active tab removed");
491- compare(tabs.selectedTabIndex, 0, "the next tab is selected")
492- }
493-
494- function test_z_removeTabAfterActiveTab() {
495- var activeTab = tabs.count - 2;
496- tabs.selectedTabIndex = activeTab;
497- compare(tabs.removeTab(tabs.count - 1), true, "last tab removed");
498- compare(tabs.selectedTabIndex, activeTab, "the selected tab wasn't moved");
499- }
500-
501- function test_z_removeTabBeforeActiveTab() {
502- var activeTab = tabs.count - 1;
503- tabs.selectedTabIndex = activeTab;
504- compare(tabs.removeTab(0), true, "first tab removed");
505- compare(tabs.selectedTabIndex, activeTab - 1, "the selected tab index decreased");
506- }
507-
508- function test_z_removeActiveTab() {
509- tabs.selectedTabIndex = 1;
510- compare(tabs.removeTab(1), true, "selected tab removed");
511- compare(tabs.selectedTabIndex, 1, "selected tab is next");
512-
513- tabs.selectedTabIndex = tabs.count - 1;
514- compare(tabs.removeTab(tabs.count - 1), true, "last tab removed");
515- compare(tabs.selectedTabIndex, tabs.count - 1, "selected tab moved to last item");
516- }
517-
518- function test_zz_addTabAfterCleaningUpTabs() {
519- while (tabs.count > 1) {
520- tabs.removeTab(0);
521- }
522- compare(tabs.selectedTabIndex, 0, "the only tab is the selected one");
523- // add a new tab anc check the count (default added tas should not be added anymore
524- tabs.addTab("Second tab", dynamicTab);
525- compare(tabs.count, 2, "we have two tabs only");
526- }
527-
528- function test_zz_addPredeclaredTab() {
529- while (tabs.count > 1) {
530- tabs.removeTab(0);
531- }
532- compare(tabs.selectedTabIndex, 0, "the only tab is the selected one");
533-
534- // add a predeclared tab back with original title
535- compare(tabs.addTab("", tab1), tab1, "tab1 was added back");
536- compare(tab1.title, "tab 1", "with the original title");
537-
538- // add a predeclared tab back with new title
539- compare(tabs.addTab("Original tab", tab2), tab2, "tab2 was added back");
540- compare(tab2.title, "Original tab", "with modified title");
541-
542- // add a predeclared tab which was added already
543- compare(tabs.addTab("", tab1), null, "tab1 is already in tabs");
544- }
545 }
546 }

Subscribers

People subscribed via source and target branches

to status/vote changes: