Review Request 126895: Make KGlobalAccel dependency in KXmlGui optional

Mon Feb 1 07:30:04 UTC 2016

> On Jan. 29, 2016, 9:43 a.m., David Faure wrote:
> > This looks good to me. It doesn't affect Linux, it doesn't affect API / binary compatibility, it uses #if so no risk of missing include, and it hides the globalaccel gui so it's not like users have the risk on clicking on something that doesn't work.
> > 
> > Default global shortcuts set by apps won't work of course, but AFAICS this is done using KGlobalAccel API anyway, not kxmlgui API. So that's perfect IMHO, neither users or apps will end up with broken functionality.
> 
> Martin Gräßlin wrote:
>     > but AFAICS this is done using KGlobalAccel API anyway, not kxmlgui API.
>     
>     KActionCollection sets the component name without the global shortcut won't work. So no, with the ifdef the shortcuts break.
> 
> David Faure wrote:
>     Can you explain what you mean? I have trouble parsing the first sentence.
> 
> Martin Gräßlin wrote:
>     when registering a QAction in KActionCollection it gets properties installed. Those are needed by KGlobalAccel to trigger the action. The current patch disables that code as well. Thus an application using KGlobalAccel and KActionCollection will break if KXmlGui is compiled without KGlobalaccel support.
> 
> David Faure wrote:
>     Yes of course, compiling lib2 without support for lib1, and THEN installing lib1, is always bound to create trouble. This is true for any optional dependency. "But I have lib1 installed!!!" ... "well, yeah ok, but you did that too late".
>     
>     If we have a way to set the property without using kglobalaccel that would be even better, but I didn't look into it to know if that's possible, I guess not, if it uses KGlobalAccel API?
> 
> Martin Gräßlin wrote:
>     it is possible, I ported KWin away from using KActionCollection and set the property/objectName manually. The problem is IMHO rather that it needs porting.
> 
> David Faure wrote:
>     But then why don't we do that in the kxmlgui code itself? instead of #if HAVE_KGLOBALACCEL  /* call API that sets a property */ #endif
>     we can do setProperty directly, no? Then the apps don't need any porting.

Ah no the usage of KGlobalAccel seems to be more than that. Looking at what got ifdefed there seems to be code to load the shortcuts from a config file, etc.

I'm not really familiar with Xmlgui and it's intereaction with KGlobalAccel. All I know is that back in KDELibs4 times one used a KActionCollection to get a KAction and each KAction could have a global shortcut (KShortcut). Now with QAction and QKeySequence it should be possible to do all without a KActionCollection. Whether it's possible to get the convenience of KActionCollection for registering global shortcuts without a KGlobalAccel dependency I do not know.

- Martin

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

On Jan. 28, 2016, 2:29 p.m., Andre Heinecke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126895/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2016, 2:29 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> -------
> 
> This is part of a three patch series that aims to allow a "leightweight" build of KXmlGui without DBus and KService dependencies. I've added the patches to: https://phabricator.kde.org/T1390 I'm not sure if I can create reviews that depend on changes from another review, I'll try and if it does not work I'll open one after another.
> 
> Global shortcuts are a nice optional feature to have. But as they are not strictly neccessary for the core functionality of KXmlGui, as I see it, and pull in an extra dependency to DBus and need runtime support on the target platform they should be optional.
> 
> This (and the other changes) add lots of unloved ifdefs, I could understand if thats disliked. But let me explain the background of this change:
> 
> I'm currently updating Kleopatra in Gpg4win to a KDE Frameworks based build. This is nice. Frameworks are awesome, I can just pick what I need and don't have dependencies to lots of things that are actually not needed.
> Then comes KXmlGui, adds 20 Framework dependencies, and I don't know what to do.
> I want:
> - configureable "KDE Style" GUI
> - configurable Shortcuts
> - KDE Standardactions (e.g. Help / WhatsThis)
> - kbugreport
> - KDE Integration in an KDE Environment
> 
> But I don't want:
> - Global Shortcuts (we don't have kded so this won't work for us anyway)
> - DBus (our dbus is directory scoped and there are no other applications using dbus installed by us)
> - KService dependency (System configuration has been troublesome in the past on Windows and is not neccessary if we provide just a single installation)
> 
> So these Patches are my way out of this Problem. Without the optional packages KXmlGui provides what I want and does not depend on what I don't want.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 9d79619 
>   src/CMakeLists.txt 58f0c7a 
>   src/config-xmlgui.h.cmake 07c882f 
>   src/kactioncollection.cpp 9c45725 
>   src/kkeysequencewidget.cpp b2e2b6a 
>   src/kshortcuteditwidget.cpp 670d031 
>   src/kshortcutseditor.cpp 99dfb3d 
>   src/kshortcutseditoritem.cpp 461a90c 
>   src/kxmlguifactory.cpp 2767e69 
> 
> Diff: https://git.reviewboard.kde.org/r/126895/diff/
> 
> 
> Testing
> -------
> 
> Compiled with and without dependency. Tested Kleopatra against it.
> Not yet tested on Windows, will do so in the next days.
> 
> 
> Thanks,
> 
> Andre Heinecke
> 
>

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