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
