simgunz added a comment.
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.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D21971
To: davidhurka
Cc: ngraham, simgunz, okular-devel, johnzh, andisa, siddharthmanthan, maguirre,
fbampaloukas, joaonetto, kezik, tfella, darcyshen, aacid
