Once this is fixed and implemented, TpAccount should be modified to be updated.

Created attachment 72625 [details] [review]
patch

Hum or should I use the Account.AccountPropertyChanged signal?

Anyway, this patch is wrong as I forgot the org.freedesktop.DBus.Property.EmitsChangedSignal annotation.

Created attachment 72627 [details] [review]
new spec patch

Created attachment 72628 [details] [review]
MC patch

And here is the tp-glib branch: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=uri-40393

Comment on attachment 72627 [details] [review]
new spec patch

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

::: spec/Account_Interface_Addressing.xml
@@ +19,3 @@
>    <interface name="org.freedesktop.Telepathy.Account.Interface.Addressing">
>      <tp:requires interface="org.freedesktop.Telepathy.Account"/>
>      <tp:added version="0.21.5">(as stable API)</tp:added>

<tp:changed> please.

Comment on attachment 72628 [details] [review]
MC patch

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

::: tests/twisted/account/addressing.py
@@ +53,5 @@
>      assertEquals (uri_schemes, ['email'])
>  
> +    e = q.expect('dbus-signal', signal='PropertiesChanged')
> +    assertEquals (e.args, [ cs.ACCOUNT_IFACE_ADDRESSING,
> +        { cs.ACCOUNT_IFACE_ADDRESSING + '.URISchemes' : ['email'] }, [] ])

This would be more illustrative if it added a second URI scheme, to demonstrate that when we add 'telnet' having already added 'email', the signal says ['email', 'telnet'].

Total nitpicking: the URI scheme is 'mailto' :-P

> + g_object_get (test->account, "uri-schemes", &tmp, NULL);

Whitespace nitpicking: I'd prefer the "linked pairs" pattern:

    g_object_get (test->account,
        "uri-schemes", &tmp,
        NULL);

It would be nice if the doc-comment for the property said "notify::uri-schemes cannot be relied on if the Account Manager is Mission Control version 5.12 or older", or something.

This all looks basically good, just some minor nitpicking in each affected project.

(In reply to comment #7)
> Comment on attachment 72627 [details] [review] [review]
> new spec patch
> 
> Review of attachment 72627 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: spec/Account_Interface_Addressing.xml
> @@ +19,3 @@
> >    <interface name="org.freedesktop.Telepathy.Account.Interface.Addressing">
> >      <tp:requires interface="org.freedesktop.Telepathy.Account"/>
> >      <tp:added version="0.21.5">(as stable API)</tp:added>
> 
> <tp:changed> please.

Fixed in http://cgit.collabora.com/git/user/cassidy/telepathy-spec/log/?h=uri-40393

(In reply to comment #8)
> Comment on attachment 72628 [details] [review] [review]
> MC patch
> 
> Review of attachment 72628 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/twisted/account/addressing.py
> @@ +53,5 @@
> >      assertEquals (uri_schemes, ['email'])
> >  
> > +    e = q.expect('dbus-signal', signal='PropertiesChanged')
> > +    assertEquals (e.args, [ cs.ACCOUNT_IFACE_ADDRESSING,
> > +        { cs.ACCOUNT_IFACE_ADDRESSING + '.URISchemes' : ['email'] }, [] ])
> 
> This would be more illustrative if it added a second URI scheme, to
> demonstrate that when we add 'telnet' having already added 'email', the
> signal says ['email', 'telnet'].
> 
> Total nitpicking: the URI scheme is 'mailto' :-P

Fixed in http://cgit.collabora.com/git/user/cassidy/telepathy-spec/log/?h=uri-40393

(In reply to comment #9)
> > + g_object_get (test->account, "uri-schemes", &tmp, NULL);
> 
> Whitespace nitpicking: I'd prefer the "linked pairs" pattern:
> 
>     g_object_get (test->account,
>         "uri-schemes", &tmp,
>         NULL);

fixed.

> It would be nice if the doc-comment for the property said
> "notify::uri-schemes cannot be relied on if the Account Manager is Mission
> Control version 5.12 or older", or something.

done.

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=uri-40393

Spec, tp-glib changes look fine.

MC changes are really at http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=uri-40393 and are fine except for this:

+ e = q.expect('dbus-signal', signal='PropertiesChanged')
+ assertEquals (e.args, [ cs.ACCOUNT_IFACE_ADDRESSING,
+ { cs.ACCOUNT_IFACE_ADDRESSING + '.URISchemes' : ['telnet', 'email'] }, [] ])

Unfortunately, this demonstrates a bug, which I didn't catch first time round :-( PropertiesChanged.args[1..] is meant to contain bare property names, so in this case it should be just 'URISchemes'. (It's not amazingly clear in the D-Bus Specification, but it's consistent with Get and GetAll, and GDBus matches this interpretation.)

Also, that quoted block will fail if the URISchemes happen to come out in the other order. I would suggest something like this:

q.expect('dbus-signal', signal='PropertiesChanged',
    predicate=(lambda e:
        e.args[0] == cs.ACCOUNT_IFACE_ADDRESSING and
        set(e.args[1]['URISchemes']) == set('telnet', 'email'))

(or the mailto equivalent, depending which order you do this in).

(In reply to comment #13)
> MC changes are really at
> http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/
> ?h=uri-40393 and are fine except for this:
> 
> + e = q.expect('dbus-signal', signal='PropertiesChanged')
> + assertEquals (e.args, [ cs.ACCOUNT_IFACE_ADDRESSING,
> + { cs.ACCOUNT_IFACE_ADDRESSING + '.URISchemes' : ['telnet', 'email'] }, []
> ])
> 
> Unfortunately, this demonstrates a bug, which I didn't catch first time
> round :-( PropertiesChanged.args[1..] is meant to contain bare property
> names, so in this case it should be just 'URISchemes'. (It's not amazingly
> clear in the D-Bus Specification, but it's consistent with Get and GetAll,
> and GDBus matches this interpretation.)

Ah ok, I read the spec and assumed it was the full-name. Fixed.

I updated tp-glib as well: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=uri-40393

> Also, that quoted block will fail if the URISchemes happen to come out in
> the other order. I would suggest something like this:
> 
> q.expect('dbus-signal', signal='PropertiesChanged',
>     predicate=(lambda e:
>         e.args[0] == cs.ACCOUNT_IFACE_ADDRESSING and
>         set(e.args[1]['URISchemes']) == set('telnet', 'email'))
> 
> (or the mailto equivalent, depending which order you do this in).

done.

http://cgit.collabora.com/git/user/cassidy/telepathy-mission-control/log/?h=uri-40393
updated

Ship it.

Merged for:
- telepathy-spec 0.27.1
- telepathy-mission-control 5.15.0
- telepathy-glib 0.21.0