Bug 40020 - [PATCH] Add support for LightDM
Summary: [PATCH] Add support for LightDM
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
Depends on:
Reported: 2011-08-11 09:33 UTC by Michael Terry
Modified: 2018-02-22 04:04 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:

Proposed patch (6.03 KB, patch)
2011-08-11 09:43 UTC, Michael Terry
Details | Splinter Review
Patch #2 (5.95 KB, patch)
2011-09-09 08:17 UTC, Michael Terry
Details | Splinter Review

Description Michael Terry 2011-08-11 09:33:47 UTC
Hello!  I've working on adding support to accountsservice for LightDM, which Ubuntu is switching to starting with Ubuntu 11.10.

Currently, accountsservice assumes GDM is in use.  (It matters only for saving and loading autologin settings.)

This patch will do two things:
 1) When saving autologin settings, try to write to both GDM and LightDM config files, if they are available.

 2) When loading autologin settings, try to determine which of GDM and LightDM are running and load from its respective config file.
Comment 1 Ray Strode [halfline] 2011-08-11 09:43:24 UTC
That's probably okay.

Maybe a better approach, though, would be to get away from DM specific code and make accounts service manage its own configuration that the DMs use instead of their own configuration.

Comment 2 Michael Terry 2011-08-11 09:43:30 UTC
Created attachment 50137 [details] [review]
Proposed patch

One notable thing about this patch is how it checks whether GDM is running.  It currently checks if org.gnome.DisplayManager is owned.  Is there a better way?
Comment 3 Ray Strode [halfline] 2011-08-11 09:48:16 UTC
(In reply to comment #2)
> Created an attachment (id=50137) [details]
> Proposed patch
> One notable thing about this patch is how it checks whether GDM is running.  It
> currently checks if org.gnome.DisplayManager is owned.  Is there a better way?

org.gnome.DisplayManager seems reasonable to me.

Other than that, are you happy with your patch?
Comment 4 Michael Terry 2011-08-11 09:52:59 UTC
As for the patch, I have done some quick tests and it seems to work for me.  I'll likely play with it another day or so and make sure I don't hit any issues.  So if you want to hold off until I give a more resounding vote of QA confidence, that's reasonable.

As for config files, I'm fine with either way (accountsservice tweaking others' files or having its own), but I'm not a main LightDM or GDM developer so can't really speak to how easy a central config file would be to adopt.

This is just one config value (autologin user).  If that's all that's envisioned, I'm not sure it's worth having a special config file.  But if you plan to add more display manager settings to accountsservice, maybe it starts to make sense.
Comment 5 Ray Strode [halfline] 2011-08-11 09:55:36 UTC
alright, let's go with this approach for now, but reconsider if we end up moving more options (like include/exclude lists?) or gain interest from other DMs
Comment 6 Matthias Clasen 2011-08-15 05:12:23 UTC
Fwiw, I've always considered the current 'gdm support' a hack that should go away when gdm starts talking to accountsservice. It makes a lot more sense for the accountsservice to store this information itself, and for interested DMs to query it.
Comment 7 Robert Ancell 2011-08-17 18:22:42 UTC

As the LightDM maintainer I very much support the idea of having LightDM reading configuration from the accounts service.

I am currently gauging interest on if other desktops will adopt AccountsService:
Comment 8 Martin Pitt 2011-09-09 04:06:45 UTC
Mike, the patch doesn't apply any more to 0.6.14. Do you mind porting it? Are you happy with how well it works?
Comment 9 Michael Terry 2011-09-09 08:17:30 UTC
Created attachment 51002 [details] [review]
Patch #2

Here's an updated patch for 0.6.14 (and it fixes a bug with detecting LightDM's autologin status).  Works for me now.
Comment 10 Matthias Clasen 2011-10-17 12:50:33 UTC
Comment on attachment 51002 [details] [review]
Patch #2

Review of attachment 51002 [details] [review]:

::: src/daemon.c
@@ +1446,5 @@
> +{
> +        if (!save_autologin_gdm (daemon, name, enabled, error))
> +                return FALSE;
> +        if (!save_autologin_lightdm (daemon, name, enabled, error))
> +                return FALSE;

Hmm, I feel like this should always try both save functions, if we don't want to look for which dm is actually in use.

ie it should be something like

result = TRUE;
result &= save_autologin_gdm(...);
result &= save_autologin_lightdm(...);
return result;
Comment 11 Alessio Treglia 2012-04-30 02:26:23 UTC
Hi all!

Any news on this?
Comment 12 Matthias Clasen 2012-05-15 06:23:16 UTC
Comment #6 still stands - this needs to be flipped around. Make 'autologin=yes' per-user information that is provided by the accountsservice, and make interested display managers use that information.

Adding code for reading and writing all the display managers config file formats is not a sane or scalable approach.
Comment 13 Robert Ancell 2018-02-22 04:04:55 UTC
LightDM now uses the AccountsService extension mechanism which obsoletes the majority of this bug.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.