D21971: Replace ToolAction by ToggleActionMenu

[Simone Gaiarin]

simgunz added a comment.


  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`.
  
  >> - you agree in removing `DefaultLogic`
  > 
  > No, it is needed when other actions than the exclusive group are added to the menu.
  >  Example is D21195 <https://phabricator.kde.org/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 <https://bugs.kde.org/show_bug.cgi?id=413827> to be resolved (so PopupMode enumerator is used).
  
  ok

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