Simplify ToggleActionMenu

* Remove the ImplicitDefaultAction intelligence, so ToggleActionMenu is
  not more than a KActionMenu with setDefaultAction().
* Instead, reset the default action when it gets removed from the menu().
  This is done by filtering QActionEvent from menu().
* Add an autotest for ToggleActionMenu.

This replaces prior efforts to fix problems in ToggleActionMenu
in !245 and !254, following the discussion on the virtual meeting
at 2021-02-26.

6b26a2b4b and 1786e6c99 have already ported PageView and
AnnotationActionHandler to the simplified interface.

autotests/CMakeLists.txt

@@ -133,3 +133,8 @@ if(Discount_FOUND)
         LINK_LIBRARIES Qt5::Test okularcore KF5::I18n PkgConfig::Discount
     )
 endif()
+
+ecm_add_test(toggleactionmenutest.cpp ../part/toggleactionmenu.cpp
+    TEST_NAME "toggleactionmenutest"
+    LINK_LIBRARIES Qt5::Test KF5::WidgetsAddons
+)

autotests/toggleactionmenutest.cpp

@@ -0,0 +1,88 @@
+/***************************************************************************
+ *   Copyright (C) 2020 David Hurka <david.hurka@mailbox.org>              *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ ***************************************************************************/
+
+#include <QtTest>
+
+#include "../part/toggleactionmenu.h"
+#include <QToolBar>
+
+class ToggleActionMenuTest : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void testSetDefaultAction();
+    void testDeleteToolBarButton();
+};
+
+void ToggleActionMenuTest::testSetDefaultAction()
+{
+    QToolBar dummyToolBar;
+    ToggleActionMenu menu(QStringLiteral("Menu"), this);
+    QAction *actionA = new QAction(QStringLiteral("A"), this);
+    QAction *actionB = new QAction(QStringLiteral("B"), this);
+
+    // Do not set a default action, the menu should behave as plain KActionMenu.
+    QCOMPARE(menu.defaultAction(), &menu);
+    QToolButton *menuButton = qobject_cast<QToolButton *>(menu.createWidget(&dummyToolBar));
+    QVERIFY(menuButton);
+    QCOMPARE(menuButton->defaultAction(), &menu);
+
+    // Should still behave as plain KActionMenu when actions are added.
+    menu.addAction(actionA);
+    QCOMPARE(menu.defaultAction(), &menu);
+    QCOMPARE(menuButton->defaultAction(), &menu);
+
+    // Set an action from the menu as default action, should work.
+    menu.setDefaultAction(actionA);
+    QCOMPARE(menu.defaultAction(), actionA);
+    QCOMPARE(menuButton->defaultAction(), actionA);
+
+    // Set a foreign action as default action, should reset the default action.
+    menu.setDefaultAction(actionB);
+    QCOMPARE(menu.defaultAction(), &menu);
+    QCOMPARE(menuButton->defaultAction(), &menu);
+
+    // Set an action of the menu as default action, should work.
+    menu.setDefaultAction(actionA);
+    QCOMPARE(menu.defaultAction(), actionA);
+    QCOMPARE(menuButton->defaultAction(), actionA);
+
+    // Remove default action from menu, should reset the default action.
+    menu.removeAction(actionA);
+    QCOMPARE(menu.defaultAction(), &menu);
+    QCOMPARE(menuButton->defaultAction(), &menu);
+}
+
+void ToggleActionMenuTest::testDeleteToolBarButton()
+{
+    QToolBar dummyToolBar;
+    ToggleActionMenu menu(QStringLiteral("Menu"), this);
+    QAction *actionA = new QAction(QStringLiteral("A"), this);
+    QAction *actionB = new QAction(QStringLiteral("B"), this);
+
+    // Setup: set a default action and create two toolbar buttons.
+    menu.addAction(actionA);
+    menu.addAction(actionB);
+    menu.setDefaultAction(actionA);
+    QToolButton *menuButtonA = qobject_cast<QToolButton *>(menu.createWidget(&dummyToolBar));
+    QVERIFY(menuButtonA);
+    QCOMPARE(menuButtonA->defaultAction(), actionA);
+    QToolButton *menuButtonB = qobject_cast<QToolButton *>(menu.createWidget(&dummyToolBar));
+    QVERIFY(menuButtonB);
+
+    // Delete button B, and set a new default action. Button A shall be updated without segfaulting on the deleted button B.
+    delete menuButtonB;
+    menu.setDefaultAction(actionB);
+    QCOMPARE(menuButtonA->defaultAction(), actionB);
+}
+
+QTEST_MAIN(ToggleActionMenuTest)
+
+#include "toggleactionmenutest.moc"

part/toggleactionmenu.cpp

@@ -1,5 +1,5 @@
 /***************************************************************************
- *   Copyright (C) 2019 by David Hurka <david.hurka@mailbox.org>           *
+ *   Copyright (C) 2019-2021 by David Hurka <david.hurka@mailbox.org>      *
  *                                                                         *
  *   Inspired by and replacing toolaction.h by:                            *
  *     Copyright (C) 2004-2006 by Albert Astals Cid <aacid@kde.org>        *
@@ -12,8 +12,8 @@
 
 #include "toggleactionmenu.h"
 
+#include <QActionEvent>
 #include <QMenu>
-#include <QPointer>
 
 ToggleActionMenu::ToggleActionMenu(QObject *parent)
     : ToggleActionMenu(QIcon(), QString(), parent)
@@ -25,43 +25,38 @@ ToggleActionMenu::ToggleActionMenu(const QString &text, QObject *parent)
 {
 }
 
-ToggleActionMenu::ToggleActionMenu(const QIcon &icon, const QString &text, QObject *parent, PopupMode popupMode, MenuLogic logic)
+ToggleActionMenu::ToggleActionMenu(const QIcon &icon, const QString &text, QObject *parent)
     : KActionMenu(icon, text, parent)
     , m_defaultAction(nullptr)
-    , m_suggestedDefaultAction(nullptr)
-    , m_menuLogic(logic)
 {
-    connect(this, &QAction::changed, this, &ToggleActionMenu::updateButtons);
+    slotMenuChanged();
-
-    if (popupMode == DelayedPopup) {
-        setDelayed(true);
-    } else {
-        setDelayed(false);
-    }
-    setStickyMenu(false);
-
-    if (logic & ImplicitDefaultAction) {
-        connect(menu(), &QMenu::triggered, this, &ToggleActionMenu::setDefaultAction);
-    }
 }
 
 QWidget *ToggleActionMenu::createWidget(QWidget *parent)
 {
-    QToolButton *button = qobject_cast<QToolButton *>(KActionMenu::createWidget(parent));
+    QWidget *buttonWidget = KActionMenu::createWidget(parent);
+    QToolButton *button = qobject_cast<QToolButton *>(buttonWidget);
     if (!button) {
         // This function is used to add a button into the toolbar.
         // KActionMenu will plug itself as QToolButton.
         // So, if no QToolButton was returned, this was not called the intended way.
-        return button;
+        Q_ASSERT_X(false,
+                   "ToggleActionMenu::createWidget()",
+                   "Parent implementation KActionMenu::createWidget() did not return a QToolButton, but ToggleActionMenu is designed for QToolButton. Did you call createWidget() manually, with something else than a QToolBar?");
+        return buttonWidget;
     }
 
+    // BEGIN QToolButton hack
+    // Setting the default action of a QToolButton
+    // to an action of its menu() is tricky.
     // Remove this menu action from the button,
     // so it doesn't compose a menu of this menu action and its own menu.
     button->removeAction(this);
     // The button has lost the menu now, let it use the correct menu.
     button->setMenu(menu());
+    // END QToolButton hack
 
-    m_buttons.append(QPointer<QToolButton>(button));
+    m_buttons.append(button);
 
     // Apply other properties to the button.
     updateButtons();
@@ -69,46 +64,28 @@ QWidget *ToggleActionMenu::createWidget(QWidget *parent)
     return button;
 }
 
-void ToggleActionMenu::setDefaultAction(QAction *action)
+QAction *ToggleActionMenu::defaultAction()
-{
-    m_defaultAction = action;
-    updateButtons();
-}
-
-void ToggleActionMenu::suggestDefaultAction(QAction *action)
 {
-    m_suggestedDefaultAction = action;
+    return m_defaultAction ? m_defaultAction : this;
 }
 
-QAction *ToggleActionMenu::checkedAction(QMenu *menu) const
+void ToggleActionMenu::setDefaultAction(QAction *action)
 {
-    // Look at each action a in the menu whether it is checked.
+    if (action && menu()->actions().contains(action)) {
-    // If a is a menu, recursively call checkedAction().
+        m_defaultAction = action;
-    const QList<QAction *> actions = menu->actions();
+    } else {
-    for (QAction *a : actions) {
+        m_defaultAction = nullptr;
-        if (a->isChecked()) {
-            return a;
-        } else if (a->menu()) {
-            QAction *b = checkedAction(a->menu());
-            if (b) {
-                return b;
-            }
-        }
     }
-    return nullptr;
+    updateButtons();
 }
 
 void ToggleActionMenu::updateButtons()
 {
-    for (const QPointer<QToolButton> &button : qAsConst(m_buttons)) {
+    for (QToolButton *button : qAsConst(m_buttons)) {
         if (button) {
-            button->setDefaultAction(defaultAction());
+            button->setDefaultAction(this->defaultAction());
-
-            // Override some properties of the default action,
-            // where the property of this menu makes more sense.
-            button->setEnabled(isEnabled());
 
-            if (delayed()) {
+            if (delayed()) { // TODO deprecated interface.
                 button->setPopupMode(QToolButton::DelayedPopup);
             } else if (stickyMenu()) {
                 button->setPopupMode(QToolButton::InstantPopup);
@@ -119,13 +96,21 @@ void ToggleActionMenu::updateButtons()
     }
 }
 
-QAction *ToggleActionMenu::defaultAction()
+bool ToggleActionMenu::eventFilter(QObject *watched, QEvent *event)
 {
-    if ((m_menuLogic & ImplicitDefaultAction) && !m_defaultAction) {
+    // If the defaultAction() is removed from the menu, reset the default action.
-        m_defaultAction = checkedAction(menu());
+    if (watched == menu() && event->type() == QEvent::ActionRemoved) {
-    }
+        QActionEvent *actionEvent = static_cast<QActionEvent *>(event);
-    if (!m_defaultAction) {
+        if (actionEvent->action() == defaultAction()) {
-        m_defaultAction = m_suggestedDefaultAction;
+            setDefaultAction(nullptr);
+        }
     }
-    return m_defaultAction;
+
+    return KActionMenu::eventFilter(watched, event);
+}
+
+void ToggleActionMenu::slotMenuChanged()
+{
+    menu()->installEventFilter(this);
+    // Not removing old event filter, because we would need to remember the old menu.
 }

part/toggleactionmenu.h

@@ -1,5 +1,5 @@
 /***************************************************************************
- *   Copyright (C) 2019 by David Hurka <david.hurka@mailbox.org>           *
+ *   Copyright (C) 2019-2021 by David Hurka <david.hurka@mailbox.org>      *
  *                                                                         *
  *   Inspired by and replacing toolaction.h by:                            *
  *     Copyright (C) 2004-2006 by Albert Astals Cid <aacid@kde.org>        *
@@ -14,54 +14,36 @@
 #define TOGGLEACTIONMENU_H
 
 #include <KActionMenu>
+#include <QPointer>
 #include <QSet>
 #include <QToolButton>
 
 /**
- * @brief A KActionMenu, with allows to set the default action of its toolbar buttons.
+ * @brief A KActionMenu, which allows to set the default action of its toolbar buttons.
  *
- * Usually, a KActionMenu creates toolbar buttons which reflect its own action properties
+ * This behaves like a KActionMenu, with the addition of setDefaultAction().
- * (icon, text, tooltip, checked state,...), as it is a QAction itself.
- *
- * ToggleActionMenu will use its own action properties only when plugged as submenu in another menu.
- * The default action of the toolbar buttons can easily be changed with the slot setDefaultAction().
- *
- * Naming: The user can *Toggle* the checked state of an *Action* by directly clicking the toolbar button,
- * but can also open a *Menu*.
  *
  * @par Intention
- * Setting the default action of the toolbar button can be useful for:
+ * Setting the default action of toolbar buttons has the advantage that the user
- *  * Providing the most probably needed entry of a menu directly on the menu button.
+ * can trigger a frequently used action directly without opening the menu.
- *  * Showing the last used menu entry on the menu button, including its checked state.
+ * Additionally, the state of the default action is visible in the toolbar.
- * The advantage is that the user often does not need to open the menu,
- * and that the toolbar button shows additional information
- * like checked state or the user's last selection.
  *
- * This shall replace the former ToolAction in Okular,
+ * @par Example
- * while being flexible enough for other (planned) action menus.
+ * You can make the toolbar button show the last used action with only one connection.
+ * You may want to initialize the default action.
+ * \code
+ * if (myToggleActionMenu->defaultAction() == myToggleActionMenu) {
+ *     myToggleActionMenu->setDefaultAction(myFirstAction);
+ * }
+ * connect(myToggleActionMenu->menu(), &QMenu::triggered,
+ *         myToggleActionMenu, &ToggleActionMenu::setDefaultAction);
+ * \endcode
  */
 class ToggleActionMenu : public KActionMenu
 {
     Q_OBJECT
 
 public:
-    /**
-     * Defines how the menu behaves.
-     */
-    enum MenuLogic {
-        DefaultLogic = 0x0,
-        /**
-         * Automatically makes the triggered action the default action, even if in a submenu.
-         * When a toolbar button is constructed,
-         * the default action is set to the default action set with setDefaultAction() before,
-         * otherwise to the first checked action in the menu,
-         * otherwise to the action suggested with suggestDefaultAction().
-         */
-        ImplicitDefaultAction = 0x1
-    };
-
-    enum PopupMode { DelayedPopup, MenuButtonPopup };
-
     explicit ToggleActionMenu(QObject *parent);
     ToggleActionMenu(const QString &text, QObject *parent);
     /**
@@ -70,78 +52,61 @@ public:
      * @param icon The icon of this menu, when plugged into another menu.
      * @param text The name of this menu, when plugged into another menu.
      * @param parent Parent @c QOject.
-     * @param popupMode The popup mode of the toolbar buttons.
-     * You will want to use @c DelayedPopup or @c MenuButtonPopup,
-     * @c InstantPopup would make @c ToggleActionMenu pointless.
-     * @param logic To define special behaviour of @c ToggleActionMenu,
-     * to simplify the usage.
      */
-    ToggleActionMenu(const QIcon &icon, const QString &text, QObject *parent, PopupMode popupMode = MenuButtonPopup, MenuLogic logic = DefaultLogic);
+    ToggleActionMenu(const QIcon &icon, const QString &text, QObject *parent);
 
     QWidget *createWidget(QWidget *parent) override;
 
     /**
      * Returns the current default action of the toolbar buttons.
+     * May be @c this.
      *
-     * In ImplicitDefaultAction mode,
+     * This action is set by setDefaultAction().
-     * when the default action was not yet set with setDefaultAction(),
-     * it will determine it from the first checked action in the menu,
-     * otherwise from the action set with suggestDefaultAction().
      */
     QAction *defaultAction();
 
-    /**
+public Q_SLOTS:
-     * Suggests a default action to be used as fallback.
-     *
-     * It will be used if the default action is not determined another way.
-     * This is useful for ImplicitDefaultAction mode,
-     * when you can not guarantee that one action in the menu
-     * will be checked.
-     *
-     * @note
-     * In DefaultLogic mode, or when you already have called setDefaultAction(),
-     * you have to use setDefaultAction() instead.
-     */
-    void suggestDefaultAction(QAction *action);
-
-public slots:
     /**
      * Sets the default action of the toolbar buttons.
      *
-     * This action will be triggered by clicking directly on the toolbar buttons.
+     * Toolbar buttons are updated immediately.
-     * It will also set the text, icon, checked state, etc. of the toolbar buttons.
      *
-     * @note
+     * Calling setDefaultAction(nullptr) will reset the default action
-     * The default action will not set the enabled state or popup mode of the menu buttons.
+     * to this menu itself.
-     * These properties are still set by the corresponding properties of this ToggleActionMenu.
      *
-     * @warning
+     * @note
-     * The action will not be added to the menu,
+     * @p action must be present in the menu as direct child action.
-     * it usually makes sense to addAction() it before to setDefaultAction() it.
+     * The default action will be reset to this menu itself
+     * when @p action is removed from the menu.
      *
-     * @see suggestDefaultAction()
+     * @note
+     * @p action will define all properties of the toolbar buttons.
+     * When you disable @p action, the toolbar button will become disabled too.
+     * Then the menu can no longer be accessed.
      */
     void setDefaultAction(QAction *action);
 
-private:
+protected:
-    QAction *m_defaultAction;
+    /** Can store @c nullptr, which means this menu itself will be the default action. */
-    QAction *m_suggestedDefaultAction;
+    QPointer<QAction> m_defaultAction;
     QList<QPointer<QToolButton>> m_buttons;
-    MenuLogic m_menuLogic;
 
     /**
-     * Returns the first checked action in @p menu and its submenus,
+     * Updates the toolbar buttons by setting the current defaultAction() on them.
-     * or nullptr if no action is checked.
+     *
+     * (If the current defaultAction() is invalid, `this` is used instead.)
      */
-    QAction *checkedAction(QMenu *menu) const;
+    void updateButtons();
 
-private slots:
     /**
-     * Updates the toolbar buttons, using both the default action and properties of this menu itself.
+     * Updates the event filter, which listens to QMenu’s QActionEvent.
      *
-     * This ensures that the toolbar buttons reflect e. g. a disabled state of this menu.
+     * This is connected to QAction::changed().
+     * That signal is emmited when the menu changes, but that’s not documented.
      */
-    void updateButtons();
+    void slotMenuChanged();
+
+    bool eventFilter(QObject *watched, QEvent *event) override;
 };
 
 #endif // TOGGLEACTIONMENU_H