Merge lp:~aacid/unity8/select_first_submenu_when_it_appears into lp:unity8

[Albert Astals Cid]

Fix real world submenus (e.g. kate) not getting their first item selected on open

MenuBar.qml: remove unused function
MenuPopup.qml: Select first item when one arrives if there's no currentItem instead of on completion
unitymenumodel.cpp: trick rowCount() to simulate "real world late" menus
tst_MenuPopup.qml: adapt to unitymenumodel.cpp changes (and make the Up test actually press Up and not Down)

* Are there any related MPs required for this MP to build/function as expected?
https://code.launchpad.net/~aacid/qmenumodel/batch_insert_remove_from_menu

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

 * If you changed the UI, has there been a design review?
N/A

[Michael Zanetti (mzanetti)]

It does improve the situation but there is something odd:

again, using kate as an example, opening the Tools menu, navigate down to a submenu, enter it, navigate down to the third entry, press left, go to another submenu, enter it. Now the highlight will be at the third entry instead of starting at the first again.

also I managed to get into the situation where I lost keyboard focus altogether in the menu by navigating through the main menu entries. While I'm not 100% sure that is introduced by this branch, I haven't seen it in trunk so far.

review: Needs Fixing

[Unity8 CI Bot (unity8-ci-bot)]

PASSED: Continuous integration, rev:2835
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3177/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4172
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/2456
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=zesty,testname=qmluitests.sh/2456
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4200
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4037
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4037/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/3177/rebuild

review: Approve (continuous-integration)

[lp:~aacid/unity8/select_first_submenu_when_it_appears]

2836. By Albert Astals Cid on 2017-03-01

Merge menutool branch

2837. By Albert Astals Cid on 2017-03-01

We want to select the first enabled, not necessarily 0

2838. By Albert Astals Cid on 2017-03-01

this is a bit clearer

2839. By Albert Astals Cid on 2017-03-01

Test++

2840. By Albert Astals Cid on 2017-03-02

only submenus select first, not first level menus

2841. By Albert Astals Cid on 2017-03-13

Merge

Unmerged revisions

2834. By Launchpad Translations on behalf of unity-team on 2017-02-22

Launchpad automatic translations update.

=== modified file 'qml/ApplicationMenus/MenuBar.qml'
--- qml/ApplicationMenus/MenuBar.qml	2017-02-16 13:43:18 +0000
+++ qml/ApplicationMenus/MenuBar.qml	2017-03-01 14:27:32 +0000
@@ -36,10 +36,6 @@
     implicitWidth: row.width
     height: parent.height
 
-    function select(index) {
-        d.select(index);
-    }
-
     function dismiss() {
         d.dismissAll();
     }
 
=== modified file 'qml/ApplicationMenus/MenuPopup.qml'
--- qml/ApplicationMenus/MenuPopup.qml	2017-01-31 15:24:06 +0000
+++ qml/ApplicationMenus/MenuPopup.qml	2017-03-01 14:27:32 +0000
@@ -73,8 +73,8 @@
         d.currentItem = null;
     }
 
-    function select(index) {
-        d.select(index)
+    function selectFirstIndex() {
+        d.selectNext(-1);
     }
 
     function reset() {
@@ -285,6 +285,12 @@
                     Repeater {
                         id: repeater
 
+                        onCountChanged: {
+                            if (!d.currentItem && count > 0) {
+                                root.selectFirstIndex();
+                            }
+                        }
+
                         Loader {
                             id: loader
                             objectName: root.objectName + "-item" + __ownIndex
@@ -483,8 +489,7 @@
                     onChildActivated: childActivated();
                 }
 
-                Component.onCompleted: item.select(0);
-                onVisibleChanged: if (visible) { item.select(0); }
+                onVisibleChanged: if (visible) { item.selectFirstIndex(); }
             }
         }
     }
 
=== modified file 'tests/mocks/QMenuModel/unitymenumodel.cpp'
--- tests/mocks/QMenuModel/unitymenumodel.cpp	2017-01-10 14:46:09 +0000
+++ tests/mocks/QMenuModel/unitymenumodel.cpp	2017-03-01 14:27:32 +0000
@@ -18,6 +18,8 @@
 
 #include "unitymenumodel.h"
 
+#include <QTimer>
+
 enum MenuRoles {
     LabelRole  = Qt::DisplayRole + 1,
     SensitiveRole,
@@ -35,7 +37,8 @@
 };
 
 UnityMenuModel::UnityMenuModel(QObject *parent)
-:   QAbstractListModel(parent)
+ : QAbstractListModel(parent)
+ , m_rowCountStatus(NoRequestMade)
 {
 }
 
@@ -60,7 +63,7 @@
 
 void UnityMenuModel::insertRow(int row, const QVariant& data)
 {
-    row = qMin(row, rowCount());
+    row = qMin(row, m_modelData.count());
 
     beginInsertRows(QModelIndex(), row, row);
 
@@ -71,12 +74,12 @@
 
 void UnityMenuModel::appendRow(const QVariant& data)
 {
-    insertRow(rowCount(), data);
+    insertRow(m_modelData.count(), data);
 }
 
 void UnityMenuModel::removeRow(int row)
 {
-    if (row < 0 || rowCount() <= row) {
+    if (row < 0 || m_modelData.count() <= row) {
         return;
     }
 
@@ -133,6 +136,22 @@
 
 int UnityMenuModel::rowCount(const QModelIndex&) const
 {
+    // Fake the rowCount to be 0 for a while (100ms)
+    // This emulates menus in real world that don't load immediately
+    if (m_rowCountStatus == TimerRunning)
+        return 0;
+
+    if (m_rowCountStatus == NoRequestMade) {
+        UnityMenuModel *that = const_cast<UnityMenuModel*>(this);
+        that->m_rowCountStatus = TimerRunning;
+        QTimer::singleShot(100, that, [that] {
+            that->beginInsertRows(QModelIndex(), 0, that->m_modelData.count() - 1);
+            that->m_rowCountStatus = TimerFinished;
+            that->endInsertRows();
+        });
+        return 0;
+    }
+
     return m_modelData.count();
 }
 
 
=== modified file 'tests/mocks/QMenuModel/unitymenumodel.h'
--- tests/mocks/QMenuModel/unitymenumodel.h	2016-09-29 09:15:04 +0000
+++ tests/mocks/QMenuModel/unitymenumodel.h	2017-03-01 14:27:32 +0000
@@ -101,6 +101,13 @@
     QByteArray m_busName;
     QVariantMap m_actions;
     QByteArray m_menuObjectPath;
+
+    enum RowCountStatus {
+        NoRequestMade,
+        TimerRunning,
+        TimerFinished
+    };
+    RowCountStatus m_rowCountStatus;
 };
 
 #endif // MOCK_UNITYMENUMODEL_H
 
=== modified file 'tests/qmltests/ApplicationMenus/tst_MenuPopup.qml'
--- tests/qmltests/ApplicationMenus/tst_MenuPopup.qml	2017-01-25 14:10:44 +0000
+++ tests/qmltests/ApplicationMenus/tst_MenuPopup.qml	2017-03-01 14:27:32 +0000
@@ -160,9 +160,6 @@
             var priv = findInvisibleChild(menu, "d");
 
             keyClick(Qt.Key_Down, Qt.NoModifier);
-            compare(priv.currentItem, item0, "CurrentItem should have moved to item 0");
-
-            keyClick(Qt.Key_Down, Qt.NoModifier);
             compare(priv.currentItem, item1, "CurrentItem should have moved to item 1");
 
             keyClick(Qt.Key_Down, Qt.NoModifier);
@@ -170,6 +167,9 @@
 
             keyClick(Qt.Key_Down, Qt.NoModifier);
             compare(priv.currentItem, item0, "CurrentItem should have moved to item 0");
+
+            keyClick(Qt.Key_Down, Qt.NoModifier);
+            compare(priv.currentItem, item1, "CurrentItem should have moved to item 1");
         }
 
         function test_keyboardNavigation_UpKeySelectsAndOpensPreviousMenuItemAndRotates() {
@@ -181,17 +181,17 @@
 
             var priv = findInvisibleChild(menu, "d");
 
-            keyClick(Qt.Key_Down, Qt.NoModifier);
-            compare(priv.currentItem, item0, "CurrentItem should have moved to item 2");
+            keyClick(Qt.Key_Up, Qt.NoModifier);
+            compare(priv.currentItem, item2, "CurrentItem should have moved to item 2");
 
-            keyClick(Qt.Key_Down, Qt.NoModifier);
+            keyClick(Qt.Key_Up, Qt.NoModifier);
             compare(priv.currentItem, item1, "CurrentItem should have moved to item 1");
 
-            keyClick(Qt.Key_Down, Qt.NoModifier);
-            compare(priv.currentItem, item2, "CurrentItem should have moved to item 0");
+            keyClick(Qt.Key_Up, Qt.NoModifier);
+            compare(priv.currentItem, item0, "CurrentItem should have moved to item 0");
 
-            keyClick(Qt.Key_Down, Qt.NoModifier);
-            compare(priv.currentItem, item0, "CurrentItem should have moved to item 2");
+            keyClick(Qt.Key_Up, Qt.NoModifier);
+            compare(priv.currentItem, item2, "CurrentItem should have moved to item 2");
         }
 
         function test_keyboardNavigation_RightKeyEntersSubMenu() {
@@ -217,7 +217,7 @@
 
             var menuItem = findChild(menu, "menu-item0"); verify(menuItem);
             mouseClick(menuItem, menuItem.width/2, menuItem.height/2);
-            tryCompareFunction(function() { return menuItem.popup !== null && menuItem.popup.visible }, true);
+            tryCompareFunction(function() { return menuItem.popup !== null && findInvisibleChild(menuItem.popup, "d").currentItem !== null }, true);
 
             keyClick(Qt.Key_Left, Qt.NoModifier);
             tryCompareFunction(function() { return menuItem.popup !== null && menuItem.popup.visible }, false);
@@ -245,13 +245,16 @@
 
             menu.unityMenuModel.modelData = differentSizesMenu;
 
-            waitForRendering(menu);
+            // Wait for the two items to be there
+            tryCompareFunction(function() { return findChild(menu, "menu-item1") !== null; }, true);
             var longWidth = menu.width;
+
+            // Now pop one item and make sure it's smaller
             differentSizesMenu.pop();
             menu.unityMenuModel.modelData = differentSizesMenu;
 
-            waitForRendering(menu);
-            verify(menu.width < longWidth);
+            tryCompareFunction(function() { return findChild(menu, "menu-item0") !== null; }, true);
+            tryCompareFunction(function() { return menu.width < longWidth; }, true);
         }
 
         function test_minimumWidth() {
 
=== modified file 'tools/CMakeLists.txt'
--- tools/CMakeLists.txt	2016-05-09 12:38:10 +0000
+++ tools/CMakeLists.txt	2017-03-01 14:27:32 +0000
@@ -1,14 +1,4 @@
-add_executable(${SCOPE_TOOL}
-    scopetool.cpp
-    registry-tracker.cpp
-    ../src/UnixSignalHandler.cpp
-    )
-
-qt5_use_modules(${SCOPE_TOOL} Qml Quick)
-
-# install binaries
-install(TARGETS ${SCOPE_TOOL}
-    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
-    )
+add_subdirectory(menutool)
+add_subdirectory(scopetool)
 
 install(FILES unlock-device DESTINATION ${SHELL_APP_DIR})
 
=== added directory 'tools/menutool'
=== added file 'tools/menutool/CMakeLists.txt'
--- tools/menutool/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tools/menutool/CMakeLists.txt	2017-03-01 14:27:32 +0000
@@ -0,0 +1,5 @@
+add_executable(menutool
+    menutool.cpp
+    )
+
+qt5_use_modules(menutool Qml Quick)
 
=== added file 'tools/menutool/README'
--- tools/menutool/README	1970-01-01 00:00:00 +0000
+++ tools/menutool/README	2017-03-01 14:27:32 +0000
@@ -0,0 +1,11 @@
+This tool can be used to browse the menus of another application
+
+How to use:
+ * Start application that exports menus
+    e.g. QT_QPA_PLATFORMTHEME=ubuntuappmenu kate
+ * Figure out which is the dbus address
+   * This is a bit tricky, but using qdbusviewer to see the addresses
+     that appear when starting the app and looking for one that
+     contains com/Ubuntu/Menu/0 should be enough
+ * Start tool passing the dbus address
+    builddir/menutool/menutool :1.293
 
=== added file 'tools/menutool/menutool.cpp'
--- tools/menutool/menutool.cpp	1970-01-01 00:00:00 +0000
+++ tools/menutool/menutool.cpp	2017-03-01 14:27:32 +0000
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2017 Canonical, Ltd.
+ *
+ * This program 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.
+ *
+ * This program 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/>.
+ */
+
+// Qt
+#include <QDebug>
+#include <QCommandLineParser>
+#include <QGuiApplication>
+#include <QQmlContext>
+#include <QQmlEngine>
+#include <QQuickView>
+
+// local
+#include <paths.h>
+
+int main(int argc, char *argv[])
+{
+    QGuiApplication::setApplicationName("Menu Test Tool");
+    QGuiApplication *application = new QGuiApplication(argc, argv);
+
+    QCommandLineParser parser;
+    parser.parse(application->arguments());
+    const QStringList args = parser.positionalArguments();
+
+    if (args.count() != 1) {
+        qWarning() << "You need to pass the dbus address";
+        return 1;
+    }
+
+    QQuickView* view = new QQuickView();
+    view->setResizeMode(QQuickView::SizeRootObjectToView);
+    view->setTitle(QGuiApplication::applicationName());
+    view->engine()->setBaseUrl(QUrl::fromLocalFile(::qmlDirectory()));
+    view->rootContext()->setContextProperty("contextBusName", args[0]);
+
+    QUrl source(::sourceDirectory() + "/tools/menutool/menutool.qml");
+    prependImportPaths(view->engine(), ::overrideImportPaths());
+    prependImportPaths(view->engine(), ::nonMirImportPaths());
+
+    view->setSource(source);
+
+    view->show();
+
+    int result = application->exec();
+
+    delete view;
+    delete application;
+
+    return result;
+}
 
=== added file 'tools/menutool/menutool.qml'
--- tools/menutool/menutool.qml	1970-01-01 00:00:00 +0000
+++ tools/menutool/menutool.qml	2017-03-01 14:27:32 +0000
@@ -0,0 +1,103 @@
+/*
+* Copyright (C) 2017 Canonical, Ltd.
+*
+* This program 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.
+*
+* This program 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/>.
+*/
+
+import QtQuick 2.4
+import Ubuntu.Components 1.3
+
+import "../../qml/ApplicationMenus"
+import "../../qml/Panel"
+import "../../qml/Components/PanelState"
+
+import QMenuModel 0.1
+import Unity.Indicators 0.1 as Indicators
+
+Item {
+    width: units.gu(180)
+    height: units.gu(120)
+
+    UnityMenuModel {
+        id: menuModel
+        busName: contextBusName
+        menuObjectPath: "/com/ubuntu/Menu/0"
+        actions: { "unity": "/com/ubuntu/Menu/0" }
+    }
+
+    Panel {
+        id: panel
+
+        height: parent.height
+        width: parent.width / 2
+        minimizedPanelHeight: units.gu(6)
+
+        mode: "windowed"
+
+        applicationMenus {
+            model: menuModel
+        }
+
+        Rectangle {
+            width: 50
+            height: 50
+            anchors.centerIn: parent
+            color: "gray"
+            rotation: 45
+            Timer {
+                interval: 20
+                running: true
+                repeat: true
+                onTriggered: parent.rotation = parent.rotation+1
+            }
+        }
+
+        Rectangle {
+            color: "green"
+            anchors.bottom: parent.bottom
+            anchors.right: parent.right
+            Label {
+                id: label
+                anchors.centerIn: parent
+                text: "Click here to open touch menu manually"
+            }
+            width: label.width + units.gu(2)
+            height: label.height + units.gu(2)
+            MouseArea {
+                anchors.fill: parent
+                onClicked: panel.applicationMenus.show();
+            }
+            visible: !panel.applicationMenus.shown
+        }
+    }
+
+    Rectangle {
+        color: "blue"
+        height: parent.height
+        width: parent.width / 2
+        x: width
+
+        MenuBar {
+            id: menuBar
+            height: units.gu(3)
+            width: parent.width
+            enableKeyFilter: true
+            unityMenuModel: menuModel
+        }
+    }
+
+    Component.onCompleted: {
+        theme.name = "Ubuntu.Components.Themes.SuruDark";
+        PanelState.title = "Drag here to open touch menu";
+    }
+}
 
=== added directory 'tools/scopetool'
=== added file 'tools/scopetool/CMakeLists.txt'
--- tools/scopetool/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tools/scopetool/CMakeLists.txt	2017-03-01 14:27:32 +0000
@@ -0,0 +1,12 @@
+add_executable(${SCOPE_TOOL}
+    scopetool.cpp
+    registry-tracker.cpp
+    ../../src/UnixSignalHandler.cpp
+    )
+
+qt5_use_modules(${SCOPE_TOOL} Qml Quick)
+
+# install binaries
+install(TARGETS ${SCOPE_TOOL}
+    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+    )
 
=== renamed file 'tools/registry-tracker.cpp' => 'tools/scopetool/registry-tracker.cpp'
=== renamed file 'tools/registry-tracker.h' => 'tools/scopetool/registry-tracker.h'
=== renamed file 'tools/scopetool.cpp' => 'tools/scopetool/scopetool.cpp'