D21971: Replace ToolAction by ToggleActionMenu

[David Hurka]

davidhurka added a comment.


  To answer your second comment first, and to be sure you understand everything how I understand you understand it...
  
  In D21971#559081 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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...

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/d6be9d1d/attachment-0001.html>