GNOME Bugzilla – Bug 680892
user-panel: Update to new DBus interface for realmd 0.6
Last modified: 2012-08-08 12:26:00 UTC
Update to new realmd interfaces. I'll be doing this release soon: https://bugs.freedesktop.org/show_bug.cgi?id=52993 But this code works with git master of realmd as well since I've bumped the version number.
Created attachment 219957 [details] [review] user-panel: Update to new DBus interface for realmd 0.6 * In particular support of different credential methods and better hints for different owners of those credentials, so we can prompt more cleanly. * Less abstraction in the realmd interfaces
Review of attachment 219957 [details] [review]: Looks fine. We should definitely work with the latest realmd. A few comments below, but I don't think any of them are critical, so commit whenever. ::: panels/user-accounts/data/org.freedesktop.realmd.xml @@ +107,3 @@ + * Credentials: (ssv) + * type: 'ccache', 'password', 'automatic' + * owner: 'administrator', 'user', 'computer', 'secret' in the realmd source this says who not owner ::: panels/user-accounts/um-account-dialog.c @@ +429,3 @@ + um_realm_kerberos_call_change_login_policy (self->selected_realm, "", + add, remove, + g_variant_ref_sink (options), I don't think you need to ref_sink here @@ +434,3 @@ + g_object_ref (self)); + + g_variant_unref (options); or unref here. The floating reference should just work without any ref/unref. Could be I'm wrong though. You do this refsink thing in most calls ::: panels/user-accounts/um-realm-manager.c @@ +623,1 @@ + creds = g_variant_new ("(ss@v)", type, owner, g_variant_new_variant (contents)); It could be i'm missing something, but can't you just do g_variant_new ("(ssv)", type, owner, contents); ?
(In reply to comment #2) > Review of attachment 219957 [details] [review]: > > Looks fine. We should definitely work with the latest realmd. A few comments > below, but I don't think any of them are critical, so commit whenever. Alright, I pushed the patch as is. But will follow it up with another commit to tweak the stylish issues below. I didn't have time to make the changes below *and* test against a directory. I mean, it's pretty clear you're right on these things, I just feel uneasy making changes without running them especially when it comes to spooky things like floating references. > ::: panels/user-accounts/data/org.freedesktop.realmd.xml > @@ +107,3 @@ > + * Credentials: (ssv) > + * type: 'ccache', 'password', 'automatic' > + * owner: 'administrator', 'user', 'computer', 'secret' > > in the realmd source this says who not owner Will change this in realmd. > ::: panels/user-accounts/um-account-dialog.c > @@ +429,3 @@ > + um_realm_kerberos_call_change_login_policy (self->selected_realm, "", > + add, remove, > + g_variant_ref_sink > (options), > > I don't think you need to ref_sink here > > @@ +434,3 @@ > + g_object_ref (self)); > + > + g_variant_unref (options); > > or unref here. The floating reference should just work without any ref/unref. > Could be I'm wrong though. You do this refsink thing in most calls Indeed. Since the the various um_realm-kerberos_call_ wrappers pass all the arguments to g_variant_new() ... Will change. > ::: panels/user-accounts/um-realm-manager.c > @@ +623,1 @@ > + creds = g_variant_new ("(ss@v)", type, owner, g_variant_new_variant > (contents)); > > It could be i'm missing something, but can't you just do > > g_variant_new ("(ssv)", type, owner, contents); You're right. Will change.
Created attachment 220641 [details] [review] user-accounts: Cleanup GVariant usage * Don't call g_variant_ref_sink() unnecessarily. * Don't call g_variant_new_variant() unnecessarily.
Created attachment 220642 [details] [review] user-accounts: Remove needless console output
Review of attachment 220641 [details] [review]: Looks good.
Review of attachment 220642 [details] [review]: Feel free to add any g_debug() calls you think might be needed to debug in the future.
Attachment 220641 [details] pushed as 4e67ecc - user-accounts: Cleanup GVariant usage Attachment 220642 [details] pushed as ec2fc6d - user-accounts: Remove needless console output