> 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.
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. - Boudewijn ----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
