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

[Gerry Boland]

Review: Needs Fixing

+++ src/ubuntuappmenu/gmenumodelexporter.h
+class GMenuModelExporter : public QObject
I was curious if this really needed to be a QObject, but you do use that fact to ensure lifetimes of the objects in signal/lambda connections are correct. So it's ok.

+ QList<QMetaObject::Connection> m_propertyConnections;
I understand why you do this, because QObject::disconnect must have a sender (i.e. first argument) set. 

But GMenuModelPlatformMenuItem are QObjects, so on deletion they will clean up their connections. Is that not enough? Do they clean up too late?


+++ src/ubuntuappmenu/gmenumodelexporter.cpp
+inline QString getActionString(const QString& label) {
opening brace on newline - just for consistency.

+ for(auto iter = parts.begin(); iter != parts.end(); ++iter) {
space after the "for" - could you not use Q_FOREACH(const auto &part, parts) and save messing with iterators?

I'd also appreciate an explanatory comment of what "getActionString" is doing.


+ auto item = (GMenuModelPlatformMenuItem*)user_data;
Use C++ static_cast<> please

+#define MENU_OBJECT_PATH QString("/com/ubuntu/Menu/%1")
QStringLiteral would work. But odd to define more than a character constant here, 
#define MENU_OBJECT_PATH "/com/ubuntu/Menu/%1"
would be more typical


+ GMenuModelBarExporter::GMenuModelBarExporter(GMenuModelPlatformMenuBar * bar)
+      auto iter = bar->menus().begin();
+      for (; iter != bar->menus().end(); ++iter) {
Q_FOREACH nicer, no? You use these style loops everywhere, any reason?

Also, a quick comment explaining the purpose of these classes and each method would help other readers. I'm especially obsessed with documenting methods that return GThing* pointers, to clearly indicate if or how the returned GThings* should be cleaned up by the consumer.


+ GError *error = NULL;
I see lots of NULL here, can we not use nullptr, or is G stuff incompatible with it? /me obsessed with C++11 goodies

+ bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
leak, is never unref-ed. 

+ m_exportedModel = g_dbus_connection_export_menu_model(bus, menuPath.constData(), G_MENU_MODEL(m_gmainMenu), &error);
g_dbus_connection_export_menu_model returns a guint, unsigned int. m_exportedModel is an int. Implicit lossy conversion here, definitely safe?


+ error = NULL;
Leak, you should be doing g_error_free (err)


+ GMenuModelPlatformMenu* gplatformMenu = qobject_cast<GMenuModelPlatformMenu*>(platformMenu);
I see this a bunch of places, it can be simpler:
auto gplatformMenu = static_cast<GMenuModelPlatformMenu*>(platformMenu);


+void GMenuModelExporter::addSubmenuItems(GMenuModelPlatformMenu* gplatformMenu, GMenu* menu)
There's a lot going on here, quick comment explaining the intention would be good. I'm just looking at the code, it appears fine. Q_FOREACH not suitable, but you can use static_cast instead of qobject_cast here. 



+ bool checkable(GMenuModelPlatformMenuItem::get_checkable(gplatformMenuItem));
I prefer = instead of () assignment for simple types - same line could be misinterpreted as a function declaration
same for "checked" inside the conditional.

+ std::function<void(bool)> updateChecked = [gplatformMenuItem, action](bool checked) {
perfect place for an "auto"

+ auto type = g_action_get_state_type(G_ACTION(action));
'type' is a GVariantType*, I suspect it should be deleted somewhere

+ std::function<void(bool)> updateEnabled = [gplatformMenuItem, action](bool enabled) {
another auto



/me hates G code, due to the fact g_object_unref is scattered all around the code, making it is so easy to leak. Have you see this:
http://bazaar.launchpad.net/~unity-2d-team/unity-2d/trunk/view/head:/libunity-2d-private/src/gscopedpointer.h
We used this all the time, it could wrap gobjects in a scoped pointer so they'd be automatically deleted when they go out of scope. I'm not blocking this MP on this point however.


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