Code review comment for lp:~laney/lightdm/logind

Hi, thanks for the review.

On Mon, Apr 22, 2013 at 11:23:25PM -0000, Robert Ancell wrote:
> Thanks Iain!
>
> The tests are wrong:
> - You don't need to add a login1 test as it does nothing different to an autologin test, by default all logins will now uses login1 so all the other tests cover this.
> - You need to add back the no-login1 and no-console-kit tests - these test that you can still log in if these services are not available. By removing them there's no checking for these cases

The no-console-kit test was the same as the login1 test. I'll put back the
other name though. I think that came from cavalier's branch.

laney@raring> diff -u no-console-kit.conf login1.conf
--- no-console-kit.conf 2013-04-23 15:18:55.722670000 +0100
+++ login1.conf 2013-04-22 12:05:01.628529093 +0100
@@ -1,5 +1,5 @@
 #
-# Check still works when ConsoleKit is not available
+# Check session can start with logind
 #

 [test-runner-config]

I think that test-no-login1 is the same as test-console-kit? Except with the
difference that console-kit checkes for the cookie being set too.

I have made these changes though; you can fix it up later on if you like.

>
> I think this should say "using login1". This should not be considered an error, use g_debug.
>
> 105 + g_printerr ("Can't suspend using lightdm; falling back to UPower: %s",
> 106 + (*error)->message);
>
> 174 + g_printerr ("Can't hibernate using lightdm; falling back to UPower: %s",
> 175 + (*error)->message);

Fixed. Thanks, good catch - copy&paste error.

>
> Should be inside login1.h, not session.h. Also would prefer to be a function instead of a macro.
>
> 677 +#define LOGIND_RUNNING() (access ("/run/systemd/seats/", F_OK) >= 0)

Moved but I've left it as a macro - this seems to be the standard way being
implemented in GNOME and other places, advocated by pitti and Lennart.

  https://mail.gnome.org/archives/desktop-devel-list/2013-March/msg00092.html

>
> Some minor issues using two spaces for indentation instead of four, use of // for comments instead of /* */.

Fixed some whitespace errors that I (and emacs) found. Not sure I caught them
all though.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]