D21971: Replace ToolAction by ToggleActionMenu

[Simone Gaiarin]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20191106/603c4e14/attachment-0001.html>