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

[Nick Dedekind]

> +++ 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?

When the structure of the menu changes (for additions/removals from menu), we re-process all the menus so need to re-connect. Just because we remove a menu doesn't mean it's necessarily been deleted.

> 
> 
> +++ 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

Apparently not. No "transfer full".

> 
> + 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...

done all that aren't explicitly commented on above.
-- 
https://code.launchpad.net/~nick-dedekind/qtubuntu/menuTheme/+merge/296997
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu.