Bug 77772

You mean that it should be:

typedef GStrv (*TpBaseContactListDupContactGroupsFunc) (
    TpContactGroupListInterface *self,
    TpHandle contact);

instead of:

typedef GStrv (*TpBaseContactListDupContactGroupsFunc) (
    TpBaseContactList *self,
    TpHandle contact);

?

If yes, then I guess we should do this change for all the vfunc in this interface, don't we?

(In reply to comment #1)
> You mean that it should be:
> 
> typedef GStrv (*TpBaseContactListDupContactGroupsFunc) (
>     TpContactGroupListInterface *self,
>     TpHandle contact);
> 
> instead of:
> 
> typedef GStrv (*TpBaseContactListDupContactGroupsFunc) (
>     TpBaseContactList *self,
>     TpHandle contact);
> 
> ?
> 
> If yes, then I guess we should do this change for all the vfunc in this
> interface, don't we?

Yes. The pattern is, wherever we have

typedef Mushroom (*BadgerFunc) (Snake *self, ...);
                            //  ^^^^^
struct _BadgerInterface {
  ...
  BadgerFunc dance;
}

the vfunc's "self" parameter needs to be changed to match the dummy typedef for the interface:

typedef Mushroom (*BadgerFunc) (Badger *self, ...);
                             // ^^^^^^
struct _BadgerInterface {
  ...
  BadgerFunc dance;
}

This might mean splitting up some of the typedefs that are currently shared between interfaces, according to the type that their "self" parameter needs.

Simon: can you please take a quick look on http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-contact-list-iface-77772 and check if this is going into the right direction?

As you can see in the top commit, doing this means duplicating a bit of generic functions and typedef but I don't see any another option.

Also, should method like "GStrv tp_base_contact_list_dup_groups (TpBaseContactList *self);" be changed to take a TpContactGroupList as first argument? This methods assert that the iface is implemented so I guess that would be ok. But for example tp_base_contact_list_normalize_group() just return FALSE in that case.

(In reply to comment #3)
> Simon: can you please take a quick look on
> http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-
> contact-list-iface-77772 and check if this is going into the right direction?

Yes, this is the right direction.

> As you can see in the top commit, doing this means duplicating a bit of
> generic functions and typedef but I don't see any another option.

Agreed.

> Also, should method like "GStrv tp_base_contact_list_dup_groups
> (TpBaseContactList *self);" be changed to take a TpContactGroupList as first
> argument? This methods assert that the iface is implemented so I guess that
> would be ok. But for example tp_base_contact_list_normalize_group() just
> return FALSE in that case.

Oh, I'd forgotten there were functions like that. I must have thought I was being so clever while designing those interfaces :-(

For the methods where it's considered to be a programming error to call them on the wrong type, it is correct to change the first argument.

For the methods where calling them on the wrong type is allowed ... I think we should probably change the semantics to "it's a programming error to call it on the wrong type" (g_return_if_fail), and move the graceful type-check into the callers. A bit of "git grep" will be necessary.

This part can already be reviewed and merged:
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-contact-list-iface-77772

If it's fine, I'll do the other ifaces next.

Port of Gabble and Haze:
http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-contact-list-iface-77772
http://cgit.collabora.com/git/user/cassidy/telepathy-haze/log/?h=next-contact-list-iface-77772

(In reply to comment #4)
> For the methods where calling them on the wrong type is allowed ... I think
> we should probably change the semantics to "it's a programming error to call
> it on the wrong type" (g_return_if_fail), and move the graceful type-check
> into the callers. A bit of "git grep" will be necessary.

Does this apply to tp_base_contact_list_can_block() as well which is explicitely used to check if the contact list support blocking?

+ * It is an error to call this fonction if @self subclass does not implement

"function" (please git grep)

I think "if @self does not implement" would also be better.

All looks OK so far, apart from those comments.

(In reply to comment #7)
> Does this apply to tp_base_contact_list_can_block() as well which is
> explicitely used to check if the contact list support blocking?

Hmm. For now let's say "no, leave can_block() as-is" and see what the g-i looks like.

If necessary we can change all its callers to

    (TP_IS_BLOCKABLE_CONTACT_LIST (cl) &&
     tp_blockable_contact_list_can_block ((TpBlockableContactList *) cl))

but that's pretty cumbersome...

(In reply to comment #8)
> + * It is an error to call this fonction if @self subclass does not implement
> 
> "function" (please git grep)
> 
> I think "if @self does not implement" would also be better.
> 
> All looks OK so far, apart from those comments.

Fixed and merged.

More of this:
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-contact-list-iface-77772
http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-contact-list-iface-77772

> (In reply to comment #7)
> > Does this apply to tp_base_contact_list_can_block() as well which is
> > explicitely used to check if the contact list support blocking?
> 
> Hmm. For now let's say "no, leave can_block() as-is" and see what the g-i
> looks like.


      <method name="can_block"
              c:identifier="tp_base_contact_list_can_block"
              version="0.13.0">
        <doc xml:space="preserve">Return whether this contact list has a list of blocked contacts. If it
does, that list is assumed to be modifiable.

If the #TpBaseContactList subclass does not implement
%TP_TYPE_BLOCKABLE_CONTACT_LIST, this method always returns %FALSE.

For implementations of %TP_TYPE_BLOCKABLE_CONTACT_LIST, this is a virtual
method, implemented using #TpBlockableContactListInterface.can_block.
The default implementation always returns %TRUE.

In the case of a protocol where blocking may or may not work
and this is detected while connecting, the subclass can implement another
#TpBaseContactListBooleanFunc (whose result must remain constant
after the #TpBaseConnection has moved to state
%TP_CONNECTION_STATUS_CONNECTED), and use that as the implementation.

(For instance, this could be useful for XMPP, where support for contact
blocking is server-dependent: telepathy-gabble 0.8.x implements it for
connections to Google Talk servers, but not for any other server.)</doc>
        <return-value transfer-ownership="none">
          <doc xml:space="preserve">%TRUE if communication from contacts can be blocked</doc>
          <type name="gboolean" c:type="gboolean"/>
        </return-value>
        <parameters>
          <instance-parameter name="self" transfer-ownership="none">
            <doc xml:space="preserve">a contact list manager</doc>
            <type name="BaseContactList" c:type="TpBaseContactList*"/>
          </instance-parameter>
        </parameters>
      </method>

(In reply to comment #11)
>       <method name="can_block"
>               c:identifier="tp_base_contact_list_can_block"

I think that g-i looks OK.

(In reply to comment #10)
> More of this:
> http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-
> contact-list-iface-77772

I don't see any new commits there?

> http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-
> contact-list-iface-77772

The Gabble changes look OK, assuming reasonable tp-glib changes.

One comment: casting "TpMutableContactList *base" to TpBaseContactList * seems confusing. Shouldn't it be called iface or mutable or something instead? (I would be happy for your answer to be "yes but it's too much diffstat to be worth it", though.)

I assume Haze will also need updating?

(In reply to comment #12)
> (In reply to comment #11)
> >       <method name="can_block"
> >               c:identifier="tp_base_contact_list_can_block"
> 
> I think that g-i looks OK.
> 
> (In reply to comment #10)
> > More of this:
> > http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-
> > contact-list-iface-77772
> 
> I don't see any new commits there?

Is cgit being shit again? Here it show 9 commits (from db6d23db0fda5fe91f30e1bb99b6120b423a98a3 to 444f4ab3ff429e36c1a31c199b323751b74ff1c8) on top of next.

> > http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-
> > contact-list-iface-77772
> 
> The Gabble changes look OK, assuming reasonable tp-glib changes.
> 
> One comment: casting "TpMutableContactList *base" to TpBaseContactList *
> seems confusing. Shouldn't it be called iface or mutable or something
> instead? (I would be happy for your answer to be "yes but it's too much
> diffstat to be worth it", though.)

Yeah I know, but I have been too lazy to change those. :p

> I assume Haze will also need updating?

Actually no, it doesn't use any of those API.

Ping? I think there is not much work left before merging this.

Fixed in git for 0.99.12, thanks