[RFC] Replace ToolAction by a more universal “ToggleActionMenu”
Needs ReviewPublic

Authored by davidhurka on Jun 11 2019, 9:27 PM.

Details

Reviewers
None
Group Reviewers
Okular
Summary

This replaces the good working ToolAction by ToggleActionMenu.
Unlike ToolAction, ToggleActionMenu inherits from KActionMenu, and is more universally applicaple.

  • Menu can be set from outside, not hard coded.
  • Default action for toolbar button is controllable from outside. (Theoretically, the button could trigger anything now.)
  • KActionMenu instead of KSelectAction:
    • Pluggable in other menus, thus called “Menu”.
    • Doesn’t make the actions exclusive, so any actions can be added to the menu.

This will replace the (mostly similar) CheckableActionMenu from my other patch D21195, maybe leading to more consistence.

And it makes D21622 and D21635 obsolete, just because the default action is configurable.

Screenshot:
Everything like before, here with mouse_selecttool added to Tools menu to show submenu capability.

TODO:

  • Load correct selection tool from configuration at startup, after mouseModeMenu is created. (Difficult, but done another way now.)
  • Fix crash at destruction
  • Better name than ToggleActionMenu? ToolAction was probably better. (Added description for the name.)
  • License headers. (I’ve seen that mailing list thread, yes... :-/ )
  • Disable the toolbar button when the menu itself is disabled. (KActionMenu doesn’t need this, because it shows itself on the toolbar button.)
  • Tooltips needed?
Test Plan
  • Open Okular and look at toolbar button -> has correct tool selected. (Not working yet.)
  • Open a document.
  • Look at toolbar button menu -> Correct menu entries (like before, with ToolAction).
  • Select some selection tools through shortcuts and toolbar button -> behaves correctly.
  • Add ToggleActionMenu ("mouse_selecttool") to menubar (..../kxmlgui5/okular/part.rc) -> Submenu looks correctly, has no checkbox attached and so on...
  • Add diverse other actions to the menu -> still works as before.
  • Add actions when toolbar buttons are already created -> actions are added to existing buttons.
  • setDefaultAction() to some completely unrelated action. -> ToggleActionMenu does not get confused.

Diff Detail

Repository
R223 Okular
Branch
create-toggleactionmenu
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13098
Build 13116: arc lint + arc unit
davidhurka created this revision.Jun 11 2019, 9:27 PM
Restricted Application added a project: Okular. · View Herald TranscriptJun 11 2019, 9:27 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
davidhurka requested review of this revision.Jun 11 2019, 9:27 PM
davidhurka retitled this revision from This replaces the good working ToolAction by a (not so good working) ToggleActionMenu. Unlike ToolAction, ToggleActionMenu inherits from KActionMenu, and is more universally applicaple. * Menu can be set from outside, not hard coded. * Default... to Replace ToolAction by a more universal “ToggleActionMenu”.Jun 11 2019, 9:29 PM
davidhurka edited the summary of this revision. (Show Details)
davidhurka added a subscriber: VDG.Jun 11 2019, 9:36 PM

I suggested to use KActionMenu in D21622 (although not really focussed on that patch). Turned out that KActionMenu can not access already constructed toolbar buttons, so that approach is not possible.

Instead, I grabbed my code from D21195 and improved it a bit.

Let me know whether this is a realistic UI concept or a special case in Okular.

ui/toggleactionmenu.h
25

I get a warning in this line “has virtual methods but non-virtual destructor”. True, the default destructor is not virtual, hiding the virtual destructor of the base class. And yes, Okular crashes at closing.

How do I add a proper destructor whithout breaking the destructor in KActionMenu?

aacid added a subscriber: aacid.Jun 11 2019, 9:44 PM
aacid added inline comments.
ui/toggleactionmenu.h
25

What do you mean "breaking the destructor"?

Destructors are cascaded adding a destructor won't make the parent class destructor not be called anymore.

I don't see why you should get this wanrning

KActionMenu ends up inheriting from QObject that has a virtual destructor, so you shouldn't be getting that warning at all

Actually i don't get it

davidhurka edited the summary of this revision. (Show Details)Jun 11 2019, 9:45 PM
davidhurka added inline comments.Jun 11 2019, 9:50 PM
ui/toggleactionmenu.h
25

Destructors are cascaded adding a destructor won't make the parent class destructor not be called anymore.

Makes sense, so I will add a virtual destructor.

As far as i know, a class gets a “normal” destructor, if no one is declared explicitely.

aacid added inline comments.Jun 11 2019, 10:06 PM
ui/toggleactionmenu.h
25

But that's what i'm saying you don't need to add a virtual destructor because it inherits from QObject

davidhurka added inline comments.Jun 11 2019, 10:09 PM
ui/toggleactionmenu.h
25

Hmm, don’t get the warning anymore. Must have been my fault.

Then I need to look what else is crashing...

davidhurka marked 5 inline comments as done.Jun 13 2019, 7:14 PM

Theoretically works, but signal-slot is tricking me.

ui/toggleactionmenu.cpp
64

Debugging...
forgetButton() is connected to QObject::destroyed of the toolbar buttons.
So, forgetButton() is called when the toolbar button is destructed.
But that happens when this ToggleActionMenu has already been destructed.
How is that possible, to call a slot of an already destructed object?

(BTW: Providing a destructor for ToggleActionMenu does not change anything.)

aacid added inline comments.Jun 13 2019, 9:07 PM
ui/toggleactionmenu.cpp
64

What you're describing isn't really possible. Connections are disconnected when an object gets destroyed.

How do I reproduce that crash?

davidhurka added inline comments.Jun 14 2019, 4:44 PM
ui/toggleactionmenu.cpp
64

That’s what puzzles me.

It doesn’t always crash (only 5–10% of times closing Okular). If you add qDebug()s to the functions, you see the destructor beeing called before forgetButton() is called. (Assuming ~ToggleActionMenu() override is added.) Valgrind confirms that (just run valgrind okular and close the window), and complains that forgetButton() is called on an already deleted object. (Invalid read of size 4 in QSet::isEmpty().)

I hope that I misinterpret something.

I couldn’t find a reason why disconnecting my slot at destruction fails, but it indeed fails.

Workarround is to use the simpler way: QPointer. (ToolAction already does it that way.)

ui/toggleactionmenu.cpp
64
  1. ~PageView() calls ~ToggleActionMenu(), because it’s a child.
  2. ~WidgetAction() (probably called trough some XMLGui stuff) calls ~QToolButton to delete the toolbar buttons.
  3. ~QToolButton() emits destroyed(), which triggers the ToggleActionMenu, although this was already destroyed.
  4. forgetButton() accesses deallocated space (this->m_buttons), possibly causing a segfault.

My only idea why ~QObject() fails to disconnect ToggleActionMenu:
It all happens in another destructor (but not in a base class destructor; in that case this behaviour would be officially documented).

davidhurka retitled this revision from Replace ToolAction by a more universal “ToggleActionMenu” to [WIP] Replace ToolAction by a more universal “ToggleActionMenu”.Jun 16 2019, 5:26 PM
davidhurka edited the summary of this revision. (Show Details)
davidhurka updated this revision to Diff 59946.Jun 16 2019, 5:31 PM
  • Work arround connection problem by using QPointer
davidhurka updated this revision to Diff 59958.Jun 16 2019, 7:54 PM
  • Remove accidentally inserted #include
  • remove repeated calls to QToolButton::setMenu()
  • Update class documentation
davidhurka edited the summary of this revision. (Show Details)Jun 16 2019, 7:58 PM
davidhurka edited the test plan for this revision. (Show Details)

From my point of view, this is complete now. 3 TODOs left (see revision description at the top).

ToolAction used its own tooltip for the toolbar buttons. Tell me if that is still needed.

@simgunz you just told that you will use ToolAction? If you show me your implementation, it’s ok for me to use that instead. Otherwise you can tell me what you need (tooltips?). :)

From my point of view, this is complete now. 3 TODOs left (see revision description at the top).

ToolAction used its own tooltip for the toolbar buttons. Tell me if that is still needed.

@simgunz you just told that you will use ToolAction? If you show me your implementation, it’s ok for me to use that instead. Otherwise you can tell me what you need (tooltips?). :)

Currently I am using ToolAction in the new annotation toolbar to selected among different geometrical annotation. ToolAction is placed in the toolbar and behaves as it behave for the mouse selection tools. I think that replacing it with your more general tool is a good thing.
For this purpose I need that the action are checkable, and that the ToggleActionMenu is checkable displaying the selected action (exaclty as ToolAction).

I did remove the tooltip from ToolAction because it was broken: the tooltip was displayed when hoovering over the ToolAction while it was selected, and a different less informative tooltip was displayed when not selected. Moreover the tooltip was about holding the button, thing that now has been removed if I am not wrong. For my use case, I would probably need to be able to display tooltips for each action in the ToggleActionMenu, to describe what they are.

If ToggleActionMenu provides the same functionality that ToolAction was providing I can use it without problems. I need to use it for Geometrical annotations and for the Stamp annotation. For this last one I would need to display the different available stamps, so each action in the ToggleActionMenu should just be a checkable action with a full width image and no text. Still not sure how to do this. Maybe I just need to create a custom QWidgetAction to represent the stamp, and than this action can be plugged into the ToggleActionMenu. Need to think about it.

Currently I am using ToolAction in the new annotation toolbar to selected among different geometrical annotation. [...]
For this purpose I need that the action are checkable, and that the ToggleActionMenu is checkable displaying the selected action (exaclty as ToolAction).

[...] For my use case, I would probably need to be able to display tooltips for each action in the ToggleActionMenu, to describe what they are.

[...] I need to use it for Geometrical annotations and for the Stamp annotation. For this last one I would need to display the different available stamps, so each action in the ToggleActionMenu should just be a checkable action with a full width image and no text. [...]

Maybe it’s better to use a KSelectAction directly? Not sure whether it shows the current selection on the toolbar button, but probably it does so in combobox mode.

For the stamps, you probably need a combobox with a custom item view.

davidhurka edited the summary of this revision. (Show Details)Jun 17 2019, 5:01 PM
davidhurka edited the test plan for this revision. (Show Details)
davidhurka updated this revision to Diff 60002.Jun 17 2019, 5:17 PM
  • Add license headers

Is this correct?

Currently I am using ToolAction in the new annotation toolbar to selected among different geometrical annotation. [...]
For this purpose I need that the action are checkable, and that the ToggleActionMenu is checkable displaying the selected action (exaclty as ToolAction).

[...] For my use case, I would probably need to be able to display tooltips for each action in the ToggleActionMenu, to describe what they are.

[...] I need to use it for Geometrical annotations and for the Stamp annotation. For this last one I would need to display the different available stamps, so each action in the ToggleActionMenu should just be a checkable action with a full width image and no text. [...]

Maybe it’s better to use a KSelectAction directly? Not sure whether it shows the current selection on the toolbar button, but probably it does so in combobox mode.

For the stamps, you probably need a combobox with a custom item view.

The problem of KSelectAction is that in ComboBoxMode it looks pretty ugly and does not show the checked Action. My guess is that this is the reason why ToolAction was created in the first place.

davidhurka edited the summary of this revision. (Show Details)Thu, Jun 20, 10:46 AM
davidhurka updated this revision to Diff 60141.Thu, Jun 20, 3:30 PM
  • Select correct default action at startup
  • Describe name ToggleActionMenu
  • Let the toolbar buttons respect menu properties like enabled state, and add the menu to updateActionStates().
  • Make popup mode configurable correctly, and set popup mode of mouse mode menu.
davidhurka retitled this revision from [WIP] Replace ToolAction by a more universal “ToggleActionMenu” to [RFC] Replace ToolAction by a more universal “ToggleActionMenu”.Thu, Jun 20, 3:30 PM
davidhurka edited the summary of this revision. (Show Details)
davidhurka marked 4 inline comments as done.Thu, Jun 20, 3:54 PM

I have tested this patch and added some inline comments. It seems to me that ToggleActionMenu requires way more external code to make it work, compared to ToolAction, which is quite automated. I think that some things could be made default in ToggleActionMenu, and some aspect hidden as well e.g. manage the QActionGroup internally without having the user have to manage it and set the action eveytime.

I tried it by only adding the actions to it and it does not work, no menu is shown. Adding the signal connection is also not enough. After that I gave up for now (I am lazy). But I think it needs to be a little more user-friendly.

ui/pageview.cpp
660

setDelayed and setStickyMenu shoule be defaults, without having to set them here.

667

Can't this connection be defined inside ToggleActionMenu?

671

All the code below, cannot be managed inside ToggleActionMenu?

ui/toggleactionmenu.h
48

Make parent a QObject to make it more general. I would use this in AnnotationActionHandler which is not a QWidget.

To be more clear, I think that the code below should provide a default working action menu (basically as it was before, but customizable if necessary):

d->aMouseModeMenu = new ToggleActionMenu( this );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );

I have tested this patch and added some inline comments. It seems to me that ToggleActionMenu requires way more external code to make it work, compared to ToolAction, which is quite automated. I think that some things could be made default in ToggleActionMenu, and some aspect hidden as well e.g. manage the QActionGroup internally without having the user have to manage it and set the action eveytime.

What would be a use case for an integrated QActionGroup? At least it couldn’t be used for the Selection Tools menu, because there are more mouse modes. ToolAction bypasses the QActionGroup of KSelectAction by shadowing addAction().

When the actions are in a QActionGroup, triggering the checked action wouldn’t do anything. InstantPopupMode would make sense then.

I tried it by only adding the actions to it and it does not work, no menu is shown. Adding the signal connection is also not enough. After that I gave up for now (I am lazy). But I think it needs to be a little more user-friendly.

Hmm. Did you click-and-hold?

ui/pageview.cpp
660

I think you are right, but this default IMHO belongs to KActionMenu.

Ok to provide MenuButtonPopupMode as default for a constructor parameter?

667

No, the idea of ToggleActionMenu is to let the application choose the default action.

Ok to make it optional? There would certainly be some use cases.

671

No, for the same reason.

ui/toggleactionmenu.h
48

Thanks for spotting this, it was a subclassing mistake by me.

davidhurka updated this revision to Diff 60259.Fri, Jun 21, 4:27 PM
  • Fix parent object QWidget to QObject, remove redundant function call

I could imagine to add a parameter ToggleActionMenu::MenuLogic to the constructor.

enum MenuLogic {
    DefaultLogic = 0x0,
    /**
     * Automatically makes the triggered action the default action, even if in a submenu.
     * When a toolbar button is constructed, the default action is set to the first checked action in the menu.
     * setDefaultAction() provides a fallback if no action is checked.
     */
    ImplicitDefaultAction = 0x1
};

The integrated QActionGroup (for mutually exclusive actions) requires to subclass ToggleActionMenu, to shadow addAction(). addAction() would need to add the action to the action group.

To be more clear, I think that the code below should provide a default working action menu (basically as it was before, but customizable if necessary):

d->aMouseModeMenu = new ToggleActionMenu( this );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );

I just tried to implement MenuLogic with ImplicitDefaultAction mode, and then I decided it’s easiest and straightforward to add suggestDefaultAction(). So it is possible.

It is available in D21971. What do you think about it?

I have tested this patch and added some inline comments. It seems to me that ToggleActionMenu requires way more external code to make it work, compared to ToolAction, which is quite automated. I think that some things could be made default in ToggleActionMenu, and some aspect hidden as well e.g. manage the QActionGroup internally without having the user have to manage it and set the action eveytime.

What would be a use case for an integrated QActionGroup? At least it couldn’t be used for the Selection Tools menu, because there are more mouse modes.

Right. Haven't noticed that the mouse modes are more than three.

ToolAction bypasses the QActionGroup of KSelectAction by shadowing addAction().

I think that this is more a bug than a feature. Adding action->setActionGroup( selectableActionGroup() ); as first command of addAction() would make the action group and all the related methods work e.g. actions().

When the actions are in a QActionGroup, triggering the checked action wouldn’t do anything. InstantPopupMode would make sense then.

Not clear what you mean here.

I tried it by only adding the actions to it and it does not work, no menu is shown. Adding the signal connection is also not enough. After that I gave up for now (I am lazy). But I think it needs to be a little more user-friendly.

Hmm. Did you click-and-hold?

Maybe not.

What I am probably not understanding is. Is ToggleActionMenu meant to be a general component that you want to use in other parts of KDE? Or is it just meant for Okular? If it is only used in Okular, where do you plan to use it besides than for the mouse actions?
What are the other modes in which it could work, besides the one presented in D21971 i.e. as a replacement of ToolAction (basically a cleaner implementation of it).

I never used KActionMenu so maybe I am missing something and creating an unuseful fuss :-) Thanks for the patience.

I tried it by only adding the actions to it and it does not work, no menu is shown. Adding the signal connection is also not enough. After that I gave up for now (I am lazy). But I think it needs to be a little more user-friendly.

I tried to reproduce that with the current revision (i. e. with QObject* as parent, but without configuration options, i. e. https://phabricator.kde.org/D21755?id=60141), but it worked.

In pageview.cpp, I reduced the code to create the menu to:

// Mouse-Mode action menu
d->aMouseModeMenu = new ToggleActionMenu( this );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setText( i18nc( "@action", "Selection Tools" ) );
ac->addAction( QStringLiteral( "mouse_selecttools" ), d->aMouseModeMenu );

Result: Toolbar button is boringly titled “Selection Tools”, but click-and-hold opens the menu. Actions in the menu work.

Also removing the last two lines causes the ToggleActionMenu not to be pluggable in the toolbar anymore.

I conclude that it can work. Did you test it differently?

davidhurka marked 2 inline comments as done.Sat, Jun 22, 8:27 PM
davidhurka added inline comments.
ui/toggleactionmenu.cpp
64

This problem does not disappear after I have replaced the parent pointer QWidget* by QObject*. Meh...
So QPointer will stay.