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

Reply via email to