D10461: GMenu-DBusMenu-Proxy

[David Edmundson]

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


  Fundamental design seems fine.
  
  It's a massive patchset, I'm not sure I've got my head round all of it yet, I might take a second round when these minor things are addressed.

INLINE COMMENTS

> actions.cpp:62
> +    QDBusPendingReply<GMenuActionMap> reply = QDBusConnection::sessionBus().asyncCall(msg);
> +    QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(reply, this);
> +    connect(watcher, &QDBusPendingCallWatcher::finished, this, [this](QDBusPendingCallWatcher *watcher) {

leak

> actions.cpp:119
> +    QDBusPendingReply<void> reply = QDBusConnection::sessionBus().asyncCall(msg);
> +    QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(reply, this);
> +    connect(watcher, &QDBusPendingCallWatcher::finished, this, [this, name](QDBusPendingCallWatcher *watcher) {

leak

> gmenudbusmenuproxy.desktop:7
> +OnlyShowIn=KDE;
> +X-KDE-autostart-phase=0

Why 0?

> icons.cpp:24
> +
> +QString Icons::actionIcon(const QString &actionName)
> +{

does this list come from anywhere?

> menu.cpp:98
> +                qCWarning(DBUSMENUPROXY) << "Got an empty menu for" << id << "on" << m_serviceName << "at" << m_objectPath;
> +                return;
> +            }

watcher leaked

> menu.cpp:127
> +    QDBusPendingReply<void> reply = QDBusConnection::sessionBus().asyncCall(msg);
> +    QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(reply, this);
> +    connect(watcher, &QDBusPendingCallWatcher::finished, this, [this, ids](QDBusPendingCallWatcher *watcher) {

and again...

> menuproxy.cpp:195
> +
> +    if (applicationMenuObjectPath.isEmpty() && menuBarObjectPath.isEmpty()) {
> +        return;

are these always set before the window is created?

> menuproxy.cpp:249
> +    // FIXME Check type
> +    if (/*propertyReply->type == 392 && */propertyReply->format == 8 && propertyReply->value_len > 0) {
> +        const char *data = (const char *) xcb_get_property_value(propertyReply.data());

what's this about?

> window.cpp:92
> +    if (!m_applicationObjectPath.isEmpty()) {
> +        m_applicationActions = new Actions(m_serviceName, m_applicationObjectPath);
> +        connect(m_applicationActions, &Actions::actionsChanged, this, [this](const QStringList &dirtyActions) {

who owns this?

> window.cpp:357
> +
> +    new DbusmenuAdaptor(this); // do this before registering the object?
> +

I think it's safe, you'd have definitely added it before we process someone introspecting the path.

But why write it one way round and add a question when we can just move it.

> window.cpp:606
> +        if (!accel.isEmpty()) {
> +            // TODO replace "+" by "plus" and "-" by "minus"
> +            shortcut.append(accel);

so do so?

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, mart, davidedmundson
Cc: davidedmundson, mart, rk, rilian, mtallur, ngraham, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180306/997658a9/attachment-0001.html>