> On Jan. 26, 2016, 6:40 p.m., Aleix Pol Gonzalez wrote: > > Hi, > > I'm a bit afraid of all these ifndef. Do you think it would make sense to > > abstract out the KGlobalAccel usage? > > > > Otherwise, would it be possible to make KGlobalAccel useful (or just dumb) > > on Windows? > > Boudewijn Rempt wrote: > I would say: most applications do not need global accelerators, so making > kglobalaccel functional on windows is not really relevant, you wouldn't want > that dependency anyway because it doesn't add functionality. And building a > library and including it is always a burden, so I would say it's much better > to make it optional. > > Andre Heinecke wrote: > Hi, > > Abstracting it out would also be quite invasive imho. And from my > experience abstraced (optional) features are even harder to maintain then > ifdefs where you can easily see the codepath taken when something is not > available. While side effects through abstraction are harder to see when > hacking on code. > > As for the other point: > - GlobalAccel means that you basically need to have a keylogger on your > platform. I don't want that. There are system ways on Windows to create > global shortcuts already. Not as fine grained as KDE Actions but you could > use them to do command line calls. > - I don't really see this as a Windows only thing. KXmlGui provides very > useful features even without Global Shortcuts. And as for making KGlobalAccel > just dumb on Windows. While I think this would generally be a good idea I > expect that others (from Kde-Windows) who provide several KDE applications > and stuff like Systemsettings on Windows would disagree. > > We could add a dummy class in KXmlGui thats used if KGlobalAccel is not > available? This could avoid lots of ifdefs. But this would add a bit > maintenance cost when new API is used. While the IFDEFS make it quite > transparent to hackers that if you do thomething with GlobalAccel "please > guard it". > > Aleix Pol Gonzalez wrote: > @boud, yes, I also thought about your RR, in fact I looked it up but > couldn't find it. > > Ok, maybe it's actually the way to make xmlgui viable to deploy. > > Take this as a +1 by me. > > Martin Gräßlin wrote: > Could we maybe continue the route I went with making sure that > kglobalaccel doesn't start? I'm quite concerned about the ifdefs. If there > are still situations where kglobalacceld is started without being needed, > let's fix that. Let's make it a proper runtime thing instead of an ifdef > messery nobody will check. > > I'm quite concerned about making the change optional as that's a route to > breakage with distros and as the maintainer of kglobalaccel I'm not looking > forward to those bug reports. > > Boudewijn Rempt wrote: > Well, part of the point, for me at least, is not having the dependency at > all. Any extra library, especially one that adds no functionality but is just > present, is a burden just like #idefs are a burden. > > Martin Gräßlin wrote: > What can we do to make the burden not so hard on your side without adding > the ifdefs? KGlobalaccel is basically a tier 1 - the higher number is due to > the runtime part. Would it help to make the runtime part optional? Would it > help to have a BC drop in replacement which just no-ops? > > By doing the change as suggested here new burden is created and moved to > the shoulders of others. E.g. all Linux distributions which now have to be > more careful with packaging. So we need to find the right balance. > > Boudewijn Rempt wrote: > Well, for me personally it's water under the bridge. On the other hand, I > don't think that it's a burden for distributions: distributions always > install every dependency, even if it's optional. That is the big problem that > has led over the years to people complaining that Krita needed Marble, for > instance. > > Andre Heinecke wrote: > For me the build problem with KGlobalAccel is the build dependency to > DBus. BC drop in with No ops would help (in which case the configuration > entries should be completly hidden in the gui). But would a KGlobalAccel > without DBus / No-Ops be easier to maintain? > > And the best thing for me is that If I don't want some features to be > able not to build them at all instead of a replacement library. And a > KGlobalAccel "Dummy" as part of KXmlGui also appears wrong to me. > > Also my other two patches make DBus and KTextWidgets optional. For these > I definetly think that Ifdefs are the right way to go. > > > By doing the change as suggested here new burden is created and moved > to the shoulders of others. E.g. all Linux distributions which now have to be > more careful with packaging. So we need to find the right balance. > > I have to agree with Boudewijn there. We could of course only make it > optional on Windows but I would like to avoid making it a platform issue. > > Martin Gräßlin wrote: > > But would a KGlobalAccel without DBus / No-Ops be easier to maintain? > > if KGlobalAccel in it's current state is so bad that it needs to be > patched out of other frameworks, then yes KGlobalAccel needs to be modified. > Which is what I already did in the past, when it was brought to my attention > that just using xmlgui results in the runtime being started. > > Does it make sense to have a DBus free kglobalaccel? Certainly, on > non-Linux it doesn't make sense to use DBus. > > So the question to me is: is a stripped down KGlobalAccel (no DBus, no > runtime) sufficient to not get it patched out of other frameworks. If yes I > think that's the way to go. Is it work? Yes it is, but not that much. It's > only one source file with around 700 lines of code. > > Boudewijn Rempt wrote: > Well, what I am trying to say is that it's wrong to have a depedency on a > library, a chunk of code, that doesn't add functionality to the application. > > Martin Gräßlin wrote: > > it's wrong to have a depedency on a library, a chunk of code, that > doesn't add functionality to the application. > > aye, but this goes both sides. I could also say that it's wrong to have > the ifdefs. This is a balancing act. Adding ifdefs adds costs just like > adding the "chunk of code" adds costs. The question is which one adds less. > I'd argue that by adding ifdefs to all places which use kglobalaccel you add > more costs for the community (we need multiple CI build setups, we need to > handle distro issues, making code more difficult to read, more difficult to > test). Thus I suggest that we improve the other side. Get kglobalaccel into a > state where you don't care that you have that code around. If you absolute > insist on your position: yay less work for me. > > Andre Heinecke wrote: > My point agains this is the same as Boudewijn. Why would we want to > create a defunct GlobalAccel package? > > GloabAccel currently provides a useful feature. Global Shortcuts. I would > like to see this feature optional in KXMLGui. There are use cases for XMLGui > without Global Shortcuts. > Why should we include, build, distribute, include the License etc. Of a > chunk of code that adds no functionality to our software? > > And if we wen't that rode wouldn't be the conclusion of this to also > create a KTextWidgets package that adds no Features to make this optional? > Or a Dbus package without Dbus that does not add Interprocess > communication? > > Regarding the CI / Community Overhead you would also have this is you > want to really test every variant. Then you would also have to test against a > "defunct" KGlobalAccel package or handle reports about issues caused by > KGlobalAccel not behaving as expected as it misses runtime parts. > > Would it help If I would rework the patch to use a dummy class and reduce > ifdef's that way? Might make sense here although I personally find code with > Ifdefs clearer to understand then code that behaves differently in the > background. > > Martin Gräßlin wrote: > > Why should we include, build, distribute, include the License etc. Of a > chunk of code that adds no functionality to our software? > > Well it does. The only problem is that this feature is non-functional on > Windows, because it uses DBus and nobody ported the runtime part. Both is > fixable. On Windows it could use whatever Windows needs and you have working > global shortcuts. So of course it adds features. Which is also why I don't > buy into it's a disfunctional KGlobalAccel if we make it not depend on DBus. > It's a step towards working global shortcuts on Windows. > > > Would it help If I would rework the patch to use a dummy class and > reduce ifdef's that way? > > Less ifdefs is in my opinion always an improvement. My main concern in > this review is the ifdef overhead and the danger on breaking on Linux due to > making the dependency optional (sorry I'm a burnt child in that regard, have > seen it happen too often over the last two years even if sent a dedicated > mail to release team to point out about it). Concerning the latter I would > rather go for a no-DBus profile or something like that. A global cmake switch > to build without DBus and disable all frameworks which need DBus. Similar > what we did to disable finding X11 on non-Linux. That way on Linux the > dependency would still be required, because you build with DBus support. > > Andre Heinecke wrote: > > Well it does. The only problem is that this feature is non-functional > on Windows, because it uses DBus and nobody ported the runtime part. Both is > fixable. On Windows it could use whatever Windows needs and you have working > global shortcuts. So of course it adds features. Which is also why I don't > buy into it's a disfunctional KGlobalAccel if we make it not depend on DBus. > It's a step towards working global shortcuts on Windows. > > But I would like to build an KXMLGui application without GlobalShortcut > even if they would work. We could implement them for Windows (without dbus) > but you still would need to have some process handling them. (Even if its the > application itself). I do not want to have that and I do not see that this is > useful to have on Windows for the users of Kleopatra. There are already ways > for Windows how you could do global shortcuts that would work even if the > Application is not running. To include this from a KDE Library would have us > maintain windows specific shortcut code and I don't see anyone who would be > willing to maintain / test / develop something like that. And it's not really > needed there and whatever it would do would be foreign to the platform afaik. > > > Less ifdefs is in my opinion always an improvement. > > I've taken a look at this. Adding a subdirectory kglobalaccel_dummy with > a kglobalaccel.h thats included by cmake in case kglobalaccel is not found. > But I find this worse then then 29 ifdef's currently added. Dummy subclasses > for optional packages are not a pattern we should not establish. I'm not sure > for example how kde apidocs would react to a kglobalaccel class in kxmlgui > for example. Ifdefs make it more clear if some codepaths are optional. > > > My main concern in this review is the ifdef overhead. > > 29 ifdefs in ~15k lines of code are not that exccessive. > > > and the danger on breaking on Linux due to making the dependency > optional (sorry I'm a burnt child in that regard, have seen it happen too > often over the last two years even if sent a dedicated mail to release team > to point out about it). > > Well it is reccommended, as it was a hard depdenency before and is not a > new dependency most packagers would have to actively disable that. If they do > that and loose global shortcuts. Ok. That's their freedom I guess and not > your problem. > > > Concerning the latter I would rather go for a no-DBus profile or > something like that. A global cmake switch to build without DBus and disable > all frameworks which need DBus. Similar what we did to disable finding X11 on > non-Linux. That way on Linux the dependency would still be required, because > you build with DBus support. > > For me it's slightly important that this is not Windows specific so that > I can develop / test with it even under GNU/Linux. And generally DBus still > also makes sense for usecases on Windows. E.g. a Kontact on Windows would > still need functional DBus and such. I think optional is the right way. > CI only for the "recommended" / all optionals found use case would be > fine with me but I think this is unrelated.
whether kglobalshortcuts is functional or not is besides the point: the point is that whether it works or not it doesn't add any functionality to the average application. Global shortcuts are useful only for a very limited set of applications, so it should always have been an optional dependency. - Boudewijn ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126895/#review91623 ----------------------------------------------------------- On Jan. 27, 2016, 8:53 a.m., Andre Heinecke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126895/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2016, 8:53 a.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
