Code review comment for lp:~nick-dedekind/qtubuntu/menuTheme

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