Merge lp:~uriboni/webbrowser-app/tab-context-menu into lp:webbrowser-app

Proposed by Ugo Riboni on 2015-08-17
Status: Rejected
Rejected by: Olivier Tilloy on 2015-09-23
Proposed branch: lp:~uriboni/webbrowser-app/tab-context-menu
Merge into: lp:webbrowser-app
Diff against target: 842 lines (+455/-96)
8 files modified
src/app/webbrowser/Browser.qml (+6/-5)
src/app/webbrowser/Chrome.qml (+2/-2)
src/app/webbrowser/TabsBar.qml (+42/-3)
src/app/webbrowser/tabs-model.cpp (+53/-21)
src/app/webbrowser/tabs-model.h (+2/-0)
tests/unittests/qml/qml_tree_helpers.js (+43/-0)
tests/unittests/qml/tst_TabsBar.qml (+137/-8)
tests/unittests/tabs-model/tst_TabsModelTests.cpp (+170/-57)
To merge this branch: bzr merge lp:~uriboni/webbrowser-app/tab-context-menu
Reviewer Review Type Date Requested Status
Olivier Tilloy 2015-08-17 Disapprove on 2015-09-23
PS Jenkins bot continuous-integration Needs Fixing on 2015-09-17
Riccardo Padovani (community) Needs Fixing on 2015-08-31
Review via email: mp+268220@code.launchpad.net

Commit message

Add a context menu to each tab in the tab bar, allowing to insert a new tab just after, close or reload the current tab.

Description of the change

Add a context menu to each tab in the tab bar, allowing to insert a new tab just after, close or reload the current tab

To post a comment you must log in.
1135. By Ugo Riboni on 2015-08-17

Merge changes from trunk

1136. By Ugo Riboni on 2015-08-18

Access the contextual menu items in a way that is safer and will work regardless of the language used by the system running the tests

Riccardo Padovani (rpadovani) wrote :

Cool feature!

Anyway, it needs fix:

## Address bar isn't alway cleaned:
- Open the browser with only one tab
- Open another tab and go to a website
- While on the second tab, open the context menu of the first tab
- Two errors (related, I think): the tab switches to the new tab, and the address bar has the old address.

I think you need to incremente current index if you add a tab to the left of the current tab.

## Closing a left tab is broken

Similar to the previous issue: if you close a tab that is left of the current tab, and you've another tab at right, the tab selector moves to right. You need to decrement the current tab index

## Core dump

Not sure if related (but I wasn't able to have it in trunk) but I've a core dump if I open and close two tabs and navigate in both to the same address: http://pastebin.ubuntu.com/12237348/

review: Needs Fixing
1137. By Ugo Riboni on 2015-09-02

Fix wrong corner cases in TabsModel related to insert/remove and interactions with currentIndex. Refactor unit tests to more clearly and thouroughly test everything.

1138. By Ugo Riboni on 2015-09-02

Merge changes from trunk

Olivier Tilloy (osomon) wrote :

This branch has conflicts when merging into the latest trunk. Can you please resolve them?

review: Needs Fixing
1139. By Ugo Riboni on 2015-09-14

Merge changes from trunk

Ugo Riboni (uriboni) wrote :

> This branch has conflicts when merging into the latest trunk. Can you please
> resolve them?

The conflicting changes were related to the tabs model, which you changed for the "sad tab" MR. The change was about correctly setting the current index when a tab is removed before the current tab (rev 1002.3.23), and corresponding additions to the unit tests.
My branch does the same thing, even if in a different way, so I simply reverted the two conflicting files to my version when merging from trunk. As far as I can tell everything should work as you intended and should be tested too, but please double check.

1140. By Ugo Riboni on 2015-09-16

Merge changes from trunk

Olivier Tilloy (osomon) wrote :

In src/app/webbrowser/TabsBar.qml:

    var menu = PopupUtils.open(contextualOptionsComponent, tabDelegate)
    menu.targetIndex = index

  'targetIndex' should be set at instantiation time, passing properties to PopupUtils.open()

    if (tab.url.toString().length > 0) tab.webview.reload()

  This won’t work if the webview hasn’t been instantiated (they aren’t until the tabs are first activated). You’ll need to check whether the webview is not null before calling reload().

In src/app/webbrowser/tabs-model.cpp:

    if (index < 0) index == 0;

  Looks like an assignment was meant, but it’s a silent comparison instead.

    In TabsModel::insert(), the call to setCurrentIndexNoCheck, in case a tab is inserted in the model before the current tab, will fire currentIndexChanged and currentTabChanged, but this is incorrect as only the current index changed, not the current tab.

In tests/unittests/qml/qml_tree_helpers.js:

    function qmlType() can be simplified, by always returning String(item).split("_")[0]
    findChildrenByType() should probably be renamed findDescendantsByType, as it’s recursive

In tests/unittests/qml/tst_TabsBar.qml:

    it looks like bug #1205144 was fixed in the UITK, can the tests be simplified now?

In tests/unittests/tabs-model/tst_TabsModelTests.cpp:

    there are inconsistent indentations (4 spaces mixed with 2 spaces)

review: Needs Fixing
Olivier Tilloy (osomon) wrote :
review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/Browser.qml'
2--- src/app/webbrowser/Browser.qml 2015-09-13 21:24:56 +0000
3+++ src/app/webbrowser/Browser.qml 2015-09-16 22:49:55 +0000
4@@ -353,7 +353,7 @@
5 onRemoved: if (chrome.bookmarked && (url === chrome.webview.url)) chrome.bookmarked = false
6 }
7
8- onRequestNewTab: browser.openUrlInNewTab("", true)
9+ onRequestNewTab: browser.openUrlInNewTab("", makeCurrent, true, index)
10
11 onFindInPageModeChanged: if (!chrome.findInPageMode) internal.resetFocus()
12
13@@ -1221,8 +1221,9 @@
14 }
15 }
16
17- function addTab(tab, setCurrent) {
18- var index = tabsModel.add(tab)
19+ function addTab(tab, setCurrent, index) {
20+ if (index === undefined) index = tabsModel.add(tab)
21+ else index = tabsModel.insert(tab, index)
22 if (setCurrent) {
23 tabsModel.currentIndex = index
24 chrome.requestedUrl = tab.initialUrl
25@@ -1325,10 +1326,10 @@
26 }
27 }
28
29- function openUrlInNewTab(url, setCurrent, load) {
30+ function openUrlInNewTab(url, setCurrent, load, index) {
31 load = typeof load !== 'undefined' ? load : true
32 var tab = tabComponent.createObject(tabContainer, {"initialUrl": url, 'incognito': browser.incognito})
33- internal.addTab(tab, setCurrent)
34+ internal.addTab(tab, setCurrent, index)
35 if (load) {
36 tabsModel.currentTab.load()
37 }
38
39=== modified file 'src/app/webbrowser/Chrome.qml'
40--- src/app/webbrowser/Chrome.qml 2015-08-13 11:11:59 +0000
41+++ src/app/webbrowser/Chrome.qml 2015-09-16 22:49:55 +0000
42@@ -39,7 +39,7 @@
43 property alias showFaviconInAddressBar: navigationBar.showFaviconInAddressBar
44 readonly property alias bookmarkTogglePlaceHolder: navigationBar.bookmarkTogglePlaceHolder
45
46- signal requestNewTab()
47+ signal requestNewTab(int index, bool makeCurrent)
48
49 backgroundColor: incognito ? UbuntuColors.darkGrey : Theme.palette.normal.background
50
51@@ -60,7 +60,7 @@
52 sourceComponent: TabsBar {
53 model: tabsModel
54 incognito: chrome.incognito
55- onRequestNewTab: chrome.requestNewTab()
56+ onRequestNewTab: chrome.requestNewTab(index, makeCurrent)
57 }
58
59 anchors {
60
61=== modified file 'src/app/webbrowser/TabsBar.qml'
62--- src/app/webbrowser/TabsBar.qml 2015-08-10 15:22:00 +0000
63+++ src/app/webbrowser/TabsBar.qml 2015-09-16 22:49:55 +0000
64@@ -18,6 +18,7 @@
65
66 import QtQuick 2.4
67 import Ubuntu.Components 1.3
68+import Ubuntu.Components.Popups 1.3
69 import ".."
70
71 Item {
72@@ -31,7 +32,7 @@
73
74 property bool incognito: false
75
76- signal requestNewTab()
77+ signal requestNewTab(int index, bool makeCurrent)
78
79 MouseArea {
80 anchors.fill: parent
81@@ -67,7 +68,38 @@
82 color: incognito ? "white" : UbuntuColors.darkGrey
83 }
84
85- onClicked: root.requestNewTab()
86+ onClicked: root.requestNewTab(root.model.count, true)
87+ }
88+
89+ Component {
90+ id: contextualOptionsComponent
91+ ActionSelectionPopover {
92+ id: menu
93+ objectName: "tabContextualActions"
94+ property int targetIndex
95+
96+ actions: ActionList {
97+ Action {
98+ objectName: "tab_action_new_tab"
99+ text: i18n.tr("New Tab")
100+ onTriggered: root.requestNewTab(menu.targetIndex + 1, false)
101+ }
102+ Action {
103+ objectName: "tab_action_reload"
104+ text: i18n.tr("Reload")
105+ enabled: root.model.get(menu.targetIndex).url.toString().length > 0
106+ onTriggered: {
107+ var tab = root.model.get(menu.targetIndex)
108+ if (tab.url.toString().length > 0) tab.webview.reload()
109+ }
110+ }
111+ Action {
112+ objectName: "tab_action_close_tab"
113+ text: i18n.tr("Close Tab")
114+ onTriggered: internal.closeTab(menu.targetIndex)
115+ }
116+ }
117+ }
118 }
119
120 Item {
121@@ -101,7 +133,7 @@
122 MouseArea {
123 id: mouseArea
124 anchors.fill: parent
125- acceptedButtons: Qt.LeftButton | Qt.MiddleButton
126+ acceptedButtons: Qt.LeftButton | Qt.MiddleButton | Qt.RightButton
127 hoverEnabled: true
128 onPressed: {
129 if (mouse.button === Qt.LeftButton) {
130@@ -113,6 +145,13 @@
131 internal.closeTab(index)
132 }
133 }
134+ onClicked: {
135+ if (mouse.button === Qt.RightButton) {
136+ var menu = PopupUtils.open(contextualOptionsComponent, tabDelegate)
137+ menu.targetIndex = index
138+ }
139+ }
140+
141 // XXX: should not start a drag when middle button was pressed
142 drag {
143 target: tabDelegate
144
145=== modified file 'src/app/webbrowser/tabs-model.cpp'
146--- src/app/webbrowser/tabs-model.cpp 2015-09-02 16:48:01 +0000
147+++ src/app/webbrowser/tabs-model.cpp 2015-09-16 22:49:55 +0000
148@@ -87,49 +87,75 @@
149 return m_currentIndex;
150 }
151
152+void TabsModel::setCurrentIndexNoCheck(int index)
153+{
154+ if (index != m_currentIndex) {
155+ m_currentIndex = index;
156+ Q_EMIT currentIndexChanged();
157+ Q_EMIT currentTabChanged();
158+ }
159+}
160+
161 void TabsModel::setCurrentIndex(int index)
162 {
163- if (!checkValidTabIndex(index)) {
164- return;
165- }
166- if (index != m_currentIndex) {
167- m_currentIndex = index;
168- Q_EMIT currentIndexChanged();
169- Q_EMIT currentTabChanged();
170+ if (checkValidTabIndex(index)) {
171+ setCurrentIndexNoCheck(index);
172 }
173 }
174
175 QObject* TabsModel::currentTab() const
176 {
177- if (m_tabs.isEmpty()) {
178+ if (m_tabs.isEmpty() || !checkValidTabIndex(m_currentIndex)) {
179 return nullptr;
180 }
181 return m_tabs.at(m_currentIndex);
182 }
183
184 /*!
185- Add a tab to the model and return the corresponding index in the model.
186+ Append a tab to the model and return the corresponding index in the model.
187
188 It is the responsibility of the caller to instantiate the corresponding
189 Tab beforehand.
190 */
191 int TabsModel::add(QObject* tab)
192 {
193+ return insert(tab, m_tabs.count());
194+}
195+
196+/*!
197+ Add a tab to the model at the specified index, and return the index itself,
198+ or -1 if the operation failed.
199+
200+ It is the responsibility of the caller to instantiate the corresponding
201+ Tab beforehand.
202+*/
203+int TabsModel::insert(QObject* tab, int index)
204+{
205 if (tab == nullptr) {
206 qWarning() << "Invalid Tab";
207 return -1;
208 }
209- int index = m_tabs.count();
210+ if (index < 0) index == 0;
211+ else if (index > m_tabs.count()) index = m_tabs.count();
212 beginInsertRows(QModelIndex(), index, index);
213- m_tabs.append(tab);
214+ m_tabs.insert(index, tab);
215 connect(tab, SIGNAL(urlChanged()), SLOT(onUrlChanged()));
216 connect(tab, SIGNAL(titleChanged()), SLOT(onTitleChanged()));
217 connect(tab, SIGNAL(iconChanged()), SLOT(onIconChanged()));
218 endInsertRows();
219 Q_EMIT countChanged();
220- if (index == 0) {
221- setCurrentIndex(0);
222+
223+ if (m_currentIndex == -1) {
224+ // set the index to zero if this is the first item that gets added to the
225+ // model, as it should not be possible to have items in the model but no
226+ // current tab.
227+ setCurrentIndexNoCheck(0);
228+ } else if (index <= m_currentIndex) {
229+ // increment the index if we are inserting items before the
230+ // current index
231+ setCurrentIndexNoCheck(m_currentIndex + 1);
232 }
233+
234 return index;
235 }
236
237@@ -150,15 +176,21 @@
238 tab->disconnect(this);
239 endRemoveRows();
240 Q_EMIT countChanged();
241- if (index == m_currentIndex) {
242- if (!checkValidTabIndex(index)) {
243- m_currentIndex = m_tabs.count() - 1;
244- Q_EMIT currentIndexChanged();
245+
246+ if (index < m_currentIndex) {
247+ // If we removed any tab before the current one, decrease the
248+ // currentIndex to match (which will trigger a currentTab change too)
249+ setCurrentIndexNoCheck(m_currentIndex - 1);
250+ } else if (index == m_currentIndex) {
251+ // If we removed the current tab, then select the following one, if any.
252+ // This will only trigger a currentTab change since the index remains
253+ // the same.
254+ // Otherwise select the previous tab, which will trigger both changes.
255+ if (m_currentIndex == m_tabs.count()) {
256+ setCurrentIndexNoCheck(m_currentIndex - 1);
257+ } else {
258+ Q_EMIT currentTabChanged();
259 }
260- Q_EMIT currentTabChanged();
261- } else if (m_currentIndex > index) {
262- m_currentIndex -= 1;
263- Q_EMIT currentIndexChanged();
264 }
265 return tab;
266 }
267
268=== modified file 'src/app/webbrowser/tabs-model.h'
269--- src/app/webbrowser/tabs-model.h 2015-06-18 14:28:11 +0000
270+++ src/app/webbrowser/tabs-model.h 2015-09-16 22:49:55 +0000
271@@ -57,6 +57,7 @@
272 QObject* currentTab() const;
273
274 Q_INVOKABLE int add(QObject* tab);
275+ Q_INVOKABLE int insert(QObject* tab, int index);
276 Q_INVOKABLE QObject* remove(int index);
277 Q_INVOKABLE QObject* get(int index) const;
278 Q_INVOKABLE void move(int from, int to);
279@@ -76,6 +77,7 @@
280 int m_currentIndex;
281
282 bool checkValidTabIndex(int index) const;
283+ void setCurrentIndexNoCheck(int index);
284 void onDataChanged(QObject* tab, int role);
285 };
286
287
288=== added file 'tests/unittests/qml/qml_tree_helpers.js'
289--- tests/unittests/qml/qml_tree_helpers.js 1970-01-01 00:00:00 +0000
290+++ tests/unittests/qml/qml_tree_helpers.js 2015-09-16 22:49:55 +0000
291@@ -0,0 +1,43 @@
292+/*
293+ * Copyright 2015 Canonical Ltd.
294+ *
295+ * This file is part of webbrowser-app.
296+ *
297+ * webbrowser-app is free software; you can redistribute it and/or modify
298+ * it under the terms of the GNU General Public License as published by
299+ * the Free Software Foundation; version 3.
300+ *
301+ * webbrowser-app is distributed in the hope that it will be useful,
302+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
303+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
304+ * GNU General Public License for more details.
305+ *
306+ * You should have received a copy of the GNU General Public License
307+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
308+ */
309+
310+// Given the naming convention in QML for class names, we should
311+// never have a class name with underscores in it, so the following
312+// should be a safe way to remove the rest of the extra metatype
313+// information produced by converting QML objects to strings.
314+function qmlType(item) {
315+ var itemType = String(item).split("_")
316+ return itemType.length > 0 ? itemType[0] : String(item)
317+}
318+
319+function findChildrenByType(item, type, list) {
320+ list = list || []
321+ if (qmlType(item) === type) list.push(item)
322+ for (var i in item.children) {
323+ findChildrenByType(item.children[i], type, list)
324+ }
325+ return list
326+}
327+
328+function findAncestorByType(item, type) {
329+ while (item.parent) {
330+ if (qmlType(item.parent) === type) return item.parent
331+ item = item.parent
332+ }
333+ return null
334+}
335
336=== modified file 'tests/unittests/qml/tst_TabsBar.qml'
337--- tests/unittests/qml/tst_TabsBar.qml 2015-08-10 15:22:00 +0000
338+++ tests/unittests/qml/tst_TabsBar.qml 2015-09-16 22:49:55 +0000
339@@ -21,12 +21,14 @@
340 import Ubuntu.Test 1.0
341 import "../../../src/app/webbrowser"
342 import webbrowserapp.private 0.1
343+import "qml_tree_helpers.js" as Tree
344
345 Item {
346 id: root
347
348 width: 600
349- height: 50
350+ height: 200
351+ signal reload(string url)
352
353 TabsModel {
354 id: tabsModel
355@@ -35,22 +37,37 @@
356 Component {
357 id: tabComponent
358 QtObject {
359+ id: tab
360 property url url
361 property string title
362 property url icon
363 function close() { destroy() }
364+ property QtObject webview: QtObject {
365+ function reload() { root.reload(tab.url) }
366+ }
367 }
368 }
369
370 TabsBar {
371 id: tabs
372- anchors.fill: parent
373+
374+ // Make the tabs bar smaller than the window and aligned in the middle
375+ // to leave room for the context menu to pop up and have all its items
376+ // visible within the screen.
377+ anchors.left: parent.left
378+ anchors.right: parent.right
379+ anchors.verticalCenter: parent.verticalCenter
380+ height: 50
381+
382 model: tabsModel
383- onRequestNewTab: appendTab("", "", "")
384+ onRequestNewTab: insertTab("", "", "", index)
385 function appendTab(url, title, icon) {
386+ insertTab(url, title, icon, model.count)
387+ model.currentIndex = model.count - 1
388+ }
389+ function insertTab(url, title, icon, index) {
390 var tab = tabComponent.createObject(root, {"url": url, "title": title, "icon": icon})
391- model.add(tab)
392- model.currentIndex = model.count - 1
393+ model.insert(tab, index)
394 }
395 }
396
397@@ -60,13 +77,45 @@
398 signalName: "requestNewTab"
399 }
400
401+ SignalSpy {
402+ id: reloadSpy
403+ target: root
404+ signalName: "reload"
405+ }
406+
407+ // Ideally we would get menu items by their objectName, however they are
408+ // created dynamically by the ActionSelectionPopover and the objectName
409+ // of the source action is not copied to the generated item.
410+ // So we first select the source Action by objectName then look for an item
411+ // with the same text as the action. This is not ideal but it will work
412+ // since we don't have items with the same text.
413+ // See this bug report for a request to fix this: http://pad.lv/1205144
414+ function getMenuItemForAction(menu, actionName) {
415+ actionName = "tab_action_" + actionName
416+ var text = ""
417+ var actions = menu.actions.actions
418+ for (var i = 0; i < actions.length; i++) {
419+ if (actions[i].objectName === actionName) {
420+ text = actions[i].text
421+ break
422+ }
423+ }
424+ if (text === "") return null
425+
426+ var menuItems = Tree.findChildrenByType(menu, "Label")
427+ var matching = menuItems.filter(function(item) { return item.text === text })
428+ if (matching.length === 0) return null
429+ else return Tree.findAncestorByType(matching[0], "Empty")
430+ }
431+
432 UbuntuTestCase {
433 name: "TabsBar"
434 when: windowShown
435
436- function clickItem(item) {
437+ function clickItem(item, button) {
438+ if (button === undefined) button = Qt.LeftButton
439 var center = centerOf(item)
440- mouseClick(item, center.x, center.y)
441+ mouseClick(item, center.x, center.y, button)
442 }
443
444 function getTabDelegate(index) {
445@@ -80,11 +129,22 @@
446 return null
447 }
448
449+ function popupMenuOnTab(index) {
450+ var tab = getTabDelegate(index)
451+ if (tab) {
452+ clickItem(tab, Qt.RightButton)
453+ var menu = findChild(root, "tabContextualActions")
454+ waitForRendering(menu)
455+ return menu
456+ } else return null
457+ }
458+
459 function cleanup() {
460 while (tabsModel.count > 0) {
461 tabsModel.remove(0).destroy()
462 }
463 newTabRequestSpy.clear()
464+ reloadSpy.clear()
465 }
466
467 function populateTabs() {
468@@ -101,6 +161,9 @@
469 clickItem(newTabButton)
470 }
471 compare(newTabRequestSpy.count, 3)
472+ compare(newTabRequestSpy.signalArguments[0][0], 0)
473+ compare(newTabRequestSpy.signalArguments[1][0], 1)
474+ compare(newTabRequestSpy.signalArguments[2][0], 2)
475 }
476
477 function test_mouse_left_click() {
478@@ -117,11 +180,19 @@
479 populateTabs()
480 for (var i = 2; i >= 0; --i) {
481 var tab0 = getTabDelegate(0)
482- mouseClick(tab0, centerOf(tab0).x, centerOf(tab0).y, Qt.MiddleButton)
483+ clickItem(tab0, Qt.MiddleButton)
484 compare(tabsModel.count, i)
485 }
486 }
487
488+ function test_mouse_right_click() {
489+ // Right click pops up the contextual actions menu
490+ populateTabs()
491+ var menu = popupMenuOnTab(0)
492+ verify(menu)
493+ verify(menu.visible)
494+ }
495+
496 function test_mouse_wheel() {
497 // Wheel events cycle through open tabs
498 populateTabs()
499@@ -169,5 +240,63 @@
500 tab = getTabDelegate(1)
501 dragTab(tab, -tab.width * 2, 0)
502 }
503+
504+ function test_menu_states_on_new_tab() {
505+ populateTabs()
506+ var menu = popupMenuOnTab(0)
507+ var item = getMenuItemForAction(menu, "new_tab")
508+ verify(item.enabled)
509+ item = getMenuItemForAction(menu, "reload")
510+ verify(!item.enabled)
511+ item = getMenuItemForAction(menu, "close_tab")
512+ verify(item.enabled)
513+ }
514+
515+ function test_menu_states_on_page() {
516+ tabs.appendTab("http://localhost/", "tab", "")
517+ var menu = popupMenuOnTab(0)
518+ var item = getMenuItemForAction(menu, "new_tab")
519+ verify(item.enabled)
520+ item = getMenuItemForAction(menu, "reload")
521+ verify(item.enabled)
522+ item = getMenuItemForAction(menu, "close_tab")
523+ verify(item.enabled)
524+ }
525+
526+ function test_context_menu_close() {
527+ populateTabs()
528+ var menu = popupMenuOnTab(1)
529+ var item = getMenuItemForAction(menu, "close_tab")
530+ clickItem(item)
531+ compare(tabsModel.count, 2)
532+ compare(tabsModel.get(0).title, "tab 0")
533+ compare(tabsModel.get(1).title, "tab 2")
534+ }
535+
536+ function test_context_menu_reload() {
537+ var baseUrl = "http://localhost/"
538+ tabs.appendTab(baseUrl + "1", "tab 1", "")
539+ tabs.appendTab(baseUrl + "2", "tab 2", "")
540+ var menu = popupMenuOnTab(1)
541+ var item = getMenuItemForAction(menu, "reload")
542+ clickItem(item)
543+ compare(reloadSpy.count, 1)
544+ compare(reloadSpy.signalArguments[0][0], baseUrl + "2")
545+ }
546+
547+ function test_context_menu_new_tab() {
548+ var baseUrl = "http://localhost/"
549+ tabs.appendTab(baseUrl + "1", "tab 1", "")
550+ tabs.appendTab(baseUrl + "2", "tab 2", "")
551+ var menu = popupMenuOnTab(0)
552+ var item = getMenuItemForAction(menu, "new_tab")
553+ clickItem(item)
554+ compare(newTabRequestSpy.count, 1)
555+ compare(newTabRequestSpy.signalArguments[0][0], 1)
556+ compare(tabsModel.count, 3)
557+ compare(tabsModel.get(0).url, baseUrl + "1")
558+ compare(tabsModel.get(1).url, "")
559+ compare(tabsModel.get(2).url, baseUrl + "2")
560+ }
561 }
562 }
563
564=== modified file 'tests/unittests/tabs-model/tst_TabsModelTests.cpp'
565--- tests/unittests/tabs-model/tst_TabsModelTests.cpp 2015-09-02 16:48:01 +0000
566+++ tests/unittests/tabs-model/tst_TabsModelTests.cpp 2015-09-16 22:49:55 +0000
567@@ -17,6 +17,7 @@
568 */
569
570 // Qt
571+#include <QtCore/QStringList>
572 #include <QtQml/QQmlComponent>
573 #include <QtQml/QQmlEngine>
574 #include <QtQml/QQmlProperty>
575@@ -47,6 +48,22 @@
576 return item;
577 }
578
579+ QQuickItem* createTabWithTitle(const QString& title) {
580+ QQuickItem* tab = createTab();
581+ tab->setProperty("title", title);
582+ return tab;
583+ }
584+
585+ void verifyTabsOrder(QStringList orderedTitles) {
586+ QCOMPARE(model->rowCount(), orderedTitles.count());
587+
588+ int i = 0;
589+ Q_FOREACH(QString title, orderedTitles) {
590+ QCOMPARE(model->get(i)->property("title").toString(), title);
591+ i++;
592+ }
593+ }
594+
595 private Q_SLOTS:
596 void init()
597 {
598@@ -113,6 +130,51 @@
599 QCOMPARE(model->rowCount(), 1);
600 }
601
602+ void shouldNotInsertNullTab()
603+ {
604+ QCOMPARE(model->insert(0, 0), -1);
605+ QCOMPARE(model->rowCount(), 0);
606+ }
607+
608+ void shouldReturnIndexWhenInsertingTab()
609+ {
610+ for(int i = 0; i < 3; ++i) {
611+ model->add(createTab());
612+ }
613+ for(int i = 2; i >= 0; --i) {
614+ QCOMPARE(model->insert(createTab(), i), i);
615+ }
616+ }
617+
618+ void shouldUpdateCountWhenInsertingTab()
619+ {
620+ QSignalSpy spy(model, SIGNAL(countChanged()));
621+ model->insert(createTab(), 0);
622+ QCOMPARE(spy.count(), 1);
623+ QCOMPARE(model->rowCount(), 1);
624+ }
625+
626+ void shouldInsertAtCorrectIndex()
627+ {
628+ model->insert(createTabWithTitle("B"), 0);
629+ model->insert(createTabWithTitle("A"), 0);
630+ verifyTabsOrder(QStringList({"A", "B"}));
631+ model->insert(createTabWithTitle("X"), 1);
632+ verifyTabsOrder(QStringList({"A", "X", "B"}));
633+ model->insert(createTabWithTitle("C"), 3);
634+ verifyTabsOrder(QStringList({"A", "X", "B", "C"}));
635+ }
636+
637+ void shouldClampIndexWhenInsertingTabOutOfBounds()
638+ {
639+ model->add(createTabWithTitle("A"));
640+ model->add(createTabWithTitle("B"));
641+ model->insert(createTabWithTitle("C"), 3);
642+ verifyTabsOrder(QStringList({"A", "B", "C"}));
643+ model->insert(createTabWithTitle("X"), -1);
644+ verifyTabsOrder(QStringList({"X", "A", "B", "C"}));
645+ }
646+
647 void shouldUpdateCountWhenRemovingTab()
648 {
649 model->add(createTab());
650@@ -138,19 +200,6 @@
651 delete removed;
652 }
653
654- void shouldNotChangeCurrentTabWhenAddingUnlessModelWasEmpty()
655- {
656- QSignalSpy spy(model, SIGNAL(currentTabChanged()));
657- QQuickItem* tab = createTab();
658- model->add(tab);
659- QCOMPARE(spy.count(), 1);
660- QCOMPARE(model->currentTab(), tab);
661- spy.clear();
662- model->add(createTab());
663- QVERIFY(spy.isEmpty());
664- QCOMPARE(model->currentTab(), tab);
665- }
666-
667 void shouldNotDeleteTabWhenRemoving()
668 {
669 QQuickItem* tab = createTab();
670@@ -244,64 +293,128 @@
671 QCOMPARE(model->currentTab(), tab2);
672 }
673
674- void shouldUpdateCurrentTabWhenRemoving()
675- {
676- QSignalSpy tabSpy(model, SIGNAL(currentTabChanged()));
677- QSignalSpy indexSpy(model, SIGNAL(currentIndexChanged()));
678-
679- // Adding a tab to an empty model should update the current tab.
680- // Removing the last tab from the model should update it too.
681+ void shouldSetCurrentTabWhenAddingFirstTab()
682+ {
683+ // Adding a tab to an empty model should update the current tab
684+ // to that tab
685+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
686+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
687+ QCOMPARE(model->currentIndex(), -1);
688+ QCOMPARE(model->currentTab(), (QObject*) nullptr);
689+
690+ QQuickItem* tab1 = createTab();
691+ model->add(tab1);
692+
693+ QCOMPARE(spytab.count(), 1);
694+ QCOMPARE(spyindex.count(), 1);
695+ QCOMPARE(model->currentIndex(), 0);
696+ QCOMPARE(model->currentTab(), tab1);
697+
698+ // But adding further items should keep the index where it was
699+ model->add(createTab());
700+ model->add(createTab());
701+
702+ QCOMPARE(spytab.count(), 1);
703+ QCOMPARE(spyindex.count(), 1);
704+ QCOMPARE(model->currentIndex(), 0);
705+ QCOMPARE(model->currentTab(), tab1);
706+ }
707+
708+ void shouldSetCurrentTabWhenInsertingFirstTab()
709+ {
710+ // Inserting a tab to an empty model should update the current tab
711+ // to that tab
712+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
713+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
714+ QCOMPARE(model->currentIndex(), -1);
715+ QCOMPARE(model->currentTab(), (QObject*) nullptr);
716+
717+ QQuickItem* tab1 = createTab();
718+ model->insert(tab1, 0);
719+
720+ QCOMPARE(spytab.count(), 1);
721+ QCOMPARE(spyindex.count(), 1);
722+ QCOMPARE(model->currentIndex(), 0);
723+ QCOMPARE(model->currentTab(), tab1);
724+ }
725+
726+ void shouldSetInvalidIndexWhenRemovingLastTab()
727+ {
728+ // Removing the last item should also set the current index to -1
729+ // and the current tab to null
730 model->add(createTab());
731- tabSpy.clear();
732- indexSpy.clear();
733+
734+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
735+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
736 delete model->remove(0);
737- QCOMPARE(tabSpy.count(), 1);
738- QCOMPARE(indexSpy.count(), 1);
739+ QCOMPARE(spytab.count(), 1);
740+ QCOMPARE(spyindex.count(), 1);
741+ QCOMPARE(model->currentIndex(), -1);
742+ QCOMPARE(model->currentTab(), (QObject*) nullptr);
743+ }
744
745- // When removing a tab after the current one, neither the
746- // current tab nor the current index should change.
747+ void shouldNotChangeIndexWhenRemovingAfterCurrent()
748+ {
749+ // When removing a tab after the current one,
750+ // the current tab shouldn’t change.
751 QQuickItem* tab1 = createTab();
752 model->add(tab1);
753 model->add(createTab());
754- tabSpy.clear();
755- indexSpy.clear();
756+
757+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
758+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
759 delete model->remove(1);
760 QCOMPARE(model->currentTab(), tab1);
761- QVERIFY(tabSpy.isEmpty());
762- QVERIFY(indexSpy.isEmpty());
763+ QVERIFY(spytab.isEmpty());
764+ QVERIFY(spyindex.isEmpty());
765+ }
766
767- // When removing the current tab, if there is a tab after it, it
768- // becomes the current one, and thus the current index doesn’t change.
769+ void shouldUpdateIndexWhenRemovingCurrent()
770+ {
771+ // When removing the current tab, if there is a tab after it,
772+ // it becomes the current one.
773+ QQuickItem* tab1 = createTab();
774 QQuickItem* tab2 = createTab();
775- model->add(tab2);
776- tabSpy.clear();
777- indexSpy.clear();
778- delete model->remove(0);
779- QCOMPARE(tabSpy.count(), 1);
780- QVERIFY(indexSpy.isEmpty());
781- QCOMPARE(model->currentTab(), tab2);
782-
783- // When removing a tab before the current one, the current
784- // tab doesn’t change but the current index is updated.
785 QQuickItem* tab3 = createTab();
786+ model->add(tab1);
787+ model->add(tab2);
788 model->add(tab3);
789 model->setCurrentIndex(1);
790- tabSpy.clear();
791- indexSpy.clear();
792- delete model->remove(0);
793- QVERIFY(tabSpy.isEmpty());
794- QCOMPARE(indexSpy.count(), 1);
795+ QCOMPARE(model->currentIndex(), 1);
796+ QCOMPARE(model->currentTab(), tab2);
797+
798+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
799+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
800+ delete model->remove(1);
801+ QCOMPARE(spyindex.count(), 0);
802+ QCOMPARE(spytab.count(), 1);
803+ QCOMPARE(model->currentTab(), tab3);
804+
805+ // If there is no tab after it but one before, that one becomes current
806+ delete model->remove(1);
807+ QCOMPARE(spyindex.count(), 1);
808+ QCOMPARE(spytab.count(), 2);
809 QCOMPARE(model->currentIndex(), 0);
810-
811- // When removing the current tab, if it was the last one, the
812- // current tab should be reset to 0.
813- tabSpy.clear();
814- indexSpy.clear();
815- delete model->remove(0);
816- QCOMPARE(tabSpy.count(), 1);
817- QCOMPARE(indexSpy.count(), 1);
818- QCOMPARE(model->currentTab(), (QObject*) nullptr);
819- QCOMPARE(model->currentIndex(), -1);
820+ QCOMPARE(model->currentTab(), tab1);
821+ }
822+
823+ void shouldDecreaseIndexWhenRemovingBeforeCurrent()
824+ {
825+ // When removing a tab before the current tab, the current index
826+ // should decrease to match.
827+ model->add(createTab());
828+ model->add(createTab());
829+ QQuickItem* tab = createTab();
830+ model->add(tab);
831+ model->setCurrentIndex(2);
832+
833+ QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
834+ QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
835+ delete model->remove(1);
836+ QCOMPARE(spytab.count(), 1);
837+ QCOMPARE(spyindex.count(), 1);
838+ QCOMPARE(model->currentIndex(), 1);
839+ QCOMPARE(model->currentTab(), tab);
840 }
841
842 void shouldReturnData()

Subscribers

People subscribed via source and target branches

to status/vote changes: