webbrowser-app

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

tab-context-menu
Merge into trunk

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

Medium

Fix Released

Link a bug report

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

lp:~uriboni/webbrowser-app/tab-context-menu updated on 2015-08-17

1135. By Ugo Riboni on 2015-08-17: Merge changes from trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2111/rebuild

review: Needs Fixing (continuous-integration)

lp:~uriboni/webbrowser-app/tab-context-menu updated on 2015-08-18

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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-08-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2125/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-08-31:

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

lp:~uriboni/webbrowser-app/tab-context-menu updated on 2015-09-02

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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-02:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2164/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-02:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2167/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-03:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2168/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-04:

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

review: Needs Fixing

lp:~uriboni/webbrowser-app/tab-context-menu updated on 2015-09-14

1139. By Ugo Riboni on 2015-09-14: Merge changes from trunk

Revision history for this message

Ugo Riboni (uriboni) wrote on 2015-09-14:

> 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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-14:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2231/rebuild

review: Needs Fixing (continuous-integration)

lp:~uriboni/webbrowser-app/tab-context-menu updated on 2015-09-16

1140. By Ugo Riboni on 2015-09-16: Merge changes from trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2241/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-21:

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

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-23:

Superseded by https://code.launchpad.net/~osomon/webbrowser-app/uriboni-tab-context-menu/+merge/272148, which addresses all the comments above.

review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Ubuntu Phablet Team

Ugo Riboni

to status/vote changes:

Rachel Liu

 === modified file 'src/app/webbrowser/Browser.qml'
 --- src/app/webbrowser/Browser.qml	2015-09-13 21:24:56 +0000
 +++ src/app/webbrowser/Browser.qml	2015-09-16 22:49:55 +0000
@@ -353,7 +353,7 @@
                  onRemoved: if (chrome.bookmarked && (url === chrome.webview.url)) chrome.bookmarked = false
+             }
--            onRequestNewTab: browser.openUrlInNewTab("", true)
++            onRequestNewTab: browser.openUrlInNewTab("", makeCurrent, true, index)
              onFindInPageModeChanged: if (!chrome.findInPageMode) internal.resetFocus()
@@ -1221,8 +1221,9 @@
+             }
+         }
--        function addTab(tab, setCurrent) {
--            var index = tabsModel.add(tab)
++        function addTab(tab, setCurrent, index) {
++            if (index === undefined) index = tabsModel.add(tab)
++            else index = tabsModel.insert(tab, index)
              if (setCurrent) {
                  tabsModel.currentIndex = index
                  chrome.requestedUrl = tab.initialUrl
@@ -1325,10 +1326,10 @@
+         }
+     }
--    function openUrlInNewTab(url, setCurrent, load) {
++    function openUrlInNewTab(url, setCurrent, load, index) {
          load = typeof load !== 'undefined' ? load : true
          var tab = tabComponent.createObject(tabContainer, {"initialUrl": url, 'incognito': browser.incognito})
--        internal.addTab(tab, setCurrent)
++        internal.addTab(tab, setCurrent, index)
          if (load) {
              tabsModel.currentTab.load()
+         }
 === modified file 'src/app/webbrowser/Chrome.qml'
 --- src/app/webbrowser/Chrome.qml	2015-08-13 11:11:59 +0000
 +++ src/app/webbrowser/Chrome.qml	2015-09-16 22:49:55 +0000
@@ -39,7 +39,7 @@
      property alias showFaviconInAddressBar: navigationBar.showFaviconInAddressBar
      readonly property alias bookmarkTogglePlaceHolder: navigationBar.bookmarkTogglePlaceHolder
--    signal requestNewTab()
++    signal requestNewTab(int index, bool makeCurrent)
      backgroundColor: incognito ? UbuntuColors.darkGrey : Theme.palette.normal.background
@@ -60,7 +60,7 @@
              sourceComponent: TabsBar {
                  model: tabsModel
                  incognito: chrome.incognito
--                onRequestNewTab: chrome.requestNewTab()
++                onRequestNewTab: chrome.requestNewTab(index, makeCurrent)
+             }
              anchors {
 === modified file 'src/app/webbrowser/TabsBar.qml'
 --- src/app/webbrowser/TabsBar.qml	2015-08-10 15:22:00 +0000
 +++ src/app/webbrowser/TabsBar.qml	2015-09-16 22:49:55 +0000
@@ -18,6 +18,7 @@
  import QtQuick 2.4
  import Ubuntu.Components 1.3
++import Ubuntu.Components.Popups 1.3
  import ".."
  Item {
@@ -31,7 +32,7 @@
      property bool incognito: false
--    signal requestNewTab()
++    signal requestNewTab(int index, bool makeCurrent)
      MouseArea {
          anchors.fill: parent
@@ -67,7 +68,38 @@
              color: incognito ? "white" : UbuntuColors.darkGrey
+         }
--        onClicked: root.requestNewTab()
++        onClicked: root.requestNewTab(root.model.count, true)
++    }
++
++    Component {
++        id: contextualOptionsComponent
++        ActionSelectionPopover {
++            id: menu
++            objectName: "tabContextualActions"
++            property int targetIndex
++
++            actions: ActionList {
++                Action {
++                    objectName: "tab_action_new_tab"
++                    text: i18n.tr("New Tab")
++                    onTriggered: root.requestNewTab(menu.targetIndex + 1, false)
++                }
++                Action {
++                    objectName: "tab_action_reload"
++                    text: i18n.tr("Reload")
++                    enabled: root.model.get(menu.targetIndex).url.toString().length > 0
++                    onTriggered: {
++                        var tab = root.model.get(menu.targetIndex)
++                        if (tab.url.toString().length > 0) tab.webview.reload()
++                    }
++                }
++                Action {
++                    objectName: "tab_action_close_tab"
++                    text: i18n.tr("Close Tab")
++                    onTriggered: internal.closeTab(menu.targetIndex)
++                }
++            }
++        }
+     }
      Item {
@@ -101,7 +133,7 @@
                  MouseArea {
                      id: mouseArea
                      anchors.fill: parent
--                    acceptedButtons: Qt.LeftButton | Qt.MiddleButton
++                    acceptedButtons: Qt.LeftButton | Qt.MiddleButton | Qt.RightButton
                      hoverEnabled: true
                      onPressed: {
                          if (mouse.button === Qt.LeftButton) {
@@ -113,6 +145,13 @@
                              internal.closeTab(index)
+                         }
+                     }
++                    onClicked: {
++                        if (mouse.button === Qt.RightButton) {
++                            var menu = PopupUtils.open(contextualOptionsComponent, tabDelegate)
++                            menu.targetIndex = index
++                        }
++                    }
++
                      // XXX: should not start a drag when middle button was pressed
                      drag {
                          target: tabDelegate
 === modified file 'src/app/webbrowser/tabs-model.cpp'
 --- src/app/webbrowser/tabs-model.cpp	2015-09-02 16:48:01 +0000
 +++ src/app/webbrowser/tabs-model.cpp	2015-09-16 22:49:55 +0000
@@ -87,49 +87,75 @@
      return m_currentIndex;
+ }
++void TabsModel::setCurrentIndexNoCheck(int index)
++{
++  if (index != m_currentIndex) {
++      m_currentIndex = index;
++      Q_EMIT currentIndexChanged();
++      Q_EMIT currentTabChanged();
++  }
++}
++
  void TabsModel::setCurrentIndex(int index)
+ {
--    if (!checkValidTabIndex(index)) {
--        return;
--    }
--    if (index != m_currentIndex) {
--        m_currentIndex = index;
--        Q_EMIT currentIndexChanged();
--        Q_EMIT currentTabChanged();
++    if (checkValidTabIndex(index)) {
++        setCurrentIndexNoCheck(index);
+     }
+ }
  QObject* TabsModel::currentTab() const
+ {
--    if (m_tabs.isEmpty()) {
++    if (m_tabs.isEmpty() || !checkValidTabIndex(m_currentIndex)) {
          return nullptr;
+     }
      return m_tabs.at(m_currentIndex);
+ }
  /*!
--    Add a tab to the model and return the corresponding index in the model.
++    Append a tab to the model and return the corresponding index in the model.
      It is the responsibility of the caller to instantiate the corresponding
      Tab beforehand.
  */
  int TabsModel::add(QObject* tab)
+ {
++    return insert(tab, m_tabs.count());
++}
++
++/*!
++    Add a tab to the model at the specified index, and return the index itself,
++    or -1 if the operation failed.
++
++    It is the responsibility of the caller to instantiate the corresponding
++    Tab beforehand.
++*/
++int TabsModel::insert(QObject* tab, int index)
++{
      if (tab == nullptr) {
          qWarning() << "Invalid Tab";
          return -1;
+     }
--    int index = m_tabs.count();
++    if (index < 0) index == 0;
++    else if (index > m_tabs.count()) index = m_tabs.count();
      beginInsertRows(QModelIndex(), index, index);
--    m_tabs.append(tab);
++    m_tabs.insert(index, tab);
      connect(tab, SIGNAL(urlChanged()), SLOT(onUrlChanged()));
      connect(tab, SIGNAL(titleChanged()), SLOT(onTitleChanged()));
      connect(tab, SIGNAL(iconChanged()), SLOT(onIconChanged()));
      endInsertRows();
      Q_EMIT countChanged();
--    if (index == 0) {
--        setCurrentIndex(0);
++
++    if (m_currentIndex == -1) {
++        // set the index to zero if this is the first item that gets added to the
++        // model, as it should not be possible to have items in the model but no
++        // current tab.
++        setCurrentIndexNoCheck(0);
++    } else if (index <= m_currentIndex) {
++        // increment the index if we are inserting items before the
++        // current index
++        setCurrentIndexNoCheck(m_currentIndex + 1);
+     }
++
      return index;
+ }
@@ -150,15 +176,21 @@
      tab->disconnect(this);
      endRemoveRows();
      Q_EMIT countChanged();
--    if (index == m_currentIndex) {
--        if (!checkValidTabIndex(index)) {
--            m_currentIndex = m_tabs.count() - 1;
--            Q_EMIT currentIndexChanged();
++
++    if (index < m_currentIndex) {
++        // If we removed any tab before the current one, decrease the
++        // currentIndex to match (which will trigger a currentTab change too)
++        setCurrentIndexNoCheck(m_currentIndex - 1);
++    } else if (index == m_currentIndex) {
++        // If we removed the current tab, then select the following one, if any.
++        // This will only trigger a currentTab change since the index remains
++        // the same.
++        // Otherwise select the previous tab, which will trigger both changes.
++        if (m_currentIndex == m_tabs.count()) {
++            setCurrentIndexNoCheck(m_currentIndex - 1);
++        } else {
++            Q_EMIT currentTabChanged();
+         }
--        Q_EMIT currentTabChanged();
--    } else if (m_currentIndex > index) {
--        m_currentIndex -= 1;
--        Q_EMIT currentIndexChanged();
+     }
      return tab;
+ }
 === modified file 'src/app/webbrowser/tabs-model.h'
 --- src/app/webbrowser/tabs-model.h	2015-06-18 14:28:11 +0000
 +++ src/app/webbrowser/tabs-model.h	2015-09-16 22:49:55 +0000
@@ -57,6 +57,7 @@
      QObject* currentTab() const;
      Q_INVOKABLE int add(QObject* tab);
++    Q_INVOKABLE int insert(QObject* tab, int index);
      Q_INVOKABLE QObject* remove(int index);
      Q_INVOKABLE QObject* get(int index) const;
      Q_INVOKABLE void move(int from, int to);
@@ -76,6 +77,7 @@
      int m_currentIndex;
      bool checkValidTabIndex(int index) const;
++    void setCurrentIndexNoCheck(int index);
      void onDataChanged(QObject* tab, int role);
  };
 === added file 'tests/unittests/qml/qml_tree_helpers.js'
 --- tests/unittests/qml/qml_tree_helpers.js	1970-01-01 00:00:00 +0000
 +++ tests/unittests/qml/qml_tree_helpers.js	2015-09-16 22:49:55 +0000
@@ -0,0 +1,43 @@
++/*
++ * Copyright 2015 Canonical Ltd.
++ *
++ * This file is part of webbrowser-app.
++ *
++ * webbrowser-app is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; version 3.
++ *
++ * webbrowser-app is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ */
++
++// Given the naming convention in QML for class names, we should
++// never have a class name with underscores in it, so the following
++// should be a safe way to remove the rest of the extra metatype
++// information produced by converting QML objects to strings.
++function qmlType(item) {
++    var itemType = String(item).split("_")
++    return itemType.length > 0 ? itemType[0] : String(item)
++}
++
++function findChildrenByType(item, type, list) {
++    list = list || []
++    if (qmlType(item) === type) list.push(item)
++    for (var i in item.children) {
++        findChildrenByType(item.children[i], type, list)
++    }
++    return list
++}
++
++function findAncestorByType(item, type) {
++    while (item.parent) {
++        if (qmlType(item.parent) === type) return item.parent
++        item = item.parent
++    }
++    return null
++}
 === modified file 'tests/unittests/qml/tst_TabsBar.qml'
 --- tests/unittests/qml/tst_TabsBar.qml	2015-08-10 15:22:00 +0000
 +++ tests/unittests/qml/tst_TabsBar.qml	2015-09-16 22:49:55 +0000
@@ -21,12 +21,14 @@
  import Ubuntu.Test 1.0
  import "../../../src/app/webbrowser"
  import webbrowserapp.private 0.1
++import "qml_tree_helpers.js" as Tree
  Item {
      id: root
      width: 600
--    height: 50
++    height: 200
++    signal reload(string url)
      TabsModel {
          id: tabsModel
@@ -35,22 +37,37 @@
      Component {
          id: tabComponent
          QtObject {
++            id: tab
              property url url
              property string title
              property url icon
              function close() { destroy() }
++            property QtObject webview: QtObject {
++                function reload() { root.reload(tab.url) }
++            }
+         }
+     }
      TabsBar {
          id: tabs
--        anchors.fill: parent
++
++        // Make the tabs bar smaller than the window and aligned in the middle
++        // to leave room for the context menu to pop up and have all its items
++        // visible within the screen.
++        anchors.left: parent.left
++        anchors.right: parent.right
++        anchors.verticalCenter: parent.verticalCenter
++        height: 50
++
          model: tabsModel
--        onRequestNewTab: appendTab("", "", "")
++        onRequestNewTab: insertTab("", "", "", index)
          function appendTab(url, title, icon) {
++            insertTab(url, title, icon, model.count)
++            model.currentIndex = model.count - 1
++        }
++        function insertTab(url, title, icon, index) {
              var tab = tabComponent.createObject(root, {"url": url, "title": title, "icon": icon})
--            model.add(tab)
--            model.currentIndex = model.count - 1
++            model.insert(tab, index)
+         }
+     }
@@ -60,13 +77,45 @@
          signalName: "requestNewTab"
+     }
++    SignalSpy {
++        id: reloadSpy
++        target: root
++        signalName: "reload"
++    }
++
++    // Ideally we would get menu items by their objectName, however they are
++    // created dynamically by the ActionSelectionPopover and the objectName
++    // of the source action is not copied to the generated item.
++    // So we first select the source Action by objectName then look for an item
++    // with the same text as the action. This is not ideal but it will work
++    // since we don't have items with the same text.
++    // See this bug report for a request to fix this: http://pad.lv/1205144
++    function getMenuItemForAction(menu, actionName) {
++        actionName = "tab_action_" + actionName
++        var text = ""
++        var actions = menu.actions.actions
++        for (var i = 0; i < actions.length; i++) {
++            if (actions[i].objectName === actionName) {
++                text = actions[i].text
++                break
++            }
++        }
++        if (text === "") return null
++
++        var menuItems = Tree.findChildrenByType(menu, "Label")
++        var matching = menuItems.filter(function(item) { return item.text === text })
++        if (matching.length === 0) return null
++        else return Tree.findAncestorByType(matching[0], "Empty")
++    }
++
      UbuntuTestCase {
          name: "TabsBar"
          when: windowShown
--        function clickItem(item) {
++        function clickItem(item, button) {
++            if (button === undefined) button = Qt.LeftButton
              var center = centerOf(item)
--            mouseClick(item, center.x, center.y)
++            mouseClick(item, center.x, center.y, button)
+         }
          function getTabDelegate(index) {
@@ -80,11 +129,22 @@
              return null
+         }
++        function popupMenuOnTab(index) {
++            var tab = getTabDelegate(index)
++            if (tab) {
++                clickItem(tab, Qt.RightButton)
++                var menu = findChild(root, "tabContextualActions")
++                waitForRendering(menu)
++                return menu
++            } else return null
++        }
++
          function cleanup() {
              while (tabsModel.count > 0) {
                  tabsModel.remove(0).destroy()
+             }
              newTabRequestSpy.clear()
++            reloadSpy.clear()
+         }
          function populateTabs() {
@@ -101,6 +161,9 @@
                  clickItem(newTabButton)
+             }
              compare(newTabRequestSpy.count, 3)
++            compare(newTabRequestSpy.signalArguments[0][0], 0)
++            compare(newTabRequestSpy.signalArguments[1][0], 1)
++            compare(newTabRequestSpy.signalArguments[2][0], 2)
+         }
          function test_mouse_left_click() {
@@ -117,11 +180,19 @@
              populateTabs()
              for (var i = 2; i >= 0; --i) {
                  var tab0 = getTabDelegate(0)
--                mouseClick(tab0, centerOf(tab0).x, centerOf(tab0).y, Qt.MiddleButton)
++                clickItem(tab0, Qt.MiddleButton)
                  compare(tabsModel.count, i)
+             }
+         }
++        function test_mouse_right_click() {
++            // Right click pops up the contextual actions menu
++            populateTabs()
++            var menu = popupMenuOnTab(0)
++            verify(menu)
++            verify(menu.visible)
++        }
++
          function test_mouse_wheel() {
              // Wheel events cycle through open tabs
              populateTabs()
@@ -169,5 +240,63 @@
              tab = getTabDelegate(1)
              dragTab(tab, -tab.width * 2, 0)
+         }
++
++        function test_menu_states_on_new_tab() {
++            populateTabs()
++            var menu = popupMenuOnTab(0)
++            var item = getMenuItemForAction(menu, "new_tab")
++            verify(item.enabled)
++            item = getMenuItemForAction(menu, "reload")
++            verify(!item.enabled)
++            item = getMenuItemForAction(menu, "close_tab")
++            verify(item.enabled)
++        }
++
++        function test_menu_states_on_page() {
++            tabs.appendTab("http://localhost/", "tab", "")
++            var menu = popupMenuOnTab(0)
++            var item = getMenuItemForAction(menu, "new_tab")
++            verify(item.enabled)
++            item = getMenuItemForAction(menu, "reload")
++            verify(item.enabled)
++            item = getMenuItemForAction(menu, "close_tab")
++            verify(item.enabled)
++        }
++
++        function test_context_menu_close() {
++            populateTabs()
++            var menu = popupMenuOnTab(1)
++            var item = getMenuItemForAction(menu, "close_tab")
++            clickItem(item)
++            compare(tabsModel.count, 2)
++            compare(tabsModel.get(0).title, "tab 0")
++            compare(tabsModel.get(1).title, "tab 2")
++        }
++
++        function test_context_menu_reload() {
++            var baseUrl = "http://localhost/"
++            tabs.appendTab(baseUrl + "1", "tab 1", "")
++            tabs.appendTab(baseUrl + "2", "tab 2", "")
++            var menu = popupMenuOnTab(1)
++            var item = getMenuItemForAction(menu, "reload")
++            clickItem(item)
++            compare(reloadSpy.count, 1)
++            compare(reloadSpy.signalArguments[0][0], baseUrl + "2")
++        }
++
++        function test_context_menu_new_tab() {
++            var baseUrl = "http://localhost/"
++            tabs.appendTab(baseUrl + "1", "tab 1", "")
++            tabs.appendTab(baseUrl + "2", "tab 2", "")
++            var menu = popupMenuOnTab(0)
++            var item = getMenuItemForAction(menu, "new_tab")
++            clickItem(item)
++            compare(newTabRequestSpy.count, 1)
++            compare(newTabRequestSpy.signalArguments[0][0], 1)
++            compare(tabsModel.count, 3)
++            compare(tabsModel.get(0).url, baseUrl + "1")
++            compare(tabsModel.get(1).url, "")
++            compare(tabsModel.get(2).url, baseUrl + "2")
++        }
+     }
+ }
 === modified file 'tests/unittests/tabs-model/tst_TabsModelTests.cpp'
 --- tests/unittests/tabs-model/tst_TabsModelTests.cpp	2015-09-02 16:48:01 +0000
 +++ tests/unittests/tabs-model/tst_TabsModelTests.cpp	2015-09-16 22:49:55 +0000
@@ -17,6 +17,7 @@
   */
  // Qt
++#include <QtCore/QStringList>
  #include <QtQml/QQmlComponent>
  #include <QtQml/QQmlEngine>
  #include <QtQml/QQmlProperty>
@@ -47,6 +48,22 @@
          return item;
+     }
++    QQuickItem* createTabWithTitle(const QString& title) {
++        QQuickItem* tab = createTab();
++        tab->setProperty("title", title);
++        return tab;
++    }
++
++    void verifyTabsOrder(QStringList orderedTitles) {
++      QCOMPARE(model->rowCount(), orderedTitles.count());
++
++      int i = 0;
++      Q_FOREACH(QString title, orderedTitles) {
++        QCOMPARE(model->get(i)->property("title").toString(), title);
++        i++;
++      }
++    }
++
  private Q_SLOTS:
      void init()
+     {
@@ -113,6 +130,51 @@
          QCOMPARE(model->rowCount(), 1);
+     }
++    void shouldNotInsertNullTab()
++    {
++        QCOMPARE(model->insert(0, 0), -1);
++        QCOMPARE(model->rowCount(), 0);
++    }
++
++    void shouldReturnIndexWhenInsertingTab()
++    {
++        for(int i = 0; i < 3; ++i) {
++          model->add(createTab());
++        }
++        for(int i = 2; i >= 0; --i) {
++            QCOMPARE(model->insert(createTab(), i), i);
++        }
++    }
++
++    void shouldUpdateCountWhenInsertingTab()
++    {
++        QSignalSpy spy(model, SIGNAL(countChanged()));
++        model->insert(createTab(), 0);
++        QCOMPARE(spy.count(), 1);
++        QCOMPARE(model->rowCount(), 1);
++    }
++
++    void shouldInsertAtCorrectIndex()
++    {
++      model->insert(createTabWithTitle("B"), 0);
++      model->insert(createTabWithTitle("A"), 0);
++      verifyTabsOrder(QStringList({"A", "B"}));
++      model->insert(createTabWithTitle("X"), 1);
++      verifyTabsOrder(QStringList({"A", "X", "B"}));
++      model->insert(createTabWithTitle("C"), 3);
++      verifyTabsOrder(QStringList({"A", "X", "B", "C"}));
++    }
++
++    void shouldClampIndexWhenInsertingTabOutOfBounds()
++    {
++        model->add(createTabWithTitle("A"));
++        model->add(createTabWithTitle("B"));
++        model->insert(createTabWithTitle("C"), 3);
++        verifyTabsOrder(QStringList({"A", "B", "C"}));
++        model->insert(createTabWithTitle("X"), -1);
++        verifyTabsOrder(QStringList({"X", "A", "B", "C"}));
++    }
++
      void shouldUpdateCountWhenRemovingTab()
+     {
          model->add(createTab());
@@ -138,19 +200,6 @@
          delete removed;
+     }
--    void shouldNotChangeCurrentTabWhenAddingUnlessModelWasEmpty()
--    {
--        QSignalSpy spy(model, SIGNAL(currentTabChanged()));
--        QQuickItem* tab = createTab();
--        model->add(tab);
--        QCOMPARE(spy.count(), 1);
--        QCOMPARE(model->currentTab(), tab);
--        spy.clear();
--        model->add(createTab());
--        QVERIFY(spy.isEmpty());
--        QCOMPARE(model->currentTab(), tab);
--    }
--
      void shouldNotDeleteTabWhenRemoving()
+     {
          QQuickItem* tab = createTab();
@@ -244,64 +293,128 @@
          QCOMPARE(model->currentTab(), tab2);
+     }
--    void shouldUpdateCurrentTabWhenRemoving()
--    {
--        QSignalSpy tabSpy(model, SIGNAL(currentTabChanged()));
--        QSignalSpy indexSpy(model, SIGNAL(currentIndexChanged()));
--
--        // Adding a tab to an empty model should update the current tab.
--        // Removing the last tab from the model should update it too.
++    void shouldSetCurrentTabWhenAddingFirstTab()
++    {
++      // Adding a tab to an empty model should update the current tab
++      // to that tab
++      QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++      QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
++      QCOMPARE(model->currentIndex(), -1);
++      QCOMPARE(model->currentTab(), (QObject*) nullptr);
++
++      QQuickItem* tab1 = createTab();
++      model->add(tab1);
++
++      QCOMPARE(spytab.count(), 1);
++      QCOMPARE(spyindex.count(), 1);
++      QCOMPARE(model->currentIndex(), 0);
++      QCOMPARE(model->currentTab(), tab1);
++
++      // But adding further items should keep the index where it was
++      model->add(createTab());
++      model->add(createTab());
++
++      QCOMPARE(spytab.count(), 1);
++      QCOMPARE(spyindex.count(), 1);
++      QCOMPARE(model->currentIndex(), 0);
++      QCOMPARE(model->currentTab(), tab1);
++    }
++
++    void shouldSetCurrentTabWhenInsertingFirstTab()
++    {
++      // Inserting a tab to an empty model should update the current tab
++      // to that tab
++      QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++      QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
++      QCOMPARE(model->currentIndex(), -1);
++      QCOMPARE(model->currentTab(), (QObject*) nullptr);
++
++      QQuickItem* tab1 = createTab();
++      model->insert(tab1, 0);
++
++      QCOMPARE(spytab.count(), 1);
++      QCOMPARE(spyindex.count(), 1);
++      QCOMPARE(model->currentIndex(), 0);
++      QCOMPARE(model->currentTab(), tab1);
++    }
++
++    void shouldSetInvalidIndexWhenRemovingLastTab()
++    {
++        // Removing the last item should also set the current index to -1
++        // and the current tab to null
          model->add(createTab());
--        tabSpy.clear();
--        indexSpy.clear();
++
++        QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++        QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
          delete model->remove(0);
--        QCOMPARE(tabSpy.count(), 1);
--        QCOMPARE(indexSpy.count(), 1);
++        QCOMPARE(spytab.count(), 1);
++        QCOMPARE(spyindex.count(), 1);
++        QCOMPARE(model->currentIndex(), -1);
++        QCOMPARE(model->currentTab(), (QObject*) nullptr);
++    }
--        // When removing a tab after the current one, neither the
--        // current tab nor the current index should change.
++    void shouldNotChangeIndexWhenRemovingAfterCurrent()
++    {
++        // When removing a tab after the current one,
++        // the current tab shouldn’t change.
          QQuickItem* tab1 = createTab();
          model->add(tab1);
          model->add(createTab());
--        tabSpy.clear();
--        indexSpy.clear();
++
++        QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++        QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
          delete model->remove(1);
          QCOMPARE(model->currentTab(), tab1);
--        QVERIFY(tabSpy.isEmpty());
--        QVERIFY(indexSpy.isEmpty());
++        QVERIFY(spytab.isEmpty());
++        QVERIFY(spyindex.isEmpty());
++    }
--        // When removing the current tab, if there is a tab after it, it
--        // becomes the current one, and thus the current index doesn’t change.
++    void shouldUpdateIndexWhenRemovingCurrent()
++    {
++        // When removing the current tab, if there is a tab after it,
++        // it becomes the current one.
++        QQuickItem* tab1 = createTab();
          QQuickItem* tab2 = createTab();
--        model->add(tab2);
--        tabSpy.clear();
--        indexSpy.clear();
--        delete model->remove(0);
--        QCOMPARE(tabSpy.count(), 1);
--        QVERIFY(indexSpy.isEmpty());
--        QCOMPARE(model->currentTab(), tab2);
--
--        // When removing a tab before the current one, the current
--        // tab doesn’t change but the current index is updated.
          QQuickItem* tab3 = createTab();
++        model->add(tab1);
++        model->add(tab2);
          model->add(tab3);
          model->setCurrentIndex(1);
--        tabSpy.clear();
--        indexSpy.clear();
--        delete model->remove(0);
--        QVERIFY(tabSpy.isEmpty());
--        QCOMPARE(indexSpy.count(), 1);
++        QCOMPARE(model->currentIndex(), 1);
++        QCOMPARE(model->currentTab(), tab2);
++
++        QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++        QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
++        delete model->remove(1);
++        QCOMPARE(spyindex.count(), 0);
++        QCOMPARE(spytab.count(), 1);
++        QCOMPARE(model->currentTab(), tab3);
++
++        // If there is no tab after it but one before, that one becomes current
++        delete model->remove(1);
++        QCOMPARE(spyindex.count(), 1);
++        QCOMPARE(spytab.count(), 2);
          QCOMPARE(model->currentIndex(), 0);
--
--        // When removing the current tab, if it was the last one, the
--        // current tab should be reset to 0.
--        tabSpy.clear();
--        indexSpy.clear();
--        delete model->remove(0);
--        QCOMPARE(tabSpy.count(), 1);
--        QCOMPARE(indexSpy.count(), 1);
--        QCOMPARE(model->currentTab(), (QObject*) nullptr);
--        QCOMPARE(model->currentIndex(), -1);
++        QCOMPARE(model->currentTab(), tab1);
++    }
++
++    void shouldDecreaseIndexWhenRemovingBeforeCurrent()
++    {
++        // When removing a tab before the current tab, the current index
++        // should decrease to match.
++        model->add(createTab());
++        model->add(createTab());
++        QQuickItem* tab = createTab();
++        model->add(tab);
++        model->setCurrentIndex(2);
++
++        QSignalSpy spytab(model, SIGNAL(currentTabChanged()));
++        QSignalSpy spyindex(model, SIGNAL(currentIndexChanged()));
++        delete model->remove(1);
++        QCOMPARE(spytab.count(), 1);
++        QCOMPARE(spyindex.count(), 1);
++        QCOMPARE(model->currentIndex(), 1);
++        QCOMPARE(model->currentTab(), tab);
+     }
      void shouldReturnData()