GNOME Bugzilla – Bug 671858
port user panel to use libaccountservice
Last modified: 2013-01-03 16:38:44 UTC
Created attachment 209458 [details] [review] patch Be nice to distributions where systemd is optional. This will select the code path at runtime depending on whether the system was booted using systemd.
See the comments in bug 671861
Should use the API in bug 687821 instead.
It seems like the API in bug 687821 won't work here. That API is for determining if we're in the active session whereas the code here is trying to discover if another user has an active session...
This patch would need to be rebased on top of master, in combination with reverting f6cc4a970a98f82a2eb1c70515648869bc8bdb90 .
Review of attachment 209458 [details] [review]: ::: panels/user-accounts/um-user.c @@ +1023,3 @@ GVariant *result; GVariantIter *iter; gint n_sessions; Minor: these declarations should be moved up to the top of the block; the rest of the code is C89, so let's not add C99 here.
I think a better way to solve this problem is to toss out the home-grown copy (or sibling) of libaccountservice we have in g-c-c and use the real deal directly. I'm attaching a patch to libaccountservice to add some API that we need and then a patch for g-c-c that uses that API. Both are very much WIP.
Created attachment 229058 [details] [review] wip: new API for g-c-c port
Created attachment 229059 [details] [review] wip: port user panel to libaccountservice
Review of attachment 229058 [details] [review]: Looks good to me - do you have commit access on f.d.o ?
I've pushed the accountsservice patch to http://cgit.freedesktop.org/accountsservice/log/?h=async-api It would be great if somebody could double-check that I didn't mess up the backport from GTask to GSimpleAsyncResult - I don't want to depend on unstable glib api in accountsservice
Comment on attachment 229058 [details] [review] wip: new API for g-c-c port libaccountsservice api included in 0.6.27
Created attachment 229330 [details] [review] tested patch Here is another iteration of Ryans patch, which has been tested to work with accountsservice 0.6.28
Created attachment 229331 [details] [review] require a new enough accountsservice
*** Bug 688953 has been marked as a duplicate of this bug. ***
Created attachment 230782 [details] [review] updated patch a few changes to master have broken the old patch. here's a rebased one. it depends on a non-existent act_user_manager_get_user_by_id() though... this is required by the recent last-login-time work... it may be possible to work around that.
The patch looks good to me - without trying it. Did you have a patch for get_user_by_id ?
no... The patch still has the commented-out error checking code from before. Probably it should be removed.
I've written and committed act_user_manager_get_user_by_id now, and I'm afraid to say this patch doesn't currently work - it breaks changing the selected user. The control-center just freezes when I click on another user
Review of attachment 230782 [details] [review]: needs work
Created attachment 230963 [details] a log
Mmh, I spent some time on this, but the libaccountsservice startup code is a mess of async requests. I've found one issue so far: We don't remove queued fetch user requests when the user is loaded before the service is marked as loaded. This later causes us to remove_user/add_user the user, which confuses the user panel, since it moves the selection when the user is removed, and then doesn't know to move it back when the user is re-added. I have a patch which fixes this, but now I don't get the green bullet on my own user anymore, because the library loads sessions too early, and fails to associate them with the not-yet-loaded user. More debugging needed. But not by me, tonight.
Created attachment 231017 [details] [review] remove queued user requests
Hey, (In reply to comment #18) > I've written and committed act_user_manager_get_user_by_id now I had a look at that: http://cgit.freedesktop.org/accountsservice/commit/?id=729a2feb54dbbaeea051f36ff81093dc682202f3 I don't think it's quite right, since it never calls FindUserById, it will only work if the user is already cached by the daemon and has an associated object path. I'm going to try the stuff mentioned in this bug, too, and will report back.
I've pushed some fixes to account service git now: http://cgit.freedesktop.org/accountsservice/commit/?id=40902c369ef28d95b802e0d11ddf817d590e72ff http://cgit.freedesktop.org/accountsservice/commit/?id=f28d5bcb9e55a27fad6096c19141e8b54eadf958 http://cgit.freedesktop.org/accountsservice/commit/?id=acb79f31770318f461f2dec95d0ba971cbfbc177
Review of attachment 230782 [details] [review]: one thing first: ::: panels/user-accounts/um-user-panel.c @@ +924,3 @@ g_debug ("Got %d users\n", g_slist_length (list)); g_signal_connect (d->um, "user-changed", G_CALLBACK (user_changed), d); Also need to connect to "user-is-logged-in-changed"
Comment on attachment 231017 [details] [review] remove queued user requests (this is fixed a different way now)
Is there anything left to be done on the accounts service side? Will we get an updated patch here?
Yea i plan to update the patch here. i had to put it down for a couple of days but will get back to it soon.
Created attachment 232092 [details] [review] user-accounts: port to libaccountsservice This removes a bunch of duplicated code, and also drops a direct dependency on libsystemd-login.
Ok, I had some trouble getting this patch to apply to master. After forcing it, it turned out that the accountsservice code in 0.6.30 is even more confused than before :-( If I create a new account, it does not get added to the list anymore. This seems to be because ActUserManager::user-added is never emitted due to confusion in various overlapping state machines. I see two calls to act-user-manager.c:create_new_user. The first is coming from on_new_user_in_accounts_service; the second coming from act_user_manager_create_user_finish. The two are probably stepping on each others toes and prevent ActUser::is-loaded from ever being set.
Created attachment 232602 [details] [review] user-accounts: port to libaccountsservice This removes a bunch of duplicated code, and also drops a direct dependency on libsystemd-login. Updated for latest control-center
I've ported it to the latest control-center master above. I'm not hitting the problems you're talking about though. I'll grab you tomorrow and see what's going on. To be sure, you killed the old accountsservice after upgrading? It is possible to have more than one ActUser object with the same object path in play at once.
patch doesn't apply here
(In reply to comment #32) > I've ported it to the latest control-center master above. I'm not hitting the > problems you're talking about though. I'll grab you tomorrow and see what's > going on. > > To be sure, you killed the old accountsservice after upgrading? We need to make sure that it fails gracefully with older versions of accountsservice.
Comment on attachment 232602 [details] [review] user-accounts: port to libaccountsservice Marking as needs-work as it doesn't apply.
odd, attachment 232602 [details] [review] isn't what's in my local branch. I must have had some sort of git branching snafu
Created attachment 232643 [details] [review] user-accounts: port to libaccountsservice This removes a bunch of duplicated code, and also drops a direct dependency on libsystemd-login.
Comment on attachment 232643 [details] [review] user-accounts: port to libaccountsservice Still doesn't apply to master.
weird, i'll just attach it manually instead of using git bz
Created attachment 232646 [details] [review] user-accounts: port to libaccountsservice This removes a bunch of duplicated code, and also drops a direct dependency on libsystemd-login.
Review of attachment 232646 [details] [review]: Rest looks fine, and seems to work in my limited testing (adding/removing users, and the likes). ::: panels/user-accounts/um-password-dialog.c @@ +199,3 @@ + switch (mode) { + case 0: /* normal set */ This needs to use symbolic names from an enum, not magic numbers. Aren't those defined in the accounts service library? ::: panels/user-accounts/um-user-panel.c @@ +353,3 @@ + if (!act_user_manager_delete_user_finish (manager, res, &error)) { +#if 0 + this needs an accountsservice release > 0.6.28 Given that we require accountsservice 0.6.30... ::: panels/user-accounts/um-utils.c @@ +811,3 @@ + } + + /* Size is kosher? */ s/kosher/sane/ @@ +985,3 @@ + GError *error; + + path = g_build_filename (g_get_tmp_dir (), "usericonXXXXXX", NULL); can you please use gnome-control-center in the pattern as well?
(In reply to comment #41) > Review of attachment 232646 [details] [review]: > > Rest looks fine, and seems to work in my limited testing (adding/removing > users, and the likes). > > ::: panels/user-accounts/um-password-dialog.c > @@ +199,3 @@ > > + switch (mode) { > + case 0: /* normal set */ > > This needs to use symbolic names from an enum, not magic numbers. > Aren't those defined in the accounts service library? This is an orthogonal issue, but you're right, that would be a lot cleaner. This mode thing is a um-password-dialog specific construct, that merges various states of the act-user into one enum. > ::: panels/user-accounts/um-user-panel.c > @@ +353,3 @@ > + if (!act_user_manager_delete_user_finish (manager, res, &error)) { > +#if 0 > + this needs an accountsservice release > 0.6.28 > > Given that we require accountsservice 0.6.30... yup. That code was what prompted me to edit configure.ac. I guess I neglected to clean this up after making that change > ::: panels/user-accounts/um-utils.c > @@ +811,3 @@ > + } > + > + /* Size is kosher? */ > > s/kosher/sane/ sure. > @@ +985,3 @@ > + GError *error; > + > + path = g_build_filename (g_get_tmp_dir (), "usericonXXXXXX", NULL); > > can you please use gnome-control-center in the pattern as well? sounds good. Will push with above changes.
The following fixes have been pushed: 9deb4d6 user-accounts: port to libaccountsservice db97299 user-panel: add UmPasswordDialogMode enum instead of using numeric literals
Created attachment 232650 [details] [review] user-accounts: port to libaccountsservice This removes a bunch of duplicated code, and also drops a direct dependency on libsystemd-login.
Created attachment 232651 [details] [review] user-panel: add UmPasswordDialogMode enum instead of using numeric literals The um-password-dialog combobox has a column in its model associated with what password action to apply. Possible actions are: - Normal password - Set password at next login - No password needed - Lock account - Unlock account These actions are currently represented in the code with harded coded numeric values (0-4). This commit cleans up the hard coding to use a symbolic enumeration instead.