Created attachment 80934 [details] [review]
Factor out file monitor setup.

Created attachment 80935 [details] [review]
Also monitor /etc/groups for changes.

Created attachment 80948 [details] [review]
Also monitor /etc/groups for changes.

Comment on attachment 80934 [details] [review]
Factor out file monitor setup.

Review of attachment 80934 [details] [review]:
-----------------------------------------------------------------

::: src/daemon.c
@@ +749,5 @@
> +               void (*callback) (GFileMonitor      *monitor,
> +                                 GFile             *file,
> +                                 GFile             *other_file,
> +                                 GFileMonitorEvent  event_type,
> +                                 Daemon            *daemon))

Seems like a typedef is appropriate here, no? Or just use G_CALLBACK(), and move that cast up the call stack.

Comment on attachment 80948 [details] [review]
Also monitor /etc/groups for changes.

Review of attachment 80948 [details] [review]:
-----------------------------------------------------------------

::: src/user.c
@@ +216,4 @@
>          /* GID */
>          user->gid = pwent->pw_gid;
>  
> +        AccountType at = account_type_from_pwent (pwent);

Declarations should be at top of block.

@@ +220,5 @@
> +        if (at != user->account_type) {
> +                user->account_type = at;
> +                changed = TRUE;
> +                g_object_notify (G_OBJECT (user), "account-type");
> +        }

Isn't all of this logically a different change than what the commit message describes? Maybe apropriate to split this out into a separate patch?

(In reply to comment #4)
> Comment on attachment 80934 [details] [review] [review]
> Factor out file monitor setup.
> 
> Review of attachment 80934 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/daemon.c
> @@ +749,5 @@
> > +               void (*callback) (GFileMonitor      *monitor,
> > +                                 GFile             *file,
> > +                                 GFile             *other_file,
> > +                                 GFileMonitorEvent  event_type,
> > +                                 Daemon            *daemon))
> 
> Seems like a typedef is appropriate here, no? Or just use G_CALLBACK(), and
> move that cast up the call stack.

I'd say that GCallback is not really appropriate since setup_monitor is more specialized and we can get some compile-time type checking by using the correct type for the callback.

I'll add a FileChangedCallback typedef.

> @@ +220,5 @@
> > +        if (at != user->account_type) {
> > +                user->account_type = at;
> > +                changed = TRUE;
> > +                g_object_notify (G_OBJECT (user), "account-type");
> > +        }
> 
> Isn't all of this logically a different change than what the commit message
> describes? Maybe apropriate to split this out into a separate patch?

Sure.

Created attachment 80983 [details] [review]
Factor out file monitor setup.

Added typedef.

Created attachment 80984 [details] [review]
Also monitor /etc/groups for changes.

Don't emit "changed".

Created attachment 80985 [details] [review]
Emit "changed" signal when account type changes.

Comment on attachment 80983 [details] [review]
Factor out file monitor setup.

Review of attachment 80983 [details] [review]:
-----------------------------------------------------------------

Other than the whitespace tweak, this looks good.

::: src/daemon.c
@@ +751,5 @@
> +
> +static GFileMonitor *
> +setup_monitor (Daemon *daemon,
> +               const gchar *path,
> +               FileChangeCallback *callback)

Spacing of the arguments.

Comment on attachment 80984 [details] [review]
Also monitor /etc/groups for changes.

Review of attachment 80984 [details] [review]:
-----------------------------------------------------------------

Do all the GFileMonitor leak? Seems like group_monitor does.

Comment on attachment 80985 [details] [review]
Emit "changed" signal when account type changes.

Review of attachment 80985 [details] [review]:
-----------------------------------------------------------------

Looks good.

(In reply to comment #12)
> Comment on attachment 80984 [details] [review] [review]
> Also monitor /etc/groups for changes.
> 
> Review of attachment 80984 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Do all the GFileMonitor leak? Seems like group_monitor does.

Well, daemon_finalize is never called afaics, so...  I think that's a feature and we could put a g_assert_not_reached into daemon_finalize, even.

Created attachment 81007 [details] [review]
Factor out file monitor setup.

> Spacing of the arguments.

Yep.

i've pushed these.

(In reply to comment #17)
> i've pushed these.

Thanks!