D21971: Replace ToolAction by ToggleActionMenu

[David Hurka]

davidhurka added a comment.


  In D21971#560532 <https://phabricator.kde.org/D21971#560532>, @simgunz wrote:
  
  > In D21971#560495 <https://phabricator.kde.org/D21971#560495>, @davidhurka wrote:
  >
  > > In D21971#559485 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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.
  F7752159: image.png <https://phabricator.kde.org/F7752159>
  The last triggered action is not neccessarily checked, especcialy ones which are “actions”, not “options” and thus not checkable.
  
  > 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.

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/20191110/249b2761/attachment-0001.html>