GNOME Bugzilla – Bug 669850
[PATCH] port g-s from CK to systemd
Last modified: 2012-02-22 13:21:13 UTC
Created attachment 207304 [details] [review] gdm: port gnome-shell --gdm-mode to systemd ConsoleKit is obsoleted by systemd-logind. Accordingly, the attached patch ports over the current CK code to systemd. In order to be nice to the Debian people this patch falls back to CK if systemd is not found, so that the code makes the best of whatever it runs on.
Created attachment 207317 [details] [review] automount: port from CK to systemd-logind And here's a second patch, porting over the other CK using bit of gnome-shell: the automounting logic.
Review of attachment 207317 [details] [review]: ::: js/ui/automountManager.js @@ +82,3 @@ + if (haveSystemd()) + this.ckListener = new ConsoleKitManager(); We only init CK if we have systemd?
(In reply to comment #2) > Review of attachment 207317 [details] [review]: > > ::: js/ui/automountManager.js > @@ +82,3 @@ > > + if (haveSystemd()) > + this.ckListener = new ConsoleKitManager(); > > We only init CK if we have systemd? Yes, exactly. We don't need CK on systemd systems. It's what this is about: systemd-logind obsoletes CK so that we don't have to install CK any longer.
(In reply to comment #3) > Yes, exactly. We don't need CK on systemd systems. It's what this is about: > systemd-logind obsoletes CK so that we don't have to install CK any longer. Well right. I just think your logic is backwards. We should only be initializing ckListener if we *don't* have systemd, right?
Review of attachment 207304 [details] [review]: Overall looks good (has some style issues though). I am not sure I really like the "if (Systemd.haveSystemd())" stuff but can't think of a cleaner solution so ... ::: js/Makefile.am @@ +5,3 @@ gdm/batch.js \ gdm/consoleKit.js \ + gdm/systemd.js \ This list is supposed to be alphabetically ordered. ::: js/gdm/powerMenu.js @@ +68,1 @@ + if (Systemd.haveSystemd()) { Hmm calling this all over the place is kind of ugly ... but abstracting it in separate class would just move the ugliness around. @@ +68,2 @@ + if (Systemd.haveSystemd()) { + Useless newline. @@ +84,3 @@ + + } else { + Useless newline. @@ +92,3 @@ + this._haveShutdown = false; + + if (this._haveShutdown) { Don't use curly braces in single line if / else blocks. @@ +107,2 @@ + if (Systemd.haveSystemd()) { + Useless newline. @@ +123,3 @@ + + } else { + Useless newline. @@ +131,3 @@ + this._haveRestart = false; + + if (this._haveRestart) { Don't use braces in single line if / else blocks.
Created attachment 207487 [details] [review] gdm: port gnome-shell --gdm-mode to systemd (In reply to comment #4) > (In reply to comment #3) > > Yes, exactly. We don't need CK on systemd systems. It's what this is about: > > systemd-logind obsoletes CK so that we don't have to install CK any longer. > > Well right. I just think your logic is backwards. We should only be > initializing ckListener if we *don't* have systemd, right? Ouch, you are right. Fixed now.
Created attachment 207489 [details] [review] automount: port from CK to systemd-logind (In reply to comment #5) > ::: js/Makefile.am > @@ +5,3 @@ > gdm/batch.js \ > gdm/consoleKit.js \ > + gdm/systemd.js \ > > This list is supposed to be alphabetically ordered. Fixed. > @@ +68,2 @@ > + if (Systemd.haveSystemd()) { > + > > Useless newline. Fixed. > @@ +92,3 @@ > + this._haveShutdown = false; > + > + if (this._haveShutdown) { > > Don't use curly braces in single line if / else blocks. I explicitly wanted to avoid touching this code, it's the original CK code, just wrapped in an if/else block. If the CK code should be cleaned up like this, this should be done independently of this code I think. Also note that my own code does not use {} for the one-line-ifs.
Review of attachment 207487 [details] [review]: Code looks good, commit message could be improved a bit i.e push with something like: gdm: Port to systemd ConsoleKit is obsoleted by systemd-logind. Accordingly, port the current CK code to systemd. In order to be nice to the Debian people fall back to CK if systemd is not found, so that the code makes the best of whatever it runs on.
Review of attachment 207489 [details] [review]: Looks good.
OK, thanks, description fixed, and commited.