Re: libs/kworkspace/kdisplaymanager.cpp mess
Am Dienstag, 7. Mai 2013, 22:21:46 schrieb Àlex Fiestas: Would be nice to have some kind of unittests on this, that will make a different with the current implementation (which afaik it has none) Even better would be to have them first to prove that nothing changed afterwards ;) Eike signature.asc Description: This is a digitally signed message part.
Re: libs/kworkspace/kdisplaymanager.cpp mess
Would be nice to have some kind of unittests on this, that will make a different with the current implementation (which afaik it has none) On Mon, May 6, 2013 at 4:14 PM, Martin Briza mbr...@redhat.com wrote: Well, as I said last week, I started working on it. First few commits are in branch mbriza/kdisplaymanager-split. Currently, PLEASE, don't judge me for the KDMBackendPrivate class stuff. :) I have just cut that out of KDisplayManager, not really knowing what to do with it further. I'll try to merge it in the code tomorrow in a fashion that doesn't look so horrible and is shared only for the classes which really do need it. I'm pretty sure it doesn't work in this state because all the initializations (and static storage of the pointers to the backends) aren't complete yet. But it at least compiles. Regarding design: I removed all OldGDM and OldKDM code. In the end, it seems two backends will be needed: * One to communicate with something that manages sessions (systemd-logind, CK, the DM itself) to be able to switch them. I'm naming the classes somethingSMBackend - SM as for Session Manager. * The second to be able to control the DM to make it possible to command it to create a new greeter on an other VT. The classes are called somethingDMBackend - as for Display Manager. Currently, I have added the following SM Backends: Null, Login1, ConsoleKit and Basic (contains all the previous code) and the following DM Backends: Null, DBus (previously called LightDM), Basic (again, with the old code). What I'm not sure about are inheritance chains. I think it would be nice to be able to use multiple backends when it's possible, for example we can try to shut down the computer using systemd-logind, if it fails, there still can be CK and when both fail, there's still possibility to call shutdown directly. Of course there's still a lot of mess left and coding standards aren't exactly followed but I'll fix that when the work is done completely.
Re: libs/kworkspace/kdisplaymanager.cpp mess
Well, as I said last week, I started working on it. First few commits are in branch mbriza/kdisplaymanager-split. Currently, PLEASE, don't judge me for the KDMBackendPrivate class stuff. :) I have just cut that out of KDisplayManager, not really knowing what to do with it further. I'll try to merge it in the code tomorrow in a fashion that doesn't look so horrible and is shared only for the classes which really do need it. I'm pretty sure it doesn't work in this state because all the initializations (and static storage of the pointers to the backends) aren't complete yet. But it at least compiles. Regarding design: I removed all OldGDM and OldKDM code. In the end, it seems two backends will be needed: * One to communicate with something that manages sessions (systemd-logind, CK, the DM itself) to be able to switch them. I'm naming the classes somethingSMBackend - SM as for Session Manager. * The second to be able to control the DM to make it possible to command it to create a new greeter on an other VT. The classes are called somethingDMBackend - as for Display Manager. Currently, I have added the following SM Backends: Null, Login1, ConsoleKit and Basic (contains all the previous code) and the following DM Backends: Null, DBus (previously called LightDM), Basic (again, with the old code). What I'm not sure about are inheritance chains. I think it would be nice to be able to use multiple backends when it's possible, for example we can try to shut down the computer using systemd-logind, if it fails, there still can be CK and when both fail, there's still possibility to call shutdown directly. Of course there's still a lot of mess left and coding standards aren't exactly followed but I'll fix that when the work is done completely.
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Tue, 30 Apr 2013 17:33:57 +0200, Aaron J. Seigo ase...@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.
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Tue, 30 Apr 2013 17:35:01 +0200, Aaron J. Seigo ase...@kde.org wrote: On Thursday, April 25, 2013 15:11:25 Martin Briza wrote: - The KDisplayManager class is used only on a few places so replacing its constructions with calls to the factory will be easy. oh, btw.. the LightDM code there is also shared by SDDM. calling it LightDM is a bit of a misnomer now as a result. might be better to call it something related to the DBus interface it implements? Yes, I actually like this approach to DM interfacing a lot. I'd like to stick to something related to the interface's name as you wrote, but org.freedesktop.DisplayManager is a tad long and calling it just DisplayManager is too general...
Re: libs/kworkspace/kdisplaymanager.cpp mess
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 :)
- 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.
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.
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.
i would probably also drop the oldKDM and oldGDM paths.
i assume you'll implement this in a feature branch to make discussing during
devel easy?
--
Aaron J. Seigo
signature.asc
Description: This is a digitally signed message part.
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Thursday, April 25, 2013 15:11:25 Martin Briza wrote: - The KDisplayManager class is used only on a few places so replacing its constructions with calls to the factory will be easy. oh, btw.. the LightDM code there is also shared by SDDM. calling it LightDM is a bit of a misnomer now as a result. might be better to call it something related to the DBus interface it implements? -- Aaron J. Seigo signature.asc Description: This is a digitally signed message part.
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Thu, Apr 25, 2013 at 03:11:25PM +0200, Martin Bříza wrote: after fixing a bit in the $subj file I've realized it (in my opinion) should be split into one abstract class with a factory handling the back-ends provided by the current session managers such as ConsoleKit and systemd-logind while providing one module for the legacy DMs with their socket communication protocols. To provide reasons why to do it: - The current state is nearly unmaintainable that tiny file? you have a low threshold. ;) and having all three (for now) ways of session handling in one file doesn't feel very clean. it was the best at the time of creation due to the amount of shared code (the old gdm and kdm socket code was identical except for the string literals and a few conditionals). the ck code was tacked on later. at this point the old gdm code can be probably purged, making the kdm code independent. you should do that in a separate first commit. the ck and systemd paths may share some code, though. maybe add a DBusUtils class. 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. good luck with that. i'll do reviews, not a bit more. note that partial works (be it regressions or just a highly asymmetrical code structure) will not be accepted. if you don't find somebody to do the refactoring for you, you need to take the route of minimal modification.
Re: libs/kworkspace/kdisplaymanager.cpp mess
Hi, On Fri, 26 Apr 2013 23:30:43 +0200, Albert Astals Cid aa...@kde.org wrote: El Dijous, 25 d'abril de 2013, a les 15:11:25, Martin Briza va escriure: after fixing a bit in the $subj Which repo is that? Are we installing that file header? The systemd support fix is now in master branch, if you meant that. Header of the library is public, yes. Kind Regards, Martin
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Mon, Apr 29, 2013 at 03:48:35PM +0200, Martin Briza wrote: On Mon, 29 Apr 2013 09:17:15 +0200, Oswald Buddenhagen o...@kde.org wrote: note that partial works (be it regressions or just a highly asymmetrical code structure) will not be accepted. if you don't find somebody to do the refactoring for you, you need to take the route of minimal modification. It will work the same as it does now but I'll split the systemd code away from the main (let's say legacy) part of the code. this is what i mean with highly asymmetrical. no-go.
Re: libs/kworkspace/kdisplaymanager.cpp mess
Le Monday 29 April 2013 15:47:40 Martin Briza a écrit : Hi, On Fri, 26 Apr 2013 23:30:43 +0200, Albert Astals Cid aa...@kde.org wrote: El Dijous, 25 d'abril de 2013, a les 15:11:25, Martin Briza va escriure: after fixing a bit in the $subj Which repo is that? Are we installing that file header? The systemd support fix is now in master branch, if you meant that. Header of the library is public, yes. I guess Albert was wondering about the git repository. This file is in the kde-workspace git repo. Aurélien
libs/kworkspace/kdisplaymanager.cpp mess
Hi everyone, after fixing a bit in the $subj file I've realized it (in my opinion) should be split into one abstract class with a factory handling the back-ends provided by the current session managers such as ConsoleKit and systemd-logind while providing one module for the legacy DMs with their socket communication protocols. 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. The systemd module would be done by me as all the legacy methods of session management are not necessary with it. To provide reasons why to do it: - The current state is nearly unmaintainable and having all three (for now) ways of session handling in one file doesn't feel very clean. - The KDisplayManager class is used only on a few places so replacing its constructions with calls to the factory will be easy. - Of course there will be a bit less D-Bus calls and a little overall performance improvement (but I doubt it'll be noticeable). What I'm asking is if anyone's opposed or has any objections please, before I split the file and open the review request itself. Kind regards, Martin
Re: libs/kworkspace/kdisplaymanager.cpp mess
El Dijous, 25 d'abril de 2013, a les 15:11:25, Martin Briza va escriure: Hi everyone, after fixing a bit in the $subj Which repo is that? Are we installing that file header? Cheers, Albert file I've realized it (in my opinion) should be split into one abstract class with a factory handling the back-ends provided by the current session managers such as ConsoleKit and systemd-logind while providing one module for the legacy DMs with their socket communication protocols. 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. The systemd module would be done by me as all the legacy methods of session management are not necessary with it. To provide reasons why to do it: - The current state is nearly unmaintainable and having all three (for now) ways of session handling in one file doesn't feel very clean. - The KDisplayManager class is used only on a few places so replacing its constructions with calls to the factory will be easy. - Of course there will be a bit less D-Bus calls and a little overall performance improvement (but I doubt it'll be noticeable). What I'm asking is if anyone's opposed or has any objections please, before I split the file and open the review request itself. Kind regards, Martin
