Refactoring KWin’s Workspace

From time to time it happens that I talk with fellow developers who tried to work on KWin. One of the common feedback is that it is difficult to find your way inside KWin, though most is in fact described in the Hacking guide. But when considering our largest class Workspace, I kind of understand why people find it difficult to work on KWin.

The implementation of this class starts with an interesting discussion put into a comment:

Rikkus: This class is too complex. It needs splitting further. It’s a nightmare to understand, especially with so few comments 🙁

Matthias: Feel free to ask me questions about it. Feel free to add comments. I dissagree that further splittings makes it easier. 2500 lines are not too much. It’s the task that is complex, not the code.

I do not know when this comment was added (git blame only allows going back till 2007), but I fully agree with Rikkus on this one, because today it’s not the complex task which is dominating and it’s also not 2500 lines but a little bit more. Also most likely I could not ask Matthias (Ettrich) anything about KWin anymore.

And we actually did something about it. Last year we had a Google Summer of Code project to identify areas which should not belong into Workspace and splitted those out. For example the handling of Screen Edges had been moved into an own class.

But one of the larger tasks which did not get merged during the project was to split out the Compositor from the Workspace class. So far the Workspace had been responsible for both Window Management and Compositing. But on closer look we realize that these are two orthogonal tasks with only very small overlap.

Recently I took up this work and rebased it on current master and started to extend the work. The compositing related code part has always been responsible for creating the Scene (the actual renderer) and the Effect system. The effect system is only initialized if the Scene could be created and when compositing gets stopped both are destroyed. So we have a clear connection between the Compositor and the Effects system and the Scene. Nevertheless both are implemented as a static pointer available to everything in KWin. But nothing outside the Compositor needs the Scene. In fact it was only used as a check whether Compositing is active.

As a result the Scene is now owned by the new introduced Compositor class and I’m working on making the Effects system also completely owned by the Compositor, though for the Effects the static pointer probably has to be kept around. Nevertheless inside KWin there is actually no need to access this pointer outside the Compositor.

But even after moving the Compositor from Workspace into an own class quite a few methods remained in Workspace for the D-Bus interface. When looking through the interface I realized that most of these calls were actually delegating to either the effects system or the compositor. So I decided to also split up this interface and export the effects system and the compositor as own objects to D-Bus. This has also the advantage that we do not offer to e.g. load an effect if the compositor is suspended.

Of course we cannot remove methods from our D-Bus interface as it is kind of our public API to the outside world and we don’t want to break 3rd party application or our own config modules. Given that the split is not sufficient to have less unrelated methods in Workspace. To solve this problem I introduced a very thin wrapper generated from the D-Bus Introspection XML. The implementation of this wrapper just delegates to either the Workspace, Compositor or Effects system. The result is that the Workspace is no longer directly exported to D-Bus and that the unrelated methods could be dropped – they were all unused inside the application. When leaving Qt 4 behind us we can then clean up the D-Bus interface and drop the unrelated methods and the wrapper.

The Compositor has not been the only area where I did some refactoring. Recently I added a new sub-menu to the Alt+F3 menu and was rather annoyed by the process. This menu is also part of Workspace and to implement an addition one has to add a private method, two private slot and two private members. Given the size of the class it’s lot of searching where to add the elements and the names are not the best. Basically one has to namespace the method and variable names to make them non confusing. To make the work even less pleasant the variable names vary in the naming: variable_name vs mVariableName. Personally I don’t mind the way how variables are written, just consistent please.

I was so annoyed working on this code that I decided to never again be annoyed about it and started to move it into an own class. Not only did Workspace get smaller by this change, but also the functionality of the menu is nicely coupled and I was able to rename the variables and methods to have sane and consistent names.

Overall the result of these three topics highlighted in this post are that the header file of Workspace dropped 120 lines of code which are about 10 % of the header’s size. And the two new introduced header both have more than twice the number of lines due to documentation being added to it. To be fair the menu already had documentation, but in Qt style on the implementation and not in the header. Most of our documentation is in the header so this also adds some more consistency.