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

[Nick Dedekind]

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

C++17 only.

> 
> + 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_registeredPro
> cessId, m_path);
> +    m_registeredProcessId = ~0;
> +}
> 
> m_registeredProcessId = ~0; happens nonetheless, an if/else would save the
> duplicate line.

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