[Merge] lp:~nick-dedekind/qtubuntu/menuTheme into lp:qtubuntu

[Gerry Boland]

Review: Needs Fixing

=== added file 'src/ubuntuappmenu/gmenumodelplatformmenu.h'

+    GMenuModelExporter* m_exporter;
+    MenuRegistrar* m_registrar;
If you used QScopedPointer, you would not need to worry about deleting them in a destructor. And then can have a trivial destructor

+class Q_DECL_EXPORT GMenuModelPlatformMenu : public QPlatformMenu
+    GMenuModelExporter* m_exporter;
+    MenuRegistrar* m_registrar;
same as above, but you'll need to set them with reset(). Note you're leaking these currently.



=== added file 'src/ubuntuappmenu/gmenumodelplatformmenu.cpp'
+int logRecusion = 0;
polluting the global namespace, please stuck in an anonymous NS.

+ connect(menu, SIGNAL(structureChanged()), this, SIGNAL(structureChanged()));
new style connect statement gives us compile-time checking of this, please convert. I see this 2 times


for (auto iter = m_menus.begin(); iter != m_menus.end(); ++iter) {
I think there's the odd loop you could convert to Q_FOREACH. menuForTag & menuItemForTag anyway

+QDebug GMenuModelPlatformMenuBar::operator<<(QDebug stream)
+ auto myMenu = qobject_cast<GMenuModelPlatformMenu*>(menu);
static_cast works

+void GMenuModelPlatformMenuBar::setReady(bool _ready)
underscore, yuk! :) Just "ready" please


=== added file 'src/ubuntuappmenu/menuregistrar.cpp'
+bool isMirClient() {
+    return qgetenv("QT_QPA_PLATFORM") == "ubuntumirclient";
+}
move to anonymous NS please, and qGuiApp->platformName() would be a safer thing to check.


+ : m_registeredProcessId(~0)
Putting my security hat on for a second, it is not impossible for an application to have a pid ~0, and also pid 0 (I believe pids can wrap around). Highly unlikely, but an attack vector. Consequences here aren't dire, so I'm ok with it.

std::optional is nice to avoid such munging of set/unset with value.


+ bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
leaked again here. NULLs!

+    if (!bus) {
+ qCWarning(ubuntuappmenuRegistrar, "Failed to retreive session bus - %s", error ? error->message : "unknown error");
error leaked here

+void MenuRegistrar::unregisterApplicationMenu()
+{
+    if (!UbuntuMenuRegistry::instance()->isConnected()) {
+        m_registeredProcessId = ~0;
+        return;
+    }
+    UbuntuMenuRegistry::instance()->unregisterApplicationMenu(m_registeredProcessId, m_path);
+    m_registeredProcessId = ~0;
+}

m_registeredProcessId = ~0; happens nonetheless, an if/else would save the duplicate line.


-- 
https://code.launchpad.net/~nick-dedekind/qtubuntu/menuTheme/+merge/296997
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu.