on xenial help sign in
Fixed CachedUnityMenuModel destruction unhinging indicators. Fixes: 1328646, 1357311
commitcd04516cd7048ebe179e3bd0e948a9e83242c372 authorNick Dedekind <nicholas.dedekind@gmail.com> dateWed 20 Aug 2014, 20:03 (12 years ago) parentcea2bd7098c94f
14 files changed+206−89
plugins/Unity/Indicators/CachedUnityMenuModel.qml−60 plugins/Unity/Indicators/qmldir−3 plugins/Unity/Indicators/unitymenumodelcache.cpp+22−7 plugins/Unity/Indicators/unitymenumodelcache.h+5−3 qml/Greeter/Clock.qml+2−1 tests/mocks/Unity/Indicators/CachedUnityMenuModel.qml → qml/Panel/Indicators/CachedUnityMenuModel.qml+14−5 qml/Panel/Indicators/DefaultIndicatorWidget.qml+2−1 qml/Panel/Indicators/IndicatorBase.qml+4−2 tests/mocks/QMenuModel/unitymenumodel.cpp+4−5 tests/mocks/Unity/Indicators/CMakeLists.txt+6−1 tests/mocks/Unity/Indicators/fakeplugin.cpp+9 tests/mocks/Unity/Indicators/qmldir−1 tests/qmltests/CMakeLists.txt+1 tests/qmltests/Panel/Indicators/tst_CachedUnityMenuModel.qml+137
plugins/Unity/Indicators/CachedUnityMenuModel.qmldeleted−60
@@ -1,60 +0,0 @@
1−/*
2− * Copyright 2013 Canonical Ltd.
3− *
4− * This program is free software; you can redistribute it and/or modify
5− * it under the terms of the GNU Lesser General Public License as published by
6− * the Free Software Foundation; version 3.
7− *
8− * This program is distributed in the hope that it will be useful,
9− * but WITHOUT ANY WARRANTY; without even the implied warranty of
10− * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11− * GNU Lesser General Public License for more details.
12− *
13− * You should have received a copy of the GNU Lesser General Public License
14− * along with this program. If not, see <http://www.gnu.org/licenses/>.
15− *
16− * Authors:
17− * Nick Dedekind <nick.dedekind@canonical.com>
18− */
19−
20−import QtQuick 2.0
21−import QMenuModel 0.1 as QMenuModel
22−import Unity.Indicators 0.1 as Indicators
23−
24−// Make sure we don't duplicate models.
25−Item {
26− id: cachedModel
27− property string busName: ""
28− property string actionsObjectPath: ""
29− property string menuObjectPath: ""
30−
31− readonly property bool ready: busName!=="" && actionsObjectPath!=="" && menuObjectPath!==""
32−
33− property QtObject model: {
34− if (!ready) {
35− return null;
36− }
37−
38− var component = Indicators.UnityMenuModelCache.model(menuObjectPath);
39− if (!component) {
40− component = modelComponent.createObject(cachedModel);
41− return component;
42− }
43− return component;
44− }
45−
46− Component {
47− id: modelComponent
48−
49− QMenuModel.UnityMenuModel {
50− id: unityModel
51− busName: cachedModel.busName
52− actions: { "indicator": cachedModel.actionsObjectPath }
53− menuObjectPath: cachedModel.menuObjectPath
54−
55− Component.onCompleted: {
56− Indicators.UnityMenuModelCache.registerModel(cachedModel.menuObjectPath, unityModel);
57− }
58− }
59− }
60−}
plugins/Unity/Indicators/qmldir−3
@@ -1,6 +1,3 @@
11module Unity.Indicators
22plugin IndicatorsQml
33typeinfo Indicators.qmltypes
4−
5−CachedUnityMenuModel 0.1 CachedUnityMenuModel.qml
6−IndicatorBase 0.1 IndicatorBase.qml
plugins/Unity/Indicators/unitymenumodelcache.cpp+22−7
@@ -29,14 +29,29 @@ UnityMenuModelCache::~UnityMenuModelCache()
2929{
3030}
3131
32−
33−UnityMenuModel* UnityMenuModelCache::model(const QString& path) const
32+UnityMenuModel* UnityMenuModelCache::model(const QByteArray& bus,
33+ const QByteArray& path,
34+ const QVariantMap& actions)
3435{
35− return m_menuModels.value(path, NULL);
36+ if (m_registry.contains(path))
37+ return m_registry[path];
38+
39+ UnityMenuModel* menuModel = new UnityMenuModel;
40+ connect(menuModel, &QObject::destroyed, this, [menuModel, this](QObject*) {
41+ QList<QByteArray> keys = m_registry.keys(menuModel);
42+ Q_FOREACH(const QByteArray& key, keys) {
43+ m_registry.remove(key);
44+ }
45+ });
46+ m_registry[path] = menuModel;
47+
48+ menuModel->setBusName(bus);
49+ menuModel->setMenuObjectPath(path);
50+ menuModel->setActions(actions);
51+ return menuModel;
3652}
3753
38−void UnityMenuModelCache::registerModel(const QString& path, UnityMenuModel* menuModel)
54+bool UnityMenuModelCache::contains(const QByteArray& path)
3955{
40− menuModel->setParent(this);
41− m_menuModels[path] = menuModel;
42−}
56+ return m_registry.contains(path);
57+}
plugins/Unity/Indicators/unitymenumodelcache.h+5−3
@@ -34,11 +34,13 @@ public:
3434 UnityMenuModelCache(QObject*parent=NULL);
3535 ~UnityMenuModelCache();
3636
37− Q_INVOKABLE UnityMenuModel* model(const QString& path) const;
38− Q_INVOKABLE void registerModel(const QString& path, UnityMenuModel* menuModel);
37+ Q_INVOKABLE UnityMenuModel* model(const QByteArray& bus,
38+ const QByteArray& path,
39+ const QVariantMap& actions);
40+ Q_INVOKABLE bool contains(const QByteArray& path);
3941
4042private:
41− QHash<QString, UnityMenuModel*> m_menuModels;
43+ QHash<QByteArray, UnityMenuModel*> m_registry;
4244};
4345
4446#endif // UNITYMENUMODELCACHE_H
qml/Greeter/Clock.qml+2−1
@@ -16,6 +16,7 @@
1616
1717import QtQuick 2.0
1818import Ubuntu.Components 0.1
19+import "../Panel/Indicators"
1920import Unity.Indicators 0.1 as Indicators
2021
2122Item {
@@ -33,7 +34,7 @@ Item {
3334 }
3435 }
3536
36− Indicators.CachedUnityMenuModel {
37+ CachedUnityMenuModel {
3738 id: timeModel
3839 objectName: "timeModel"
3940
tests/mocks/Unity/Indicators/CachedUnityMenuModel.qml → qml/Panel/Indicators/CachedUnityMenuModel.qmlrenamed+14−5
@@ -18,13 +18,22 @@
1818 */
1919
2020import QtQuick 2.0
21+import QMenuModel 0.1
22+import Unity.Indicators 0.1 as Indicators
2123
24+// Make sure we don't duplicate models.
2225Item {
23− property string busName: ""
24− property string actionsObjectPath: ""
25− property string menuObjectPath: ""
26+ id: cachedModel
27+ property string busName
28+ property string actionsObjectPath
29+ property string menuObjectPath
30+ readonly property bool ready: busName!=="" && actionsObjectPath!=="" && menuObjectPath!==""
2631
27− readonly property bool ready: false
32+ property var model: {
33+ if (!ready) return null;
2834
29− property QtObject model: null
35+ return Indicators.UnityMenuModelCache.model(cachedModel.busName,
36+ cachedModel.menuObjectPath,
37+ { "indicator": cachedModel.actionsObjectPath });
38+ }
3039}
qml/Panel/Indicators/DefaultIndicatorWidget.qml+2−1
@@ -63,6 +63,7 @@ IndicatorBase {
6363 }
6464
6565 Row {
66+ id: iconRow
6667 anchors {
6768 top: parent.top
6869 bottom: parent.bottom
@@ -73,7 +74,7 @@ IndicatorBase {
7374
7475 Item {
7576 width: itemImage.width + units.gu(1)
76− anchors { top: parent.top; bottom: parent.bottom }
77+ height: iconRow.height
7778
7879 StatusIcon {
7980 id: itemImage
qml/Panel/Indicators/IndicatorBase.qml+4−2
@@ -34,6 +34,7 @@ Item {
3434 property string deviceMenuObjectPath: menuObjectPath
3535
3636 property alias menuModel: cachedModel.model
37+ property alias rootActionState: rootAction
3738
3839 CachedUnityMenuModel {
3940 id: cachedModel
@@ -42,8 +43,9 @@ Item {
4243 menuObjectPath: indicatorItem.deviceMenuObjectPath
4344 }
4445
45− property RootActionState rootActionState: RootActionState {
46− menu: menuModel
46+ RootActionState {
47+ id: rootAction
48+ menu: menuModel ? menuModel : null
4749 onUpdated: indicatorItem.rootActionStateChanged()
4850 }
4951}
tests/mocks/QMenuModel/unitymenumodel.cpp+4−5
@@ -17,7 +17,6 @@
1717 */
1818
1919#include "unitymenumodel.h"
20−#include <QDebug>
2120
2221enum MenuRoles {
2322 LabelRole = Qt::DisplayRole + 1,
@@ -38,6 +37,10 @@ UnityMenuModel::UnityMenuModel(QObject *parent)
3837{
3938}
4039
40+UnityMenuModel::~UnityMenuModel()
41+{
42+}
43+
4144QVariant UnityMenuModel::modelData() const
4245{
4346 return m_modelData;
@@ -83,10 +86,6 @@ void UnityMenuModel::removeRow(int row)
8386 endRemoveRows();
8487}
8588
86−UnityMenuModel::~UnityMenuModel()
87−{
88−}
89−
9089QByteArray UnityMenuModel::busName() const
9190{
9291 return m_busName;
tests/mocks/Unity/Indicators/CMakeLists.txt+6−1
@@ -14,6 +14,7 @@ set(IndicatorsFakeQml_SOURCES
1414 fakeplugin.cpp
1515 ${CMAKE_SOURCE_DIR}/plugins/Unity/Indicators/indicators.h
1616 ${CMAKE_SOURCE_DIR}/plugins/Unity/Indicators/menucontentactivator.cpp
17+ ${CMAKE_SOURCE_DIR}/plugins/Unity/Indicators/unitymenumodelcache.cpp
1718 ${CMAKE_SOURCE_DIR}/plugins/Unity/Indicators/unitymenumodelstack.cpp
1819 ${CMAKE_SOURCE_DIR}/plugins/Unity/Indicators/visibleindicatorsmodel.cpp
1920 fakeindicatorsmodel.cpp
@@ -33,7 +34,11 @@ add_library(IndicatorsFakeQml SHARED
3334)
3435add_definitions(-DUNITYINDICATORS_LIBRARY)
3536
36−target_link_libraries(IndicatorsFakeQml)
37+add_dependencies(IndicatorsFakeQml QMenuModelQml)
38+target_link_libraries(IndicatorsFakeQml
39+ -L${CMAKE_BINARY_DIR}/tests/mocks/QMenuModel
40+ -lqmenumodel
41+)
3742
3843qt5_use_modules(IndicatorsFakeQml Core Qml Quick)
3944
tests/mocks/Unity/Indicators/fakeplugin.cpp+9
@@ -26,9 +26,16 @@
2626#include "fakeindicatorsmodel.h"
2727#include "indicators.h"
2828#include "menucontentactivator.h"
29+#include "unitymenumodelcache.h"
2930#include "unitymenumodelstack.h"
3031#include "visibleindicatorsmodel.h"
3132
33+static QObject* menuModelCacheSingleton(QQmlEngine* engine, QJSEngine* scriptEngine) {
34+ Q_UNUSED(engine);
35+ Q_UNUSED(scriptEngine);
36+ return new UnityMenuModelCache;
37+}
38+
3239void IndicatorsFakePlugin::registerTypes(const char * uri)
3340{
3441 Q_INIT_RESOURCE(indicators_fake);
@@ -41,6 +48,8 @@ void IndicatorsFakePlugin::registerTypes(const char * uri)
4148 qmlRegisterType<UnityMenuModelStack>(uri, 0, 1, "UnityMenuModelStack");
4249 qmlRegisterType<VisibleIndicatorsModel>(uri, 0, 1, "VisibleIndicatorsModel");
4350
51+ qmlRegisterSingletonType<UnityMenuModelCache>(uri, 0, 1, "UnityMenuModelCache", menuModelCacheSingleton);
52+
4453 // external uncreatables
4554 qmlRegisterUncreatableType<MenuContentState>(uri, 0, 1, "MenuContentState", "Can't create MenuContentState class");
4655 qmlRegisterUncreatableType<ActionState>(uri, 0, 1, "ActionState", "Can't create ActionState class");
tests/mocks/Unity/Indicators/qmldir−1
@@ -2,7 +2,6 @@ module Unity.Indicators
22plugin IndicatorsFakeQml
33typeinfo Indicators.qmltypes
44
5−CachedUnityMenuModel 0.1 CachedUnityMenuModel.qml
65IndicatorsModel 0.1 IndicatorsModel.qml
76FakeMenuPage 0.1 FakeMenuPage.qml
87RootActionState 0.1 RootActionState.qml
tests/qmltests/CMakeLists.txt+1
@@ -72,6 +72,7 @@ add_qml_test(Panel Panel)
7272add_qml_test(Panel SearchIndicator)
7373add_qml_test(Panel/Indicators DefaultIndicatorWidget ENVIRONMENT "LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/tests/mocks/QMenuModel")
7474add_qml_test(Panel/Indicators DefaultIndicatorPage ENVIRONMENT "LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/tests/mocks/QMenuModel")
75+add_qml_test(Panel/Indicators CachedUnityMenuModel ENVIRONMENT "LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/tests/mocks/QMenuModel")
7576# These MenuItemFactory tests need the test/mocks/ to come before plugins/
7677add_qml_test(Panel/Indicators MenuItemFactory IMPORT_PATHS ${CMAKE_BINARY_DIR}/tests/mocks ${qmltest_DEFAULT_IMPORT_PATHS})
7778add_qml_test(Panel/Indicators MessageMenuItemFactory IMPORT_PATHS ${CMAKE_BINARY_DIR}/tests/mocks ${qmltest_DEFAULT_IMPORT_PATHS})
tests/qmltests/Panel/Indicators/tst_CachedUnityMenuModel.qmlnew file+137
@@ -0,0 +1,137 @@
1+/*
2+ * Copyright 2014 Canonical Ltd.
3+ *
4+ * This program is free software; you can redistribute it and/or modify
5+ * it under the terms of the GNU General Public License as published by
6+ * the Free Software Foundation; version 3.
7+ *
8+ * This program is distributed in the hope that it will be useful,
9+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
10+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11+ * GNU General Public License for more details.
12+ *
13+ * You should have received a copy of the GNU General Public License
14+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
15+ */
16+
17+import QtQuick 2.0
18+import QtTest 1.0
19+import Unity.Test 0.1 as UT
20+import QMenuModel 0.1
21+import Unity.Indicators 0.1 as Indicators
22+import "../../../../qml/Panel/Indicators"
23+
24+Item {
25+ id: root
26+ width: units.gu(40)
27+ height: units.gu(70)
28+
29+ Component {
30+ id: model
31+ CachedUnityMenuModel {}
32+ }
33+
34+ UT.UnityTestCase {
35+ name: "CachedUnityMenuModel"
36+ when: windowShown
37+
38+ function cleanup() {
39+ doGC();
40+ }
41+
42+ function doGC() {
43+ // need to put some wait cycles here to get gc going properly.
44+ wait(10);
45+ gc();
46+ wait(10);
47+ }
48+
49+ function test_createDifferent() {
50+ var cachedObject = model.createObject(null,
51+ {
52+ "busName": "com.canonical.test1",
53+ "menuObjectPath": "/com/canonical/test1",
54+ "actionsObjectPath": "/com/canonical/test1"
55+ });
56+
57+ var cachedObject2 = model.createObject(null,
58+ {
59+ "busName": "com.canonical.test2",
60+ "menuObjectPath": "/com/canonical/test2",
61+ "actionsObjectPath": "/com/canonical/test2"
62+ });
63+
64+ verify(cachedObject.model !== cachedObject2.model);
65+ }
66+
67+ function test_createSame() {
68+ var cachedObject = model.createObject(null,
69+ {
70+ "busName": "com.canonical.test3",
71+ "menuObjectPath": "/com/canonical/test3",
72+ "actionsObjectPath": "/com/canonical/test3"
73+ });
74+
75+ var cachedObject2 = model.createObject(null,
76+ {
77+ "busName": "com.canonical.test3",
78+ "menuObjectPath": "/com/canonical/test3",
79+ "actionsObjectPath": "/com/canonical/test3"
80+ });
81+
82+ verify(cachedObject.model === cachedObject2.model);
83+ }
84+
85+ // Tests that changing cached model data does not change the model path of others
86+ function test_lp1328646() {
87+ var cachedObject = model.createObject(null,
88+ {
89+ "busName": "com.canonical.test4",
90+ "menuObjectPath": "/com/canonical/test4",
91+ "actionsObjectPath": "/com/canonical/test4"
92+ });
93+
94+ var cachedObject2 = model.createObject(null,
95+ {
96+ "busName": "com.canonical.test4",
97+ "menuObjectPath": "/com/canonical/test4",
98+ "actionsObjectPath": "/com/canonical/test4"
99+ });
100+
101+ cachedObject.menuObjectPath = "/com/canonical/test5";
102+ compare(cachedObject.model.menuObjectPath, "/com/canonical/test5");
103+ compare(cachedObject2.model.menuObjectPath, "/com/canonical/test4");
104+
105+ verify(cachedObject.model !== cachedObject2.model);
106+ }
107+
108+ function createAndDestroy(test) {
109+ var cachedObject = model.createObject(null,
110+ {
111+ "busName": "com.canonical."+test,
112+ "menuObjectPath": "/com/canonical/"+test,
113+ "actionsObjectPath": "/com/canonical/"+test
114+ });
115+ var cachedObject2 = model.createObject(null,
116+ {
117+ "busName": "com.canonical."+test,
118+ "menuObjectPath": "/com/canonical/"+test,
119+ "actionsObjectPath": "/com/canonical/"+test
120+ });
121+ cachedObject.destroy();
122+ return cachedObject2;
123+ }
124+
125+ function test_destroyAllDeletesModel() {
126+ createAndDestroy("test6");
127+ doGC();
128+ compare(Indicators.UnityMenuModelCache.contains("/com/canonical/test6"), false);
129+ }
130+
131+ function test_destroyPartialKeepsModel() {
132+ var model = createAndDestroy("test7");
133+ doGC();
134+ compare(Indicators.UnityMenuModelCache.contains("/com/canonical/test7"), true);
135+ }
136+ }
137+}