Drive-by review by Will on IRC:

> Release() should remove before re-choosing a presence

Hm, why? I thought it was a way for clients to change their mind about the
presence they want, not as a shortcut to Release()/Request().

Example:
  1. client sets MinimumPresence to busy
  2. user sets RequestedPresence to offline (presence stays busy)
  3. client changes MinimumPresence to hidden
  4. presence changes to hidden

If it were to remove presence before re-choosing it:
  1. client sets MinimumPresence to busy
  2. user sets RequestedPresence to offline (presence stays busy)
  3. client changes MinimumPresence to hidden, but this first releases it
  4. connection goes down, since presence is offline
  5. now new MinimumPresence request is in effect, connection is brought back up

I can't see any reason why this would be useful. Also, not releasing before updating requested presence seems to me analogous of how RequestedPresence works - you don't go via offline/unset when changing from one (oneline) presence to another.

> it doesn't notice if clients call Request(), then die before calling Relese()

Added (in several patches). Also noticed that my original code set new
minimum presence before removing the presence being removed from the list, oops!

> copyrights are wrong

pushed the patch to correct this

(In reply to comment #1)
> Drive-by review by Will on IRC:
> 
> > Release() should remove before re-choosing a presence
> 
> [...]
> 
> > it doesn't notice if clients call Request(), then die before calling Relese()
> 
> Added (in several patches). Also noticed that my original code set new
> minimum presence before removing the presence being removed from the list,
> oops!

...and the latter is what I meant by my first comment above. Sorry for the lack of clarity. :)

  *
  * Contact: Alberto Mardegan  <alberto.mardegan@nokia.com>

I guess he'd rather not be contacted about a file he's never touched? We could just bin that line.

+    priv = g_new0 (McdAccountPresencePrivate, 1);

g_slice_new0 plz.

Otherwise looks good!

(In reply to comment #3)
>   *
>   * Contact: Alberto Mardegan  <alberto.mardegan@nokia.com>
> 
> I guess he'd rather not be contacted about a file he's never touched? We could
> just bin that line.
> 
> +    priv = g_new0 (McdAccountPresencePrivate, 1);
> 
> g_slice_new0 plz.
> 
> Otherwise looks good!

Fixed those two and merged, thanks!

Reopening for updated spec draft.

Updated implementation to DRAFT2 of the spec.

Looks good so far, but I think you're missing an implementation of MinimumPresenceRequestsChanged.

(In reply to comment #7)
> Looks good so far, but I think you're missing an implementation of
> MinimumPresenceRequestsChanged.

Oops, indeed I do! Updated my branch to implement & test it.

You've lost the (implicit) assertion that SetMinimumPresence doesn't fail. I'd prefer each call to call_async to be followed by an expectation that SetMinimumPresence returns successfully (dbus-return) and the signal is emitted (dbus-signal).

Depending on the semantics you want, you could either have an expect_many, or two expects in a specified order. I think we normally say setters should emit change notification before returning.

(In reply to comment #9)
> You've lost the (implicit) assertion that SetMinimumPresence doesn't fail. I'd
> prefer each call to call_async to be followed by an expectation that
> SetMinimumPresence returns successfully (dbus-return) and the signal is emitted
> (dbus-signal).

It seems this was fixed in the branch at some point. review+

From the department of removing unused unstable API... MinimumPresence turns out to be unused after all. So here's a branch that removes it: http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=shortlog;h=refs/heads/remove-MinimumPresence

I like deletion of unused code.

Merged; will be in 5.7.2