[Differential] [Commented On] D4057: Reuse QAction and QMenu items on updates

[Mark Gaiser]

markg added a comment.


  Hi David,
  
  You have an interesting approach here!
  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.
  I ended up doing the bookkeeping in QAction itself.
  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.
  
  What you would need to do for this:
  
  1. Get rid of the bookkeeping, you don't need them.
  
    typedef QMap<int, QAction* > ActionForId;
    ActionForId m_actionForId;
  
  
  
  2. The remove action part becomes something like this:
  
  (note: why do you mix foreach and Q_FOREACH? pick one or the other)
  
    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;
            }
        }
    }
  
  
  
  3. The add action becomes something like:
  
    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());
        }
    }
  
  Not tested and guessed some code.. But you get the point i think.
  Also, i seem to be missing where you set DBUSMENU_PROPERTY_ID on an action (or it's done in d->createAction?).
  
  That's more elegant, right?
  
  Good luck :)

REPOSITORY
  R120 Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: markg, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, andreaska, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170109/e018820c/attachment.html>