I did test this today and it seems to be working correctly, at least in the use cases of Okular, i.e. for select annotation tools and for the annotation toolbar. Actually I am currently using in D15580 now (will push soon), given that ToolAction was not enough for what I needed ( I'll update my code accordingly if this revision is modified).

Suggested changes:

• Make QToolButton::MenuButtonPopup, and ToggleActionMenu::ImplicitDefaultAction the defaults in the constructor so we can call it as d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this);
• I do not fully understand the purpose of the method setSuggestedDefaultAction. I would remove it and leave only setDefaultAction.

This is how I am currently using it:

ToggleActionMenu *ta = new ToggleActionMenu( QIcon(),QString(), this, QToolButton::MenuButtonPopup, ToggleActionMenu::ImplicitDefaultAction );
connect(ta, &ToggleActionMenu::toggled, this, &AnnotationActionHandler::setTextToolsEnabled);
ac->addAction( QStringLiteral("annotation_geometrical_shape"), ta );
ta->setText( i18nc( "@action", "Geometrical shapes" ) );
ta->addAction( annArrow );
ta->addAction( annStraightLine );
ta->addAction( annRectangle );
ta->setDefaultAction( annArrow );

I am only considering the use cases of Okular.

@simgunz Thanks for your feedback. I have just got some more private to-do, so if I do not look at your feedback later today, please wait at least until wednesday. :)

In D21971#503218, @simgunz wrote:

Suggested changes:

• Make QToolButton::MenuButtonPopup, and ToggleActionMenu::ImplicitDefaultAction the defaults in the constructor so we can call it as d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this);

That would make the longer way necessary for my own ideas, where I have put “Configure...” into the Color Mode menu.

• I do not fully understand the purpose of the method setSuggestedDefaultAction. I would remove it and leave only setDefaultAction.

I don’t think the ImplicitDefaultAction mode makes sense without it. For the mouse mode menu (as shown in this patch), it is needed, because the default action should be Text Selection, because that mode is most likely needed. But it is possible that Area Selection is already checked, and in that case Area Selection should be shown on the toolbar button.

Or do you mean ImplicitDefaultAction mode should change the behaviour of setDefaultAction(), so it only suggests the default action? I don’t think that’s good, because it will not really “set” then. Additionally, toggleactionmenu.cpp:57 would not work like it is done now.

This is how I am currently using it: [...]

Yeah, in that case I would recommend suggestDefaultAction(), for the case that annStraightLine is already checked.

simgunz added a dependent revision: D15580: [WIP] New annotation toolbar.

Maybe time to look at this and your patch again, maybe next week...

D15580 depends on this code (not this Differential Revision, currently) now. Before landing this, I have to think about checkedAction(). Theoretically, it doesn’t find checked actions in submenus, while the connection to the signal QActionGroup::triggered(QAction*) does.

In D21755#503309, @davidhurka wrote:

In D21755#483631, @simgunz wrote:

What are the other modes in which it could work, besides the one presented in D21971 i.e. as a replacement of ToolAction (basically a cleaner implementation of it).

None so far, except the “default mode” like in D21195.

Nate suggested to cycle through the Geometrical Shapes menu when the 9 key is pressed. This could be another mode, but I don’t know how to generalize the behaviour.

@ngraham wrote:

• It might be nice if hitting the [9] key multiple times cycled through the items in the shape annotation menu

I have tried to analyze all the possible cases regarding ToggleActionMenu placed in a toolbar.

From what I understand:

• DefaultLogic means, the developer needs to manage everything manually calling/connecting setDefaultAction.
• ImplicitDefaultAction instead allows to set the default action automatically when an action is triggered

Case 1

QToolButton::MenuButtonPopup or QToolButton::DelayedPopup and m_menuLogic == DefaultLogic

If I do not call suggestDefaultAction and no action is selected:

1. the ToggleActionMenu icon is shown
2. Clicking on the toolbar button nothing happens [weird, but maybe ok]
3. Checking an action has no effect on the toolbar button
4. The toolbar button is never shown as checked [weird]

If I do call suggestDefaultAction and no action is selected:

1. the toolbar button icon is set to the one of the suggested default action
2. Clicking on the toolbarbutton always activates the suggested default action
3. Checking a different action has no effect on points 1 and 2 [weird]

Case 2

QToolButton::MenuButtonPopup or QToolButton::DelayedPopup and m_menuLogic == ImplicitDefaultAction
If I do call suggestDefaultAction and no action is selected everything works as expected.
If I do not call suggestDefaultAction and no action is selected:

1. the ToggleActionMenu icon is shown
2. Clicking on the toolbar button nothing happens [weird, but I think it is how QToolButton is supposed to work]
3. Checking an action makes it the default [ok]
4. The toolbar button is shown as checked when an action is selected [ok]

Case 3

QToolButton::InstantPopup and m_menuLogic == DefaultLogic
If I do not call suggestDefaultAction everything works as expected
If I do call suggestDefaultAction: (should raise an exception)

• The button icon is set to that of the suggested action and stays as so forever [weird]

Case 4

QToolButton::InstantPopup and m_menuLogic == ImplicitDefaultAction (should raise an exception)
If I do not call suggestDefaultAction:

• The button icon is set to that of the checked action [weird]

If I do call suggestDefaultAction:

• The button icon is set to that of the suggested action [weird]
• The button icon is set to that of the checked action [weird]

I suggest to change ToggleActionMenu::defaultAction to:

QAction * ToggleActionMenu::defaultAction()
{
    if ( ( m_menuLogic & ImplicitDefaultAction ) )
    {
        QAction * aChecked = checkedAction( menu() );
        if ( aChecked )
        {
            return aChecked;
        }
    }
    return m_defaultAction;
}

and get rid of ToggleActionMenu::suggestDefaultAction.

In this way we simplify the things and in ImplicitDefaultAction mode it is enough to check the action after the ToggleActionMenu has been created to make it the default action automatically.

In PageView::setupActions we should have the following:

d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this,
                                              QToolButton::MenuButtonPopup,
                                              ToggleActionMenu::ImplicitDefaultAction );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setDefaultAction( d->aMouseTextSelect );

...

d->aMouseSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::RectSelect );
d->aMouseTextSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TextSelect );
d->aMouseTableSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TableSelect );

what do you think?

I suggest this change and adding an exception for case 4 in the constructor.

I have not yet tested what happens if you place ToggleActionMenu in a menu and if my suggested change breaks the things.

I also would like to get rid of DefaultLogic and just use ImplicitDefaultAction, but I am not sure if this creates problem when ToggleActionMenu is placed in a menu. Should it behave as a standard KActionMenu when in a menu? I think that when in a menu there is no meaning of "default action", because when clicked, the ToggleActionMenu should just open the submenu, right?

To answer your second comment first, and to be sure you understand everything how I understand you understand it...

In D21971#559081, @simgunz wrote:

I also would like to get rid of DefaultLogic and just use ImplicitDefaultAction, but I am not sure if this creates problem when ToggleActionMenu is placed in a menu.

No, creates no problem.

Should it [ToggleActionMenu] behave as a standard KActionMenu when in a menu? I think that when in a menu there is no meaning of "default action", because when clicked, the ToggleActionMenu should just open the submenu, right?

Yes, there it should behave as a plain submenu. And yes, in a submenu there is no thing like default action.

In D21971#559080, @simgunz wrote:

I have tried to analyze all the possible cases regarding ToggleActionMenu placed in a toolbar.

From what I understand:

• DefaultLogic means, the developer needs to manage everything manually calling/connecting setDefaultAction.
• ImplicitDefaultAction instead allows to set the default action automatically when an action is triggered

Exactly.

CASE 1

QToolButton::MenuButtonPopup or QToolButton::DelayedPopup and m_menuLogic == DefaultLogic

If I do not call suggestDefaultAction and no action is selected:

• the ToggleActionMenu icon is shown
• Clicking on the toolbar button nothing happens [weird, but maybe ok]
• Checking an action has no effect on the toolbar button
• The toolbar button is never shown as checked [weird]

That’s not an intended usage. When you choose DefaultLogic mode, you have to explicitely set the default action, like with a plain QToolButton.

If I do call suggestDefaultAction and no action is selected:

• the toolbar button icon is set to the one of the suggested default action
• Clicking on the toolbarbutton always activates the suggested default action
• Checking a different action has no effect on points 1 and 2 [weird]

Yes, in DefaultLogic mode you have to update the default action yourself.

CASE 2

QToolButton::MenuButtonPopup or QToolButton::DelayedPopup and m_menuLogic == ImplicitDefaultAction
If I do call suggestDefaultAction and no action is selected everything works as expected.
If I do not call suggestDefaultAction and no action is selected:

• the ToggleActionMenu icon is shown
• Clicking on the toolbar button nothing happens [weird, but I think it is how QToolButton is supposed to work]

Correct.

• Checking an action makes it the default [ok]
• The toolbar button is shown as checked when an action is selected [ok]
  
  CASE 3 [& 4]
  
  QToolButton::InstantPopup [...] [weird]

Yes, InstantPopup is totally pointless for ToggleActionMenu.

---

I suggest to change ToggleActionMenu::defaultAction to:

QAction * ToggleActionMenu::defaultAction()
{
    if ( ( m_menuLogic & ImplicitDefaultAction ) )
    {
        QAction * aChecked = checkedAction( menu() );
        if ( aChecked )
        {
            return aChecked;
        }
    }
    return m_defaultAction;
}

and get rid of ToggleActionMenu::suggestDefaultAction.

In this way we simplify the things and in ImplicitDefaultAction mode it is enough to check the action after the ToggleActionMenu has been created to make it the default action automatically.

(That is already possible.)

In PageView::setupActions we should have the following:

d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this,
                                              QToolButton::MenuButtonPopup,
                                              ToggleActionMenu::ImplicitDefaultAction );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setDefaultAction( d->aMouseTextSelect );

...

d->aMouseSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::RectSelect );
d->aMouseTextSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TextSelect );
d->aMouseTableSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TableSelect );

what do you think?

Should be probably ok, that’s just not really “implicit”.
I don’t know where ToggleActionMenu will be used elsewhere, and how the actions will be pre-checked there. Probably there will be usecases without a definite time to pre-check the actions.

I suggest this change and adding an exception for case 4 in the constructor.

I don’t know about exceptions yet. Are they this stuff that crashes the application when something goes slightly wrong and the caller doesn’t care?

Dear User,
*MOOOP MOOOP* Unhandled Exception 0x11050!!!()65567 *MOOOP MOOOP*
Lang.Excep.ion.Blah.blah
[ Ok ]

In that case: No, I don’t think it makes sense to crash an application because a QToolButton has the wrong default action, but the user can still use the menu...

To sum it up, if I understand correctly:

• you agree in removing DefaultLogic
• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

The change to ToggleActionMenu::defaultAction is required, otherwise if setDefaultAction is called on action1 manually and then action2 is checked, defaultAction returns action1.

Should be probably ok, that’s just not really “implicit”.

In this case setDefaultAction has the meaning of suggestDefaultAction, i.e., I set the textSelectionAction as a default, but then if any of the other actions are checked the default action is changed.
An alternative is to keep suggestDefaultAction public and make setDefaultAction private. (even better I would rename suggestDefaultAction > setDefaultAction and setDefaultAction > something else)

Thinking forward, ToggleActionMenu could become just a 'mode' of KActionMenu. Right?

In D21971#559485, @simgunz wrote:

To sum it up, if I understand correctly:

• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

Yes, but how? It can’t simply be forbidden in the constructor argument list. Remove it from the body of the constructor, so KActionMenu is set to MenuButtonPopup instead?

• you agree in removing DefaultLogic

No, it is needed when other actions than the exclusive group are added to the menu.
Example is D21195, where an action to configure the color modes is added to the Color Mode menu. When you click that action in ImplicitDefaultAction mode, it would become the default action, and the toolbar tells you that you are in color mode “Configure...”.
Other option is to allow only checkable actions to become the default action. But then, you can’t add real “actions” (opposed to “tools”). Imagine the user has some construction plan, and the application has several actions of kind “Mark for <job>”, and the user wants to apply one of these action to several items. Then it could be useful to collect all “Mark for <job>” actions in a ToggleActionMenu ImplicitDefaultAction mode, while the actions can not be checked.

The change to ToggleActionMenu::defaultAction is required, otherwise if setDefaultAction is called on action1 manually and then action2 is checked, defaultAction returns action1.

I can’t follow you here. Yes, when in DefaultLogic mode, and setDefaultAction(action1) was called, defaultAction() returns action1. Maybe your thinking was focused on checkable actions so far?

[...]
Thinking forward, ToggleActionMenu could become just a 'mode' of KActionMenu. Right?

Yes. But I would like Bug 413827 to be resolved (so PopupMode enumerator is used).

In D21971#560495, @davidhurka wrote:

In D21971#559485, @simgunz wrote:

To sum it up, if I understand correctly:

• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

Yes, but how? It can’t simply be forbidden in the constructor argument list. Remove it from the body of the constructor, so KActionMenu is set to MenuButtonPopup instead?

Create a new enum ToggleActionMenu::ToolButtonPopupMode with only DelayedPopup and MenuButtonPopup.

• you agree in removing DefaultLogic

No, it is needed when other actions than the exclusive group are added to the menu.
Example is D21195, where an action to configure the color modes is added to the Color Mode menu. When you click that action in ImplicitDefaultAction mode, it would become the default action, and the toolbar tells you that you are in color mode “Configure...”.
Other option is to allow only checkable actions to become the default action. But then, you can’t add real “actions” (opposed to “tools”). Imagine the user has some construction plan, and the application has several actions of kind “Mark for <job>”, and the user wants to apply one of these action to several items. Then it could be useful to collect all “Mark for <job>” actions in a ToggleActionMenu ImplicitDefaultAction mode, while the actions can not be checked.

ok

The change to ToggleActionMenu::defaultAction is required, otherwise if setDefaultAction is called on action1 manually and then action2 is checked, defaultAction returns action1.

I can’t follow you here. Yes, when in DefaultLogic mode, and setDefaultAction(action1) was called, defaultAction() returns action1. Maybe your thinking was focused on checkable actions so far?

Try the following code and steps:

d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this,
                                              QToolButton::MenuButtonPopup,
                                              ToggleActionMenu::ImplicitDefaultAction );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setDefaultAction( d->aMouseTextSelect );

...

d->aMouseSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::RectSelect );
d->aMouseTextSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TextSelect );
d->aMouseTableSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TableSelect );

• Check Browse Mode
• Restart Okular

Results: Browse Mode is checked / Text selection is the default action and unchecked

• Check Text selection
• Restart Okular

Results: Text selection is the default action and checked

• Check Table Selection
• Restart Okular

Results: Text selection is the default action and unchecked / Table Selection is checked but not the default action

With my change the last case will result in:
Results: Text selection is the default action and checked

[...]
Thinking forward, ToggleActionMenu could become just a 'mode' of KActionMenu. Right?

Yes. But I would like Bug 413827 to be resolved (so PopupMode enumerator is used).

ok

In D21971#560532, @simgunz wrote:

In D21971#560495, @davidhurka wrote:

In D21971#559485, @simgunz wrote:

To sum it up, if I understand correctly:

• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

Yes, but how? It can’t simply be forbidden in the constructor argument list. Remove it from the body of the constructor, so KActionMenu is set to MenuButtonPopup instead?

Create a new enum ToggleActionMenu::ToolButtonPopupMode with only DelayedPopup and MenuButtonPopup.

Sounds reasonable. Can we be sure that no one wants to use InstantPopup, because KSelectAction does that? KSelectAction does not plug in a menu, right? I can’t think of such a use case, even the View Mode menu (see D21196) wouldn’t fit.

The change to ToggleActionMenu::defaultAction is required, otherwise if setDefaultAction is called on action1 manually and then action2 is checked, defaultAction returns action1.

I can’t follow you here. Yes, when in DefaultLogic mode, and setDefaultAction(action1) was called, defaultAction() returns action1. Maybe your thinking was focused on checkable actions so far?

I think I understood now what you meant. Your suggested changes to the method defaultAction():

In D21971#559080, @simgunz wrote:

I suggest to change ToggleActionMenu::defaultAction to:

QAction * ToggleActionMenu::defaultAction()
{
    if ( ( m_menuLogic & ImplicitDefaultAction ) )
    {
        QAction * aChecked = checkedAction( menu() );
        if ( aChecked )
        {
            return aChecked;
        }
    }
    return m_defaultAction;
}

I thought whether this could give unexpected results, when you trigger an action so it becomes unchecked, but there are more checkable options further down in the menu. (Imagine Color Mode from D21195, with an additional option Preserve images.) But then, you wouldn’t use ImplicitDefaultAction mode.

But there are more general use cases where this does not work, like the Create an additive primitive action menu in FreeCAD. This could be created with a ToggleActionMenu in ImplicitDefaultAction mode, by simply adding all actions and suggesting/setting a default one.

Try the following code and steps:

d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this,
                                              QToolButton::MenuButtonPopup,
                                              ToggleActionMenu::ImplicitDefaultAction );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setDefaultAction( d->aMouseTextSelect );

...

d->aMouseSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::RectSelect );
d->aMouseTextSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TextSelect );
d->aMouseTableSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TableSelect );

• Check Browse Mode
• Restart Okular Results: Browse Mode is checked / Text selection is the default action and unchecked
• Check Text selection
• Restart Okular Results: Text selection is the default action and checked
• Check Table Selection
• Restart Okular Results: Text selection is the default action and unchecked, Table Selection is checked but not the default action
  
  With my change the last case will result in: Results: Text selection is the default action and checked

Yes, QAction::setChecked() does not emit triggered(), but toggled(). So theoreticaly I should connect ToggleActionMenu::setDefaultAction() to toggled(), but that’s not emitted by QMenu, so I have to connect to all actions in the menu manually. Furthermore, an action could be toggled from outside ToggleActionMenu’s menu, e. g. by using the shortcut. It may or may not be intended that this updates the toolbar button. When the application itself toggles an action, it is usually not intended that the toolbar buttons are updated.

In D21971#560810, @davidhurka wrote:

• Restrict toolbar button popup mode to DelayedPopup and MenuButtonPopup using a custom enum
  
  Honestly, I’m not sure whether this is good. We say no one wants to use InstantPopup, but as well we could say no one wants DelayedPopup. @ngraham?

What distinguish ToggleActionMenu from KMenuAction is that when plugged in a toolbar and an action is checked, that action becomes the default. This functionality makes sense for both MenuButtonPopup and DelayedPopup while it does not for InstantPopup. If one need to use InstantPopup he should not use this class but the others that already provide this functionality.

Sorry, what is it that you'd like my input on? This conversation is rather long and I'd like to be 100% sure I understand what you're asking about.

In D21971#564195, @ngraham wrote:

Sorry, what is it that you'd like my input on?

No problem. :) It’s about the popup modes of the toolbar buttons. We recognized that InstantPopup is useless for ToggleActionMenu, because then it has just the functionality of KSelectAction, so one could use that instead. So InstantPopup is excluded by a custom enum ToggleActionMenu::PopupMode now.

But I am unsure whether this is an oppurtunity to ban DelayedPopup as well, because we already recognized earlier that it is annoying in many cases. (Like for ToolAction, we changed it to MenuButtonPopup recently.)

So the question is: When InstantPopup is excluded, should DelayedPopup be excluded as well?

Sorry, this technical discussion is beyond me. :)

Is it me or this thing doesn't compile?

tsdgeos@xps:~/devel/kde/okular/build:arcpatch-D21971$ make -j1
[  0%] Automatic MOC for target okularcore
[  0%] Built target okularcore_autogen
[ 14%] Built target okularcore
[ 15%] Automatic MOC for target okularpart
[ 15%] Built target okularpart_autogen
[ 16%] Building CXX object CMakeFiles/okularpart.dir/okularpart_autogen/mocs_compilation.cpp.o
In file included from /home/tsdgeos/devel/kde/okular/build/okularpart_autogen/UYX5XTB5RZ/moc_toggleactionmenu.cpp:10,
                 from /home/tsdgeos/devel/kde/okular/build/okularpart_autogen/mocs_compilation.cpp:24:
/home/tsdgeos/devel/kde/okular/ui/toggleactionmenu.h:63:5: error: new types may not be defined in a return type
   63 |     enum PopupMode {
      |     ^~~~
/home/tsdgeos/devel/kde/okular/ui/toggleactionmenu.h:63:5: note: (perhaps a semicolon is missing after the definition of ‘ToggleActionMenu::PopupMode’)
/home/tsdgeos/devel/kde/okular/ui/toggleactionmenu.h:68:48: error: return type specification for constructor invalid
   68 |     explicit ToggleActionMenu( QObject *parent );
      |                                                ^
make[2]: *** [CMakeFiles/okularpart.dir/build.make:95: CMakeFiles/okularpart.dir/okularpart_autogen/mocs_compilation.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:848: CMakeFiles/okularpart.dir/all] Error 2
make: *** [Makefile:141: all] Error 2

Needs a rebase for sure

It does not compile indeed. My fault in the review process, I missed the last commit. I have added the inline comments to fix the bug.

@davidhurka friendly ping. :)

You need to merge master into this review or rebase because there is a merge conflict. I suggested you the modifications that were introduced in master (qAsConst) and few other changes.

I’m getting a bit dizy about the repository. I thought I merged master, but apparently I made a mistake. Thanks for pointing me along the right way.

Unfortunately I can’t compile now because I don’t have the time, can you try it?

I'm going to merge this because it's been hanging around for more than 6 months and Simone is vouching for it, but honestly for stuff like this we should have UI autotests since it's the thing that will *defenitely* break in the future because it does so many things people will not even be sure what was the intended behaviour.

My suggestion before you go and continue trying to get D15580 landed is get some autotests on the ToggleActionMenu functionality.

Also i would really appreciate if you could change to using invent.kde.org instead of phabricator, this way we have the automated code checking tools running on your code patch.