libs/kworkspace/kdisplaymanager.cpp mess

[Martin Briza]

On Tue, 30 Apr 2013 17:33:57 +0200, Aaron J. Seigo <aseigo at kde.org> wrote:

> On Thursday, April 25, 2013 15:11:25 Martin Briza wrote:
>> I would leave creating the CK module to somebody who is experienced with
>> how exactly the whole system works and knows if it is safe.
>
> if it is a refactor, this should be a matter of moving the CK code as-is  
> into a new file. if the refactor
> requires significant changes to the existing design of class in terms of  
> how it works, then it's
> probably not a refactoring :)

I'm still talking about refactoring. I'm a bit afraid of moving the CK  
code though as I'm not 100% sure all the functionality needed is actually  
there and would work as it does now. That's the reason I'm proposing the  
change with temporarily leaving CK support in the state as it is now.
To avoid misunderstanding (I'm quite convinced I'm not able to make myself  
clear enough due to my English): this doesn't break anything nor introduce  
any kind of regression, the functionality stays the same.

>>   - The KDisplayManager class is used only on a few places so replacing  
>> its
>> constructions with calls to the factory will be easy.
>
> i don't think KDisplayManager's public API needs to be changed in any  
> way.

The API itself wouldn't change. I'm just proposing usage of a singleton  
factory class to have only one instance of the KDisplayManager class  
instead of constructing the object all over. But say the word and I  
implement it as you suggested below.

> what would be nice is to have an abstract base class and have a subclass  
> for each of the session
> management methods. then in KDisplayManager's ctor it can decide which  
> is the correct backend
> to implement.
>
> right now the pattern is sth like:
>
> void KDisplayManager::someAction()
> {
>      switch (DMType) {
>           case NewKDM:
>                 ... some kdm specific code ...
>           case NewGDM:
>                 ... some gdm specific code ...
>      }
> }
>
> very non-object oriented, but made sense given what it started out. now  
> .. as you noticed .. not so
> much. :) with an ABC that defines an interface containing all the  
> actions (e.g. canShutdown, etc.)
> the pattern would then be sth like:
>
> void KDisplayManager::someAction()
> {
>      d->backend->someAction();
> }
>
> and polymorphism would take care of having the right code be called.

Yes, I totally agree with the abstract base class idea, though I still  
think having a private subclass is a bit messy practice. Yet once again,  
I'm here to listen and learn, so the final word on this topic is up to you.

> each backend could go into its own file (though i imagine some backends  
> will end up sharing
> code, cover multiple cases and/or subclass other backends). in the  
> source tree, they could all go
> into a subdir of libs/kworkspace/ to keep it tidy.

Yes, I'm thinking about leaving the old code behind in the abstract parent  
class to leave the choice of which methods to override to the backends  
itself.

> i would probably also drop the oldKDM and oldGDM paths.

If anyone isn't opposed because backwards compatibility, I can only agree.

> i assume you'll implement this in a feature branch to make discussing  
> during devel easy?

Of course.