Cache sub-menus.

Approved by Renato Araujo Oliveira Filho, PS Jenkins bot.

5 files changed, 261 insertions, 15 deletions

diff --git a/.bzrignore b/.bzrignore
index 9038e8d..7c2d57a 100644
--- a/.bzrignore
+++ b/.bzrignore
@@ -16,6 +16,7 @@ coverage/
 
 tests/client/*.moc
 tests/client/actiongrouptest
+tests/client/cachetest
 tests/client/convertertest
 tests/client/menuchangestest
 tests/client/modeltest
diff --git a/libqmenumodel/src/qmenumodel.cpp b/libqmenumodel/src/qmenumodel.cpp
index 53dc966..1326d77 100644
--- a/libqmenumodel/src/qmenumodel.cpp
+++ b/libqmenumodel/src/qmenumodel.cpp
@@ -15,6 +15,7 @@
  *
  * Authors:
  *      Renato Araujo Oliveira Filho <renato@canonical.com>
+ *      Olivier Tilloy <olivier.tilloy@canonical.com>
  */
 
 extern "C" {
@@ -24,8 +25,6 @@ extern "C" {
 #include "qmenumodel.h"
 #include "converter.h"
 
-#include <QDebug>
-
 /*!
     \qmltype QMenuModel
     \brief The QMenuModel class implements the base list model for menus
@@ -41,6 +40,7 @@ QMenuModel::QMenuModel(GMenuModel *other, QObject *parent)
       m_menuModel(0),
       m_signalChangedId(0)
 {
+    m_cache = new QHash<int, QMenuModel*>;
     setMenuModel(other);
 
     connect(this, SIGNAL(rowsInserted(const QModelIndex &, int, int)), SIGNAL(countChanged()));
@@ -52,6 +52,7 @@ QMenuModel::QMenuModel(GMenuModel *other, QObject *parent)
 QMenuModel::~QMenuModel()
 {
     clearModel();
+    delete m_cache;
 }
 
 /*!
@@ -131,10 +132,10 @@ void QMenuModel::clearModel()
         m_menuModel = NULL;
     }
 
-    QList<QMenuModel*> list = findChildren<QMenuModel*>(QString(), Qt::FindDirectChildrenOnly);
-    Q_FOREACH(QMenuModel *model, list) {
-        delete model;
+    Q_FOREACH(QMenuModel* child, *m_cache) {
+        delete child;
     }
+    m_cache->clear();
 }
 
 /*! \internal */
@@ -219,17 +220,25 @@ QVariant QMenuModel::getStringAttribute(const QModelIndex &index,
 QVariant QMenuModel::getLink(const QModelIndex &index,
                              const QString &linkName) const
 {
-    GMenuModel *link;
-
-    link = g_menu_model_get_item_link(m_menuModel,
-                                      index.row(),
-                                      linkName.toUtf8().data());
-
+    GMenuModel *link = g_menu_model_get_item_link(m_menuModel,
+                                                  index.row(),
+                                                  linkName.toUtf8().data());
     if (link) {
-        QMenuModel *other = new QMenuModel(link, const_cast<QMenuModel*>(this));
-        return QVariant::fromValue<QObject*>(other);
+        QMenuModel* child = 0;
+        int key = index.row();
+        if (m_cache->contains(key)) {
+            QMenuModel* cached = m_cache->value(key);
+            if (cached->menuModel() == link) {
+                child = cached;
+            }
+        }
+        if (child == 0) {
+            child = new QMenuModel(link);
+            m_cache->insert(key, child);
+        }
+        g_object_unref(link);
+        return QVariant::fromValue<QObject*>(child);
     }
-
     return QVariant();
 }
 
@@ -265,21 +274,47 @@ QVariant QMenuModel::getExtraProperties(const QModelIndex &index) const
 }
 
 /*! \internal */
-void QMenuModel::onItemsChanged(GMenuModel *,
+QHash<int, QMenuModel*> QMenuModel::cache() const
+{
+    return *m_cache;
+}
+
+/*! \internal */
+void QMenuModel::onItemsChanged(GMenuModel *model,
                                 gint position,
                                 gint removed,
                                 gint added,
                                 gpointer data)
 {
     QMenuModel *self = reinterpret_cast<QMenuModel*>(data);
+    QHash<int, QMenuModel*>* cache = self->m_cache;
 
+    int prevcount = g_menu_model_get_n_items(model) + removed - added;
     if (removed > 0) {
         self->beginRemoveRows(QModelIndex(), position, position + removed - 1);
+        // Remove invalidated menus from the cache
+        for (int i = position, iMax = position + removed; i < iMax; ++i) {
+            if (cache->contains(i)) {
+                delete cache->take(i);
+            }
+        }
+        // Update the indexes of other cached menus to account for the removals
+        for (int i = position + removed; i < prevcount; ++i) {
+            if (cache->contains(i)) {
+                cache->insert(i - removed, cache->take(i));
+            }
+        }
         self->endRemoveRows();
     }
 
     if (added > 0) {
         self->beginInsertRows(QModelIndex(), position, position + added - 1);
+        // Update the indexes of cached menus to account for the insertions
+        for (int i = prevcount - removed - 1; i >= position; --i) {
+            if (cache->contains(i)) {
+                cache->insert(i + added, cache->take(i));
+            }
+        }
         self->endInsertRows();
     }
 }
diff --git a/libqmenumodel/src/qmenumodel.h b/libqmenumodel/src/qmenumodel.h
index 9371bd8..54a5c42 100644
--- a/libqmenumodel/src/qmenumodel.h
+++ b/libqmenumodel/src/qmenumodel.h
@@ -60,7 +60,11 @@ protected:
     void setMenuModel(GMenuModel *model);
     GMenuModel *menuModel() const;
 
+    // helper getter intended for use in tests only
+    QHash<int, QMenuModel*> cache() const;
+
 private:
+    QHash<int, QMenuModel*>* m_cache;
     GMenuModel *m_menuModel;
     guint m_signalChangedId;
 
diff --git a/tests/client/CMakeLists.txt b/tests/client/CMakeLists.txt
index 392437c..0fcac9e 100644
--- a/tests/client/CMakeLists.txt
+++ b/tests/client/CMakeLists.txt
@@ -58,6 +58,7 @@ declare_test(modeltest)
 declare_test(actiongrouptest)

 declare_test(qmltest)

 declare_simple_test(convertertest)

+declare_simple_test(cachetest)

 

 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/qmlfiles.h.in

                ${CMAKE_CURRENT_BINARY_DIR}/qmlfiles.h)

diff --git a/tests/client/cachetest.cpp b/tests/client/cachetest.cpp
new file mode 100644
index 0000000..22fe5d3
--- /dev/null
+++ b/tests/client/cachetest.cpp
@@ -0,0 +1,205 @@
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser 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 Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *      Renato Araujo Oliveira Filho <renato@canonical.com>
+ *      Olivier Tilloy <olivier.tilloy@canonical.com>
+ */
+
+#include "qmenumodel.h"
+
+extern "C" {
+#include <gio/gio.h>
+}
+
+#include <QtTest>
+
+
+class TestModel : public QMenuModel
+{
+    Q_OBJECT
+public:
+    TestModel() : QMenuModel(0)
+    {
+        GMenu *menu3 = g_menu_new();
+        g_menu_append(menu3, "menu4", NULL);
+        g_menu_append(menu3, "menu5", NULL);
+        g_menu_append(menu3, "menu6", NULL);
+
+        GMenu *menu = g_menu_new();
+        g_menu_append(menu, "menu0", NULL);
+        g_menu_append(menu, "menu1", NULL);
+        g_menu_append(menu, "menu2", NULL);
+        g_menu_append_section(menu, "menu3", G_MENU_MODEL(menu3));
+
+        setMenuModel(G_MENU_MODEL(menu));
+
+        m_menus << menu << menu3;
+    }
+
+    void removeItem(int section, int index)
+    {
+        GMenu *menu = m_menus[section];
+        g_menu_remove(menu, index);
+    }
+
+    void insertItem(int section, int index, const QString &label)
+    {
+        GMenu *menu = m_menus[section];
+        g_menu_insert(menu, index, label.toUtf8().data(), NULL);
+    }
+
+    QList<int> cacheIndexes() const
+    {
+        QList<int> indexes = cache().keys();
+        qSort(indexes);
+        return indexes;
+    }
+
+private:
+    QList<GMenu*> m_menus;
+};
+
+class CacheTest : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void initTestCase()
+    {
+        g_type_init();
+    }
+
+    // Verify that normal menu items are not cached (only sub-menus are)
+    void testCacheContents()
+    {
+        TestModel menu;
+        QVERIFY(menu.cacheIndexes().isEmpty());
+
+        menu.data(menu.index(1), QMenuModel::Label);
+        QVERIFY(menu.cacheIndexes().isEmpty());
+
+        menu.data(menu.index(2), QMenuModel::Action);
+        QVERIFY(menu.cacheIndexes().isEmpty());
+    }
+
+    // Verify that the link attribute always returns the same cached menu
+    void testStaticMenuCache()
+    {
+        TestModel menu;
+
+        QModelIndex index = menu.index(3);
+
+        QVariant data = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        QVariant data2 = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        QVERIFY(data.value<QObject*>() == data2.value<QObject*>());
+
+        QMenuModel *section = qvariant_cast<QMenuModel*>(data);
+
+        index = section->index(1);
+        data = menu.data(index, QMenuModel::LinkSection);
+        data2 = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+        QVERIFY(data.value<QObject*>() == data2.value<QObject*>());
+    }
+
+    // Verify that the cache is correctly updated after inserting a new item
+    void testInsertItems()
+    {
+        TestModel menu;
+
+        QModelIndex index = menu.index(3);
+        QVariant data = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        menu.insertItem(0, 4, "newMenu");
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        menu.insertItem(0, 1, "newMenu");
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 4);
+
+        index = menu.index(4);
+        QVariant data2 = menu.data(index, QMenuModel::LinkSection);
+
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 4);
+        QVERIFY(data.value<QObject*>() == data2.value<QObject*>());
+    }
+
+    // Verify that the cache is correctly updated after removing an item that wasn’t cached
+    void testRemoveNonCachedItem()
+    {
+        TestModel menu;
+
+        QModelIndex index = menu.index(3);
+        QVariant data = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        menu.removeItem(0, 1);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 2);
+
+        index = menu.index(2);
+        QVariant data2 = menu.data(index, QMenuModel::LinkSection);
+
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 2);
+        QVERIFY(data.value<QObject*>() == data2.value<QObject*>());
+    }
+
+    // Verify that the cache is correctly updated after removing a cached item
+    void testRemoveCachedItem()
+    {
+        TestModel menu;
+
+        QModelIndex index = menu.index(3);
+        QVariant data = menu.data(index, QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        menu.removeItem(0, 3);
+        QVERIFY(menu.cacheIndexes().isEmpty());
+    }
+
+    // Verify that the cache is correctly updated after multiple insertions and removals 
+    void testMultiplesUpdates()
+    {
+        TestModel menu;
+        QVERIFY(menu.cacheIndexes().isEmpty());
+
+        menu.data(menu.index(3), QMenuModel::LinkSection);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 3);
+
+        menu.insertItem(0, 1, "newMenu");
+        menu.insertItem(0, 2, "newMenu");
+        menu.insertItem(0, 6, "newMenu");
+        menu.insertItem(0, 3, "newMenu");
+        menu.insertItem(0, 7, "newMenu");
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 6);
+
+        menu.removeItem(0, 4);
+        menu.removeItem(0, 6);
+        menu.removeItem(0, 2);
+        QCOMPARE(menu.cacheIndexes(), QList<int>() << 4);
+
+        menu.removeItem(0, 4);
+        QVERIFY(menu.cacheIndexes().isEmpty());
+    }
+};
+
+QTEST_MAIN(CacheTest)
+
+#include "cachetest.moc"
+