Review Request 126895: Make KGlobalAccel dependency in KXmlGui optional
Kåre Särs
kare.sars at iki.fi
Fri Jan 29 08:11:33 UTC 2016
> 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.
>
> Boudewijn Rempt wrote:
> 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.
>
> Martin Gräßlin wrote:
> > If they do that and loose global shortcuts. Ok. That's their freedom I guess and not your problem.
>
> Of course it's my problem. I'm the maintainer, I get the users bug reports. And that's what's happening. Look for example at https://bugs.kde.org/show_bug.cgi?id=356318 which I investigated yesterday - distro did incorrect packaging, bug report ended on my plate. As I said: I'm a burnt child here. That's not an issue that happens once, it's a problem which happens constantly over all distributions. Making the dependency optional or recommended will mean distros will ship without it. It's just something we need to consider here. It's not something I make up. This is going to happen. I can promise you. And you can imagine what kind of mess it is to investigate such "impossible to happen" bugs.
>
> That's why the build dependency needs to be handled gracefully. Did you consider the "No DBus" profile thing in ECM. An easy way to disable all dependencies which require DBus. So that it's only not required if build on that profile.
>
> Similarily I will promise you that the ifdefs will break at some point in one of the two configurations if we don't have
> a) build tests
> b) actual runtime tests for that.
>
> Seen it all, have maintained enough frameworks with multi platform code ;-)
>
> Andre Heinecke wrote:
> > Of course it's my problem. I'm the maintainer, I get the users bug reports. And that's what's happening. Look for example at https://bugs.kde.org/show_bug.cgi?id=356318 which I investigated yesterday - distro did incorrect packaging, bug report ended on my plate. As I said: I'm a burnt child here. That's not an issue that happens once, it's a problem which happens constantly over all distributions.
>
> This is a structurual problem imho that you get these bug reports. (And I know the problem any problem in Gpg4win is reported to Kleopatra ;-)) They should have been reported to opensuse, an opensuse maintainer should have investigated this. And if I understand it right this bug is about the case you are arguing for an optional runtime dependency that created problems when it wasn't available.
>
> > distro did incorrect packaging, bug report ended on my plate.
>
> There is no correct or incorrect packaging here. Either users will get the Feature to configure Global Shortcuts through KXMLGui or they won't.
>
> > Making the dependency optional or recommended will mean distros will ship without it
>
> Yes. One of the largest KDE Application Distribution on Windows (Gpg4win) _wants_ to ship without this dependency! :-) (And another, Krita, already does.)
>
> > That's why the build dependency needs to be handled gracefully.
>
> Of course my goal here is not to produce a buggy library in case GlobalAccel is not available at build time. But a libary that is stable and completely fine to use for KDE Applications that don't use Global Shortcuts through KGlobalAccel.
>
> > Did you consider the "No DBus" profile thing in ECM. An easy way to disable all dependencies which require DBus. So that it's only not required if build on that profile.
>
> I don't see how this would help. You would still need the optional handling this patch is about. No?
>
> > Similarily I will promise you that the ifdefs will break at some point in one of the two configurations if we don't have
>
> This is totally fine with me. The alternative if I don't get a "ship it" in this review is that I will have to patch this. This will break even more likely.
>
> > Seen it all, have maintained enough frameworks with multi platform code ;-)
>
> Please also believe me (and Boudewijn) as the packagers responsible for large deployments on one of these mutli-platforms that the huge amounts of dependencies a KDE Application entailed are (were) a big problem for deployment / packaging. I hoped this would be solved by Frameworks. And in really large parts this has been true and is awesome. Thanks to Frameworks i can see a clear path to be able to build my KDE Application only with useful dependencies. But I'd like to see the problems that are still there for me (like this one) solved upstream instead of having to patch around them.
>
>
> How can we move forward here? There has been a +1 from Aleix and strong support from Boudwijn.
>
> But there has also have been arguments from you here and from Andreas Hartmetz on the mailing list against this patch. What is the process to resolve such a conflict?
>
> Are you strongly against this patch?
>
> Martin Gräßlin wrote:
> > And if I understand it right this bug is about the case you are arguing for an optional runtime dependency that created problems when it wasn't available.
>
> Runtime/build time doesn't matter in this case. The problem is that a dependency was optional and distros currently don't handle that. I could show you cases where I sent out a mail to kde-release-team telling the distros which dependency they need to include and they got it wrong nevertheless. That's just the current state on Linux currently: optional dependencies don't work.
>
> > There is no correct or incorrect packaging here. Either users will get the Feature to configure Global Shortcuts through KXMLGui or they won't.
>
> Of course there is. On Linux building without Global Shortcut in KXMLGui will break existing software on the platform Plasma. That's the problem I try to bring here. The fact that the change as suggested here will break software running on Plasma and that those bugs will go to me. Nobody will report a bug report saying "Xmlgui is build without KGlobalaccel", it will be "yakuake doesn't work".
>
> > I don't see how this would help. You would still need the optional handling this patch is about. No?
>
> No. The difference is that it would only be optional if in the profile. This would solve the "linux distro issue" as they would never ever set this profile.
>
> > Are you strongly against this patch?
>
> I think we need to handle the two cases. I understand your need to not require it, I and others provided several different ideas in this thread including:
> - global cmake profile switch to disable DBus
> - changing kglobalaccel to not use DBus
> - having a no-op kglobalaccel in xmlgui
>
> Given that I doubt that going strictly on this must be optional and ifdef is the only choice is true. I am convinced that there is a middle ground which solves the requirements.
>
> Fixes to deployment issues on Windows shouldn't create deployment issues on other platforms. The patch as it is right now is going to create deployment issues on Linux.
>
> Andre Heinecke wrote:
> Ok. Let's say that on Linux GlobalAccel support in KXmlGui is so important that losing it would constitute a bug in itself. So we should not give the option to packagers not to include this because not including it would automatically break a core feature of KXmlGui.
>
> The obvious compromise I see with that position is to keep it required for Linux and make it optional for other platforms which might have different Global Shortcut concepts.
>
> Martin Gräßlin wrote:
> what about the profile approach through ECM? Don't you think it's better to have that defined as a global thing?
>
> To explain why I am nagging about it: some time ago the non-Linux systems started to add code like
> if (NOT APPLE)
> find_package(X11)
> endif()
>
> I pointed out that this is wrong, etc. etc. I got ignored and now we have all over the place these things. But nowadays we also have a proper solution in ECM. And we need to remove all those custom checks again.
>
> I don't want to repeat this mistake. I want to have it from the start that we have it working correctly without needing patches all over the place. That's why I suggest the "No DBus profile". A global thing to say "hey I don't want any dependencies which depend on DBus".
>
> Andre Heinecke wrote:
> We've discussed this in IRC as I was not really clear about what this would mean.
>
> As I see it now it would mean that globally CMAKE_DISABLE_FIND_PACKAGE is set for Qt Dbus.
> In this case this would not help much because for it work the packages already need to be optional. Which is what this patch is about.
> The use case for that which i see is that you can build things without optional dependencies even if they are available. This is not a Problem in the usual KDE-Windows build setups. So there won't be patches like:
> if (NOT WIN32)
> find_package (KGlobalAccel)
> endif()
> Where indeed the ECM Profile approach would be better.
>
> But in this patch this would allow replacing the if (UNIX) check added in my latest change by if (BUILDING_WITH_DBUS) or something. I've argued that even if you build with DBus (as kdepim, and the general kde-windows applications do) you might want to have KGlobalAccel optional on a foreign platform that might have other shortcut concepts.
>
> My general goal is to have more options and one of those options is to build some frameworks without DBus. We would also have the same discussion with another patch that [Makes KTextWidgets optional](https://phabricator.kde.org/file/data/toramjtxpqos7pjd6lja/PHID-FILE-hmizzhkgp6mpz6xgaaon/0003-Make-KTextWidgets-optional.patch)
> Which is unrelated to the DBus as there it's more about avoiding KService and Spellchecking / dictionaries.
>
>
> Martin: Do you think we need more discussion about the general merits of optional dependencies? My current tendency is to just guard them with if (UNIX) when we think they are neccessary for the Plasma Desktop usecase.
>
> For the three patches I have against XmlGui this would mean to me:
> - KGlobalAccel only optional on Non-Unix
> - DBus only optional on Non-Unix (KMainWindow / Ktoolbar dbus integration)
> - KTextwidgets always optional (Spell checking in kbugreport) (or just completely dropped but this will be another review)
>
> Martin Gräßlin wrote:
> > Do you think we need more discussion about the general merits of optional dependencies?
>
> yes, please. We need to find a solution which scales and doesn't run into the risk of not being picked up by distros. Also it needs to be semantically correct. There are probably many approaches. One thing that comes to my mind just now is a -DFRAMEWORKS_BUILD_MINIMAL which would disable all not required dependencies.
>
> Boudewijn Rempt wrote:
> Marijn created and I maintained a MINIMAL_BUILD option for kdelibs4, actually. I never dared propose to add it to regular kdelibs4, it was extremely invasive.
What about CMake option() to disable just specified features in stead of "if (UNIX)"? That would make the "default" REQUIRED and if you know you do not want the feature you explicitly disable it without the need for patches.
A global MINIMAL_BUILD option would IMO not be optimal. As an example on Windows Kate does not benefit from KGlobalAccel, but DBus does give the option to enable "Open only one Kate instance".
- Kåre
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126895/#review91623
-----------------------------------------------------------
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160129/14922bf5/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list