davidhurka added a comment.
In D21755#482891 <https://phabricator.kde.org/D21755#482891>, @simgunz wrote: > 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? INLINE COMMENTS > simgunz wrote in pageview.cpp:660 > setDelayed and setStickyMenu shoule be defaults, without having to set them > here. I think you are right, but this default IMHO belongs to KActionMenu. Ok to provide MenuButtonPopupMode as default for a constructor parameter? > simgunz wrote in pageview.cpp:667 > Can't this connection be defined inside ToggleActionMenu? 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. > simgunz wrote in pageview.cpp:671 > All the code below, cannot be managed inside ToggleActionMenu? No, for the same reason. > simgunz wrote in toggleactionmenu.h:47 > Make parent a QObject to make it more general. I would use this in > AnnotationActionHandler which is not a QWidget. Thanks for spotting this, it was a subclassing mistake by me. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D21755 To: davidhurka, #okular Cc: simgunz, aacid, #vdg, okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen
