D9881: Replace deprecated QSignalMapper by lambda expressions

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  excellent work! some nits, otherwise lgtm

INLINE COMMENTS

> midebuggerplugin.cpp:64
> +public:
> +    DBusProxy(const QString & service, const QString & path, QObject* parent)
> +        : QObject(parent),

style: remove space before &

> midebuggerplugin.cpp:70
> +
> +    QDBusInterface * interface()
> +    {

dito

> midebuggerplugin.cpp:151
>          // New registration
> -        QDBusInterface* drkonqiInterface = new QDBusInterface(service, QStringLiteral("/krashinfo"),
> -                                                              QString(), QDBusConnection::sessionBus(),
> -                                                              this);
> -        m_drkonqis.insert(service, drkonqiInterface);
> -
> -        connect(drkonqiInterface, SIGNAL(acceptDebuggingApplication()), m_drkonqiMap, SLOT(map()));
> -        m_drkonqiMap->setMapping(drkonqiInterface, drkonqiInterface);
> -
> -        drkonqiInterface->call(QStringLiteral("registerDebuggingApplication"), i18n("KDevelop"));
> +        auto drkonqiProxy = new DBusProxy(service, QStringLiteral("/krashinfo"), this);
> +        m_drkonqis.insert(service, drkonqiProxy);

the old code was odd here already, but shouldn't you first check whether the service is already known and a proxy exists?
or at least ensure that its freed by calling `slotDBusServiceUnregistered`

also, add a comment here please saying that we use the proxy to map N services to the one slot that takes the proxy.

Finally, can't we just use old-style connect + `sender()` here to simplify the code? Yes, `sender()` isn't the nicest thing on earth, but it certainly is less code than this here.

> registersview.cpp:244
> +    const QString atext = a->text();
> +    connect(a, &QAction::triggered, this, [this, atext](){ menuTriggered(atext); });
>  }

why not capture a and then call `a->text()` in the lambda?

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D9881

To: croick, #kdevelop, mwolff
Cc: mwolff, apol, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180125/e2d78944/attachment.html>