Review Request 126895: Make KGlobalAccel dependency in KXmlGui optional

[Andreas Hartmetz]

On Donnerstag, 28. Januar 2016 12:07:54 CET Andre Heinecke wrote:
> Hi,
> 
> On Wednesday 27 January 2016 21:28:36 Andreas Hartmetz wrote:
> > On Mittwoch, 27. Januar 2016 17:21:33 CET Boudewijn Rempt wrote:
> > [snip]
> > 
> > > 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.
> > 
> > Here is something that is never beside the point:
> > maintenance burden and continuous-integration-ability.
> > ifdefs are somewhat bad for maintainability and really, really bad
> > for a continuous integration systems's ability to detect relevant
> > build breakage. I am repeating Martin's argument here with
> > different words.
> To repeat our argument (yay) we feel that we need to have the KXmlGui
> library as one of the core libraries for "KDE ishness" not to always
> depend on KGlobalAccel on all platforms as other Platforms have
> different concepts for Global shortcuts and they don't make sense for
> our users that we see.
> 
> Wrt maintenance burden / CI effort. Please you can say you only
> maintain and Test the recommended usage. Our alternative is that we
> have to Patch it out then we have to maintain the patches and
> basically a fork.
> 
> Also this is really a very simple patch. It's not much different
> behavior it just does not allow you to configure Global Shortcuts
> through KxmlGui.
> > The most maintainable way to make a feature optional is to disable
> > it at runtime.
> 
> I disagree with that. Runtime dependencies are more likely to cause
> bugs as a developer can not easily see / know how a function behaves
> when writing code.
> 
> Runtime dependencies also don't make sense for single Application
> deployments as we control all dependencies. The way it was in the
> past users had a Global Shortcuts configuration entry through KXmlGui
> which was always dysfunctional because we did not ship the necessary
> runtime components.
> 
> And KGlobalAccel is not the only thing. I don't want to see
> spellchecking entires either that won't work because we have not
> installed a spellchecking libary or dictionaries. Etc.
> 
> > Only when this severely hurts performance or a feature plain
> > won't build on a certain platform should it be disabled via ifdefs,
> > and in the latter case, maybe it can be put into its own class that
> > gets a real and a dummy implementation.
> 
> I dislike the dummy implementation. This makes code harder to
> understand and is more effort to maintain then simple harsh ifdefs.
> 
> It's also not a pattern already established for optional library
> depdenencies. It's established in the form of different
> implementations which might be dummies in a single library for
> different platforms. But to have dummy classes for optional
> dependencies is something different and afaik not established in KDE.
> 
> > My argument is first and foremost but for putting the knife where
> > the
> > least amount of stuff needs to be cut,
> 
> I think that XMLGui is the right place for this. XMLGui is a useful
> library without Global Shortcuts. It's very little code that needs
> GlobalAccel and it was easy to cut it out. There were not even any
> header / API changes neccessary for this.
> 
> > even if that results in shipping
> > an unnecessary (small!) module.
> 
> I fail to see how this would be advantageous at all. Having code in a
> library that allows it to be dysfunctional is wrong. And I would not
> propose making such a change in GlobalAccel. Why should anyone would
> want to waste resources for something that does not add anything at
> all.
> 
> Shipping a library means that we have to include it in our build
> system, we have to update it regularly, we have to fix build errors
> for our platform, we have to ship the license, we have to include the
> source code in our source distribution etc.
> 
> > Maybe there is a better solution, though.
> > How about a dummy KGlobalShortcuts implementation in KXmlGui? There
> > may be a few places where ifdefs are still required (e.g.
> > completely hiding - and I don't mean removing - inert UI), but the
> > vast majority of things in KGlobalShortcuts can be faked very
> > easily by doing nothing at all. Most code paths don't need to be
> > removed if there is simply no way to trigger them.
> 
> See above. A dummy library would imho be a more invasive change and
> harder to maintain then the ifdefs.
> 
I strongly disagree with that. If you have ever written unit tests, you 
know how trivial a fake implementation can be. Almost everything that is 
difficult about implementing anything does not apply in a fake 
implementation. You don't really need a "fake library" here, you just 
need the few public methods that KXmlGui calls. And they should do 
basically nothing.

> > The thing to be faked is a system that has no global shortcuts and
> > where setting them somehow always fails. Maybe pretend to
> > applications that it doesn't fail, shouldn't really matter.
> 
> Why would you want to do that? The only thing it adds is confusion for
> developers and users imho.
> 
> Regards,
> Andre