> On Jan. 29, 2016, 8: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.
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. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126895/#review91751 ----------------------------------------------------------- On Jan. 28, 2016, 1: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, 1: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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
