Review Request 120025: Have KStandardAction::create(...) call KActionCollection::setDefaultShortcuts()

[Friedrich W. H. Kossebau]

> On Sept. 1, 2014, 7:55 vorm., David Faure wrote:
> > You wrote: "I have not yet completely grasped the concept of the default shortcuts and why they are set only explicitely via KActionCollection::setDefaultShortcuts."
> > 
> > I think the concept is simple. QAction only knows "the current shortcuts, those that will trigger the action". Now imagine that the user configures the shortcut for the quit action (default Ctrl+Q) to use something else instead (Ctrl+E for exit, whatever). QAction will then only know about Ctrl+E, the actual active shortcut. But the shortcut configuration dialog needs to know that the default shortcut was Ctrl+Q, so that we can offer "revert to default", for instance (and to show that the shortcut was manually modified).
> > This information API is in KActionCollection because, well, there's no KAction anymore, and it stores the default shortcut into the QAction as a dynamic property.
> > 
> > Well, this means another solution could be to just set the dynamic property in KStandardAction, without going through KActionCollection via invokeMethod. That would probably more robust,
> > including the case of creating an action without a collection as parent, and then putting it into a collection later on.
> > 
> > Good thing I wrote the explanation, it made me realize that there is a better solution :-)

Ah, so "KActionCollection::setDefaultShortcuts" is from KAction, and might not just have found a better place? I see. When looking at the implementation I was puzzled why it is not a static method, given that it does not use the KActionCollection object itself for anything. So not really done by original design.
Putting the property already directly in KStandardAction seems an option as well. Creates an informal dependency, as the property id used has to be kept in sync, but might be okay.
I am not sure though if the property should be set also if there is no KActionCollection given as parent, because that would mean that people using KConfigWidgets, but not KXMLGUI, still will get a payload of that extra property on every standard action they create. Instinctively I would say No to that, but perhaps others are more pragmatic and do not do byte-counting?


- Friedrich W. H.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120025/#review65624
-----------------------------------------------------------


On Aug. 31, 2014, 4:40 nachm., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120025/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2014, 4:40 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 338222
>     https://bugs.kde.org/show_bug.cgi?id=338222
> 
> 
> Repository: kconfigwidgets
> 
> 
> Description
> -------
> 
> As e.g. reported in https://bugs.kde.org/show_bug.cgi?id=338222 ("False positive critical warnings for KStandardActions") currently KXMLGUIFactoryPrivate::saveDefaultActionProperties complains about lots of actions that have been created properly via KStandardActions with a KActionCollection as parent. Just grep the log of your favourite XMLGUI-based KF5-ported program to see yourself.
> 
> I have not yet completely grasped the concept of the default shortcuts and why they are set only explicitely via KActionCollection::setDefaultShortcuts. But to me it makes some sense to have this automatically called for all standardactions which are created directly with a KActionCollection as parent.
> I decided not to change KActionCollection::addAction because I had even less idea what all might be affected by that.
> 
> So this is what this patch does:
> * add a call to KActionCollection::setDefaultShortcuts if there is a standard shortcut (via QMetaObject::invokeMethod, like done for KActionCollection::addAction)
> * also move code which only should be done in case of a created action into one, same branch
> 
> Needs https://git.reviewboard.kde.org/r/120024/ to make KActionCollection::setDefaultShortcuts() invokable.
> 
> 
> Diffs
> -----
> 
>   src/kstandardaction.cpp a18527b 
> 
> Diff: https://git.reviewboard.kde.org/r/120025/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140905/c46bbc32/attachment.html>