<table><tr><td style="">markg added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D4057" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Hi David,</p>

<p>You have an interesting approach here!<br />
I've been struggling with something similar recently as well and also - initially - had my own lookup table for int -> QAction. It worked, but the added bookkeeping seemed silly so i searched for a more "elegant" solution.<br />
I ended up doing the bookkeeping in QAction itself.<br />
This is O(n) complexity, not O(1), but it saves having to maintain your own bookkeeping for actions in a menu. That can't ever be so dreadfully big to slow you down so i think you're safe with this approach.</p>

<p>What you would need to do for this:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">Get rid of the bookkeeping, you don't need them.</li>
</ol>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">typedef QMap<int, QAction* > ActionForId;
ActionForId m_actionForId;</pre></div>



<ol class="remarkup-list" start="2">
<li class="remarkup-list-item">The remove action part becomes something like this:</li>
</ol>

<p>(note: why do you mix foreach and Q_FOREACH? pick one or the other)</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">foreach (QAction *action, menu->actions()) {
    int id = action->property(DBUSMENU_PROPERTY_ID).toInt();
    Q_FOREACH(const DBusMenuLayoutItem &dbusMenuItem, rootItem.children) {
        if (dbusMenuItem.id == id) {
            action->deleteLater();
            break;
        }
    }
}</pre></div>



<ol class="remarkup-list" start="3">
<li class="remarkup-list-item">The add action becomes something like:</li>
</ol>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">Q_FOREACH(const DBusMenuLayoutItem &dbusMenuItem, rootItem.children) {

    auto it = std::find_if(d->m_actionForId.cbegin(), d->m_actionForId.cend(), [&dbusMenuItem](QAction *action)
    {
       return action->property(DBUSMENU_PROPERTY_ID).toInt() == dbusMenuItem.id;
    });

    QAction *action = nullptr;
    if (it == d->m_actionForId.end()) {
        int id = dbusMenuItem.id;
        action = d->createAction(id, dbusMenuItem.properties, menu);
        d->m_actionForId.insert(id, action);

        connect(action, &QAction::triggered, this, [action, id, this]() {
            sendClickedEvent(id);
        });

        menu->addAction(action);
    } else {
        action = *it;
        d->updateAction(*it, dbusMenuItem.properties, dbusMenuItem.properties.keys());
    }
}</pre></div>

<p>Not tested and guessed some code.. But you get the point i think.<br />
Also, i seem to be missing where you set DBUSMENU_PROPERTY_ID on an action (or it's done in d->createAction?).</p>

<p>That's more elegant, right?</p>

<p>Good luck :)</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R120 Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4057" rel="noreferrer">https://phabricator.kde.org/D4057</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>davidedmundson, Plasma<br /><strong>Cc: </strong>markg, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, andreaska, sebas<br /></div>