Refactor ActionContainer internals.

Make groups explicit instead of magic integer stuff.
That also makes group ordering explicit instead of magically working
(the way we happened to use it and the way unique id manager is
implemented).

src/plugins/coreplugin/actionmanager/actioncontainer.cpp

@@ -39,6 +39,8 @@
 #include "coreconstants.h"
 #include "uniqueidmanager.h"
 
+#include <utils/qtcassert.h>
+
 #include <QtCore/QDebug>
 #include <QtCore/QTimer>
 #include <QtGui/QAction>
@@ -165,6 +167,9 @@ using namespace Core::Internal;
 ActionContainerPrivate::ActionContainerPrivate(int id)
     : m_onAllDisabledBehavior(Disable), m_id(id), m_updateRequested(false)
 {
+    appendGroup(QLatin1String(Constants::G_DEFAULT_ONE));
+    appendGroup(QLatin1String(Constants::G_DEFAULT_TWO));
+    appendGroup(QLatin1String(Constants::G_DEFAULT_THREE));
     scheduleUpdate();
 }
 
@@ -178,51 +183,92 @@ ActionContainer::OnAllDisabledBehavior ActionContainerPrivate::onAllDisabledBeha
     return m_onAllDisabledBehavior;
 }
 
-void ActionContainerPrivate::appendGroup(const QString &group)
+void ActionContainerPrivate::appendGroup(const QString &groupId)
 {
-    int gid = UniqueIDManager::instance()->uniqueIdentifier(group);
+    m_groups.append(Group(groupId));
-    m_groups << gid;
 }
 
-QAction *ActionContainerPrivate::insertLocation(const QString &group) const
+QList<Group>::const_iterator ActionContainerPrivate::findGroup(const QString &groupId) const
 {
-    int grpid = UniqueIDManager::instance()->uniqueIdentifier(group);
+    QList<Group>::const_iterator it = m_groups.constBegin();
-    int prevKey = 0;
+    while (it != m_groups.constEnd()) {
-    int pos = ((grpid << 16) | 0xFFFF);
+        if (it->id == groupId)
-    return beforeAction(pos, &prevKey);
+            break;
+        ++it;
+    }
+    return it;
 }
 
-void ActionContainerPrivate::addAction(Command *action, const QString &group)
+
+QAction *ActionContainerPrivate::insertLocation(const QString &groupId) const
 {
-    if (!canAddAction(action))
+    QList<Group>::const_iterator it = findGroup(groupId);
+    QTC_ASSERT(it != m_groups.constEnd(), return 0);
+    return insertLocation(it);
+}
+
+QAction *ActionContainerPrivate::insertLocation(QList<Group>::const_iterator group) const
+{
+    if (group == m_groups.constEnd())
+        return 0;
+    ++group;
+    while (group != m_groups.constEnd()) {
+        if (!group->items.isEmpty()) {
+            QObject *item = group->items.first();
+            if (Command *cmd = qobject_cast<Command *>(item)) {
+                return cmd->action();
+            } else if (ActionContainer *container = qobject_cast<ActionContainer *>(item)) {
+                if (container->menu())
+                    return container->menu()->menuAction();
+            }
+            QTC_ASSERT(false, return 0);
+        }
+        ++group;
+    }
+    return 0;
+}
+
+void ActionContainerPrivate::addAction(Command *command, const QString &groupId)
+{
+    if (!canAddAction(command))
         return;
 
-    ActionManagerPrivate *am = ActionManagerPrivate::instance();
+    QString actualGroupId;
-    UniqueIDManager *idmanager = UniqueIDManager::instance();
+    if (groupId.isEmpty())
-    int grpid = idmanager->uniqueIdentifier(Constants::G_DEFAULT_TWO);
+        actualGroupId = QLatin1String(Constants::G_DEFAULT_TWO);
-    if (!group.isEmpty())
+    else
-        grpid = idmanager->uniqueIdentifier(group);
+        actualGroupId = groupId;
-    if (!m_groups.contains(grpid) && !am->defaultGroups().contains(grpid))
+
-        qWarning() << "*** addAction(): Unknown group: " << group;
+    QList<Group>::const_iterator groupIt = findGroup(actualGroupId);
-    int pos = ((grpid << 16) | 0xFFFF);
+    QTC_ASSERT(groupIt != m_groups.constEnd(), return);
-    addActionInternal(action, pos);
+    QAction *beforeAction = insertLocation(groupIt);
+    m_groups[groupIt-m_groups.constBegin()].items.append(command);
+
+    connect(command, SIGNAL(activeStateChanged()), this, SLOT(scheduleUpdate()));
+    insertAction(beforeAction, command->action());
+    scheduleUpdate();
 }
 
-void ActionContainerPrivate::addMenu(ActionContainer *menu, const QString &group)
+void ActionContainerPrivate::addMenu(ActionContainer *menu, const QString &groupId)
 {
-    ActionContainerPrivate *container = static_cast<ActionContainerPrivate *>(menu);
+    ActionContainerPrivate *containerPrivate = static_cast<ActionContainerPrivate *>(menu);
-    if (!container->canBeAddedToMenu())
+    if (!containerPrivate->canBeAddedToMenu())
         return;
+    MenuActionContainer *container = static_cast<MenuActionContainer *>(containerPrivate);
 
-    ActionManagerPrivate *am = ActionManagerPrivate::instance();
+    QString actualGroupId;
-    UniqueIDManager *idmanager = UniqueIDManager::instance();
+    if (groupId.isEmpty())
-    int grpid = idmanager->uniqueIdentifier(Constants::G_DEFAULT_TWO);
+        actualGroupId = QLatin1String(Constants::G_DEFAULT_TWO);
-    if (!group.isEmpty())
+    else
-        grpid = idmanager->uniqueIdentifier(group);
+        actualGroupId = groupId;
-    if (!m_groups.contains(grpid) && !am->defaultGroups().contains(grpid))
+
-        qWarning() << "*** addMenu(): Unknown group: " << group;
+    QList<Group>::const_iterator groupIt = findGroup(actualGroupId);
-    int pos = ((grpid << 16) | 0xFFFF);
+    QTC_ASSERT(groupIt != m_groups.constEnd(), return);
-    addMenuInternal(menu, pos);
+    QAction *beforeAction = insertLocation(groupIt);
+    m_groups[groupIt-m_groups.constBegin()].items.append(menu);
+
+    insertMenu(beforeAction, container->menu());
+    scheduleUpdate();
 }
 
 int ActionContainerPrivate::id() const
@@ -245,79 +291,6 @@ bool ActionContainerPrivate::canAddAction(Command *action) const
     return (action->action() != 0);
 }
 
-void ActionContainerPrivate::addActionInternal(Command *action, int pos)
-{
-    Action *a = static_cast<Action *>(action);
-
-    int prevKey = 0;
-    QAction *ba = beforeAction(pos, &prevKey);
-    pos = calcPosition(pos, prevKey);
-
-    m_commands.append(action);
-    m_posmap.insert(pos, action->id());
-    connect(action, SIGNAL(activeStateChanged()), this, SLOT(scheduleUpdate()));
-    insertAction(ba, a->action());
-    scheduleUpdate();
-}
-
-void ActionContainerPrivate::addMenuInternal(ActionContainer *menu, int pos)
-{
-    MenuActionContainer *mc = static_cast<MenuActionContainer *>(menu);
-
-    int prevKey = 0;
-    QAction *ba = beforeAction(pos, &prevKey);
-    pos = calcPosition(pos, prevKey);
-
-    m_subContainers.append(menu);
-    m_posmap.insert(pos, menu->id());
-    insertMenu(ba, mc->menu());
-    scheduleUpdate();
-}
-
-QAction *ActionContainerPrivate::beforeAction(int pos, int *prevKey) const
-{
-    ActionManagerPrivate *am = ActionManagerPrivate::instance();
-
-    int baId = -1;
-
-    (*prevKey) = -1;
-
-    QMap<int, int>::const_iterator i = m_posmap.constBegin();
-    while (i != m_posmap.constEnd()) {
-        if (i.key() > pos) {
-            baId = i.value();
-            break;
-        }
-        (*prevKey) = i.key();
-        ++i;
-    }
-
-    if (baId == -1)
-        return 0;
-
-    if (Command *cmd = am->command(baId))
-        return cmd->action();
-    if (ActionContainer *container = am->actionContainer(baId))
-        if (QMenu *menu = container->menu())
-            return menu->menuAction();
-
-    return 0;
-}
-
-int ActionContainerPrivate::calcPosition(int pos, int prevKey) const
-{
-    int grp = (pos & 0xFFFF0000);
-    if (prevKey == -1)
-        return grp;
-
-    int prevgrp = (prevKey & 0xFFFF0000);
-
-    if (grp != prevgrp)
-        return grp;
-
-    return grp + (prevKey & 0xFFFF) + 10;
-}
-
 void ActionContainerPrivate::scheduleUpdate()
 {
     if (m_updateRequested)
@@ -378,25 +351,32 @@ bool MenuActionContainer::updateInternal()
     bool hasitems = false;
     QList<QAction *> actions = m_menu->actions();
 
-    foreach (ActionContainer *container, subContainers()) {
+    QListIterator<Group> it(m_groups);
-        actions.removeAll(container->menu()->menuAction());
+    while (it.hasNext()) {
-        if (container == this) {
+        const Group &group = it.next();
-            qWarning() << Q_FUNC_INFO << "container" << (this->menu() ? this->menu()->title() : "") <<  "contains itself as subcontainer";
+        foreach (QObject *item, group.items) {
-            continue;
+            if (ActionContainerPrivate *container = qobject_cast<ActionContainerPrivate*>(item)) {
-        }
+                actions.removeAll(container->menu()->menuAction());
-        if (qobject_cast<ActionContainerPrivate*>(container)->updateInternal()) {
+                if (container == this) {
-            hasitems = true;
+                    qWarning() << Q_FUNC_INFO << "container" << (this->menu() ? this->menu()->title() : "") <<  "contains itself as subcontainer";
-            break;
+                    continue;
-        }
+                }
-    }
+                if (container->updateInternal()) {
-    if (!hasitems) {
+                    hasitems = true;
-        foreach (Command *command, commands()) {
+                    break;
-            actions.removeAll(command->action());
+                }
-            if (command->isActive()) {
+            } else if (Command *command = qobject_cast<Command *>(item)) {
-                hasitems = true;
+                actions.removeAll(command->action());
-                break;
+                if (command->isActive()) {
+                    hasitems = true;
+                    break;
+                }
+            } else {
+                QTC_ASSERT(false, continue);
             }
         }
+        if (hasitems)
+            break;
     }
     if (!hasitems) {
         // look if there were actions added that we don't control and check if they are enabled

src/plugins/coreplugin/actionmanager/actioncontainer_p.h

@@ -42,6 +42,12 @@
 namespace Core {
 namespace Internal {
 
+struct Group {
+    Group(const QString &id) : id(id) {}
+    QString id;
+    QList<QObject *> items; // Command * or ActionContainer *
+};
+
 class ActionContainerPrivate : public Core::ActionContainer
 {
     Q_OBJECT
@@ -54,7 +60,7 @@ public:
     ActionContainer::OnAllDisabledBehavior onAllDisabledBehavior() const;
 
     QAction *insertLocation(const QString &group) const;
-    void appendGroup(const QString &group);
+    void appendGroup(const QString &id);
     void addAction(Command *action, const QString &group = QString());
     void addMenu(ActionContainer *menu, const QString &group = QString());
 
@@ -66,9 +72,6 @@ public:
     virtual void insertAction(QAction *before, QAction *action) = 0;
     virtual void insertMenu(QAction *before, QMenu *menu) = 0;
 
-    QList<Command *> commands() const { return m_commands; }
-    QList<ActionContainer *> subContainers() const { return m_subContainers; }
-
     virtual bool updateInternal() = 0;
 
 protected:
@@ -76,23 +79,19 @@ protected:
     bool canAddMenu(ActionContainer *menu) const;
     virtual bool canBeAddedToMenu() const = 0;
 
-    void addActionInternal(Command *action, int pos);
+    // groupId --> list of Command* and ActionContainer*
-    void addMenuInternal(ActionContainer *menu, int pos);
+    QList<Group> m_groups;
 
 private slots:
     void scheduleUpdate();
     void update();
 
 private:
-    QAction *beforeAction(int pos, int *prevKey) const;
+    QList<Group>::const_iterator findGroup(const QString &groupId) const;
-    int calcPosition(int pos, int prevKey) const;
+    QAction *insertLocation(QList<Group>::const_iterator group) const;
 
-    QList<int> m_groups;
     OnAllDisabledBehavior m_onAllDisabledBehavior;
     int m_id;
-    QMap<int, int> m_posmap;
-    QList<ActionContainer *> m_subContainers;
-    QList<Command *> m_commands;
     bool m_updateRequested;
 };
 

src/plugins/coreplugin/actionmanager/actionmanager.cpp

@@ -242,12 +242,7 @@ ActionManagerPrivate::ActionManagerPrivate(MainWindow *mainWnd)
   : ActionManager(mainWnd),
     m_mainWnd(mainWnd)
 {
-    UniqueIDManager *uidmgr = UniqueIDManager::instance();
-    m_defaultGroups << uidmgr->uniqueIdentifier(Constants::G_DEFAULT_ONE);
-    m_defaultGroups << uidmgr->uniqueIdentifier(Constants::G_DEFAULT_TWO);
-    m_defaultGroups << uidmgr->uniqueIdentifier(Constants::G_DEFAULT_THREE);
     m_instance = this;
-
 }
 
 ActionManagerPrivate::~ActionManagerPrivate()
@@ -261,11 +256,6 @@ ActionManagerPrivate *ActionManagerPrivate::instance()
     return m_instance;
 }
 
-QList<int> ActionManagerPrivate::defaultGroups() const
-{
-    return m_defaultGroups;
-}
-
 QList<Command *> ActionManagerPrivate::commands() const
 {
     // transform list of CommandPrivate into list of Command

src/plugins/coreplugin/actionmanager/actionmanager_p.h

@@ -69,7 +69,6 @@ public:
     static ActionManagerPrivate *instance();
 
     void saveSettings(QSettings *settings);
-    QList<int> defaultGroups() const;
 
     QList<Command *> commands() const;
 
@@ -98,7 +97,6 @@ private:
     Action *overridableAction(const Id &id);
 
     static ActionManagerPrivate *m_instance;
-    QList<int> m_defaultGroups;
 
     typedef QHash<int, CommandPrivate *> IdCmdMap;
     IdCmdMap m_idCmdMap;