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
