Bug 76369

I've reviewed header files, but not signals or properties yet (I glanced at them where functions are implemented in terms of them, but didn't do a systematic scan).

Bad idea
========

TpBaseChannelFillPropertiesFunc takes a GHashTable<string,GValue>.

tp_contact_attribute_map_take_sliced_gvalue takes a GValue.

tp_base_connection_dup_contact_attributes_hash returns a GHashTable<string,GValue>.

tp_base_media_call_content_get_local_media_description returns some dbus-glib thing.

TpBaseMediaCallStreamAddCandidatesFunc takes some dbus-glib thing. tp_base_media_call_stream_set_stun_servers, tp_base_media_call_stream_set_relay_info too. tp_base_media_call_stream_get_local_candidates returns one.

tp_base_protocol_get_immutable_properties returns a{sv}. TpBaseProtocol:immutable-properties is the same.

tp_base_protocol_new_connection takes an a{sv}.

TpBaseRoomConfigUpdateAsync takes an a{sv}?

tp_call_content_media_description_append_codec takes some dbus-glib thing.

tp_call_stream_endpoint_add_new_candidates, 
tp_call_stream_endpoint_add_new_candidate likewise.

tp_capabilities_get_channel_classes returns a{sv}s. TpCapabilities:channel-classes is the same.

TpChannelManagerTypeChannelClassFunc, TpChannelManagerRequestFunc, tp_channel_manager_create_channel, tp_channel_manager_ensure_channel, tp_channel_manager_asv_has_unknown_properties.

tp_connection_manager_param_get_default has a parallel GVariant version already, so it can probably be deleted.

TpDBusPropertiesMixin is entirely dbus-glib-based.

TpGroupMixin too.

Unrelated, but handle.h should talk about entity types, not handle types.

tp_message_set takes a GValue.

TpPresenceStatus has a dbus-glib data type, but should just have a boolean - "allows message?" - since everything else that could go there is no longer in a non-deprecated interface in 0.x, let alone 1.0.

Slightly odd
============

tp_account_get_avatar_finish() returns a GArray<guchar> which is OK but a bit odd. I might replace it with GBytes?

Acceptable
==========

tp_base_call_channel_get_call_members() returns a GHashTable<guint,guint> which is probably OK.

Properties and signals:

Not OK
======

TpAccount:connection-error-details is an a{sv}. TpAccount::status-changed contains the same.

TpBaseCallChannel:contents, TpBaseCallChannel:call-state-reason, TpBaseCallChannel:call-state-details, TpBaseCallChannel:call-members, TpBaseCallChannel:member-identifiers

TpBaseCallContent:streams

TpBaseCallStream:remote-members (but it would be fine with a different GType)

TpBaseCallStream:remote-member-identifiers (but it would be fine with a different GType)

TpBaseConnectionManager:protocols (because it contains their properties)

TpBaseConnection:requestable-channel-classes

TpBaseConnection:dbus-status

TpBaseMediaCallContent:remote-media-descriptions, TpBaseMediaCallContent:local-media-descriptions, TpBaseMediaCallContent:media-description-offer, TpBaseMediaCallContent::local-media-description-updated

TpBaseMediaCallStream:local-credentials, TpBaseMediaCallStream:local-candidates, TpBaseMediaCallStream:stun-servers, TpBaseMediaCallStream:relay-info, TpBaseMediaCallStream:endpoints (but it would be OK with a different GType)

TpBasePasswordChannel:sasl-error-details (but it's internal anyway)

TpBaseProtocol:immutable-properties

TpCallContentMediaDescription:codecs, TpCallContentMediaDescription:ssrcs, TpCallContentMediaDescription:header-extensions, TpCallContentMediaDescription:feedback-messages

TpCallStreamEndpoint:remote-credentials, TpCallStreamEndpoint:remote-candidates, TpCallStreamEndpoint:selected-candidate-pairs, TpCallStreamEndpoint:endpoint-state, TpCallStreamEndpoint::candidate-selected, TpCallStreamEndpoint::candidate-accepted, TpCallStreamEndpoint::candidate-rejected

TpCapabilities:channel-classes (but it also has channel-classes-variant)

TpChannelDispatchOperation:cdo-properties

TpChannel:channel-properties

TpChannel::group-members-changed (the details)

TpExportableChannel:channel-properties

TpExportableChannel's weird interaction with TpSvcChannel:closed

TpObserveChannelContext:observer-info

TpTextChannel:message-types (would be OK with a different GType)



Weird
=====

TpAccountRequest:avatar is a GArray of undocumented type (by inspection: GArray<guchar>, which is a bit weird and a GBytes would probably be better).

----------------

Unrelated things I spotted:

TpHandleChannelContext:dbus-context (etc.) should probably be OBJECT rather than POINTER now.

TpProtocol:avatar-requirements should be BOXED rather than POINTER?

A more difficult one to fix: on the service side, CMs can rely on TpBase* emitting dbus-glib-mapped signals like "closed" and "status-changed".

I think the way to decouple these might be to make TpBase* stop implementing any TpSvc* interfaces, and make them implement GDBus interfaces directly.

In Mission Control, mcp_account_storage_get_identifier(), mcp_dispatch_operation_ref_nth_channel_properties(), mcp_request_ref_nth_request() should maybe get the same treatment.

(In reply to comment #3)
> A more difficult one to fix: on the service side, CMs can rely on TpBase*
> emitting dbus-glib-mapped signals like "closed" and "status-changed".

Signals to look for with "git grep":

Account (MC): "removed"

AccountManager (MC): "account-removed", "account-usability-changed"

ChannelDispatcher/CDO/CR (MC):
    "new-dispatch-operation",
    "dispatch-operation-finished",
    "finished",
    "failed",
    "succeeded",

Call1 (Gabble, Rakia):
    "streams-added"
    "streams-removed"
    "tones-deferred"
    "sending-tones"
    "stopped-tones"
    "new-media-description-offer"
    "media-description-offer-done"
    "local-media-description-changed"
    "remote-media-descriptions-changed"
    "media-descriptions-removed"
    "d-tm-fchange-requested"
    "key-frame-requested",
    "video-resolution-changed",
    "bitrate-changed",
    "framerate-changed",
    "m-tu-changed",
    "remote-members-changed",
    "local-sending-state-changed",
    "sending-state-changed",
    "receiving-state-changed",
    "local-candidates-added",
    "local-credentials-changed",
    "relay-info-changed",
    "s-tu-nservers-changed",
    "server-info-retrieved",
    "endpoints-changed",
    "i-ce-restart-requested",
    "remote-credentials-set",
    "remote-candidates-added",
    "candidate-pair-selected",
    "endpoint-state-changed",
    "controlling-changed",

Channel (all CMs):
    "closed",
    "chat-state-changed",
    "channel-merged",
    "channel-removed",
    "handle-owners-changed",
    "self-contact-changed",
    "group-flags-changed",
    "members-changed",
    "hold-state-changed",
    "password-flags-changed",
    "s-as-lstatus-changed",
    "new-challenge",
    "s-ms-channel-changed",
    "service-point-changed",
    "tube-channel-state-changed",
    "content-added",
    "content-removed",
    "call-state-changed",
    "call-members-changed",
    "search-state-changed",
    "search-result-received",
    "d-bus-names-changed",
    "file-transfer-state-changed",
    "transferred-bytes-changed",
    "initial-offset-defined",
    "u-ri-defined",
    "got-rooms",
    "listing-rooms",
    "new-remote-connection",
    "new-local-connection",
    "connection-closed",
    "message-sent",
    "pending-messages-removed",
    "message-received",

Connection (all CMs):
    "self-contact-changed",
    "connection-error",
    "status-changed",
    "aliases-changed",
    "anonymity-modes-changed",
    "avatar-updated",
    "avatar-retrieved",
    "balance-changed",
    "i-ms-ichanged",
    "client-types-updated",
    "blocked-contacts-changed",
    "contact-capabilities-changed",
    "groups-changed",
    "groups-created",
    "group-renamed",
    "groups-removed",
    "contact-info-changed",
    "contact-list-state-changed",
    "contacts-changed",
    "location-updated",
    "mails-received",
    "unread-mails-changed",
    "power-saving-changed",
    "presences-changed",
    "renamed",
    "new-channel",
    "channel-closed",
    "service-points-changed",

ConnectionManager (all CMs):
    "new-connection",

Debug (everything):
    "new-debug-message",

DBus.Properties (everything):
    "properties-changed",

Logger (tp-glib only):
    "favourite-contacts-changed",

TLSCertificate (potentially all CMs):
    "accepted",
    "rejected",

(In reply to comment #5)
> Account (MC): "removed"
> ChannelDispatcher/CDO/CR (MC):
>     "finished",
>     "failed",
>     "succeeded",

Those are actively used in MC. We could turn them into normal signals and emit them adjacent to tp_svc_foo_emit_bar().

> Debug (everything):
>     "new-debug-message",

There are a couple of false positives because the debug client has a signal of the same name, but no real uses of this or "properties-changed" in MC, 5 CMs, Folks, TPAW, Empathy.

> Logger (tp-glib only):
>     "favourite-contacts-changed",

Not used.

> TLSCertificate (potentially all CMs):
>     "accepted",
>     "rejected",

Used in Gabble and Haze. We could emit normal signals adjacent to these.

(In reply to comment #5)
> Call1 (Gabble, Rakia):

Those don't appear to be used.

(In reply to comment #1)
> I've reviewed header files, but not signals or properties yet

I think the top priorities are the client side:

> tp_capabilities_get_channel_classes returns a{sv}s.
> TpCapabilities:channel-classes is the same.

I started on this but it's pretty annoying, because the regression test is all in terms of dbus-glib.

> tp_connection_manager_param_get_default has a parallel GVariant version
> already, so it can probably be deleted.
> 
> TpDBusPropertiesMixin is entirely dbus-glib-based.
> 
> Unrelated, but handle.h should talk about entity types, not handle types.
> 
> tp_message_set takes a GValue.

> tp_account_get_avatar_finish() returns a GArray<guchar> which is OK but a
> bit odd. I might replace it with GBytes?

(In reply to comment #2)
> Properties and signals:

> TpAccount:connection-error-details is an a{sv}. TpAccount::status-changed
> contains the same.

This was easy, I have a patch already.

> TpCapabilities:channel-classes (but it also has channel-classes-variant)
> 
> TpChannelDispatchOperation:cdo-properties
> 
> TpChannel:channel-properties
> 
> TpChannel::group-members-changed (the details)

> TpObserveChannelContext:observer-info
> 
> TpTextChannel:message-types (would be OK with a different GType)

> TpAccountRequest:avatar is a GArray of undocumented type (by inspection:
> GArray<guchar>, which is a bit weird and a GBytes would probably be better).

> TpHandleChannelContext:dbus-context (etc.) should probably be OBJECT rather
> than POINTER now.
> 
> TpProtocol:avatar-requirements should be BOXED rather than POINTER?

So what's the plan to fix this? Turn TpSvc* from GInterface to GDBusSkeleton and make them has-a instead of is-a? Or just remove signals from the generated interfaces?

Related to this, if we want to use gdbus-codegen skeleton in TpBaseConnection it means we have to stop implementing TpSvcConnection and that's an API break.

I don't know how much work it would be to replace ifaces implemented by out TpBase* by skeletons.

(In reply to comment #9)
> So what's the plan to fix this? Turn TpSvc* from GInterface to GDBusSkeleton
> and make them has-a instead of is-a? Or just remove signals from the
> generated interfaces?

My current plan is to fix the low-hanging fruit (high-level API, especially client-side) first. That's a significant API break already.

Next, I'll probably stop implementing TpSvcDBusProperties (which is a weird special case) in any of our high-level API, and make the TpSvcInterface code call into TpDBusPropertiesMixin instead. MC can still override that with its own TpSvcDBusProperties implementation if it really needs to, but it seems bad for it to be an API guarantee that TpBaseConnection implements D-Bus Properties monolithically, rather than letting GDBus do it per-interface. I might GVariant'ify TpDBusPropertiesMixin at the same time.

Next, I'll go through all our CMs and see how much would break if we got rid of the signals (by grepping for their names), then implement that by either:

* using a hand-written GDBusInterfaceSkeleton for the few interfaces that are implemented by our base classes, or

* generating a GDBusInterfaceSkeleton with GDBus' codegen, or

* changing the codegen to not generate the signal names, or

* changing the codegen so the TpSvc* interfaces can either be has-a or is-a, and changing all our base classes to use them in is-a mode (I suspect it would be too disruptive to CMs if we change all of them to has-a)

> Related to this, if we want to use gdbus-codegen skeleton in
> TpBaseConnection it means we have to stop implementing TpSvcConnection and
> that's an API break.

Hmm, yes, good point.

(In reply to comment #10)
> (In reply to comment #9)
> > Related to this, if we want to use gdbus-codegen skeleton in
> > TpBaseConnection it means we have to stop implementing TpSvcConnection and
> > that's an API break.
> 
> Hmm, yes, good point.

Here is a branch to stop implementing TpSvcConnection in TpBaseConnection. There are actually much more TpSvc* part of our high-level API than I though sadly, that would be a lot of work to port them to gdbus-codegen. I don't think we'll avoid a post-1.0 API break here :( OTOH, I think this branch does the hardest part.

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-codegen-svc

Your gdbus-properties branch mostly looks good to me, but is going to need CM changes. Do you intend to do them, or are you relying on me to go round and do all the API catching-up again?

Mission Control has its own implementation of TpSvcDBusProperties for some reason or other (Bug #32416) and, while I acknowledge that that's a design flaw, I really don't want to put it on the critical path for Telepathy 1. However, now that you've deleted tp_dbus_properties_mixin_iface_init(), I think we can safely remove the TpSvcDBusProperties special case and just have MC keep implementing it. That's how I imagined it would initially work.

> +tp_dbus_object_add_skeleton

I'm really not sure I like that API. I would like to be able to have this:

* parent class P implements interface I with one skeleton
* subclass S reimplements interface I with a different skeleton

Ideally, the parent class's skeleton for that interface would not even get constructed if that happens. Concretely, this is necessary if we want to be able to move new functionality from subclasses up to a parent class when it stabilizes.

Maybe something like this:

interface Exportable {
    vfunc fill_skeletons (Exportable self,
        GHashTable<GQuark,GDBusInterfaceSkeleton> skeletons);
}

with a typical implementation looking like this pseudocode:

fill_skeletons (self, skeletons)
{
  if (skeletons.lookup (TP_IFACE_QUARK_BADGERS) == NULL)
    skeletons.insert (TP_IFACE_QUARK_BADGERS, badger_skeleton_new (self));

  if (skeletons.lookup (TP_IFACE_QUARK_MUSHROOMS) == NULL)
    skeletons.insert (TP_IFACE_QUARK_MUSHROOMS, mushroom_skeleton_new (self));

  parent_class.fill_skeletons (self);

  /* we want to override one method, or something */
  snake_skeleton = skeletons.lookup (TP_IFACE_QUARK_SNAKES);
}

The fallback implementation of fill_skeletons in the GInterface would be what tp_dbus_connection_try_register() does now:

fill_skeletons (self, skeletons)
{
  foreach TpSvc interface, svc
    {
      if (skeletons.lookup (svc.interface_name) == NULL)
        skeletons.insert (svc.interface_name, _tp_svc_interface_new (self));
    }
}

(Implementation detail: tp_dbus_connection_try_register() would make the GHashTable, iterate it, and free it before returning.)

(In reply to comment #10)
> My current plan is to fix the low-hanging fruit (high-level API, especially
> client-side) first. That's a significant API break already.

Ready for review, Bug #77139.

> Next, I'll probably stop implementing TpSvcDBusProperties (which is a weird
> special case) in any of our high-level API, and make the TpSvcInterface code
> call into TpDBusPropertiesMixin instead.

Xavier did a first cut of this, Bug #77144.

> I might GVariant'ify TpDBusPropertiesMixin at the same time.

Not yet implemented, Bug #77145.

(In reply to comment #4)
> In Mission Control, mcp_account_storage_get_identifier(),
> mcp_dispatch_operation_ref_nth_channel_properties(),
> mcp_request_ref_nth_request() should maybe get the same treatment.

Done as part of Bug #77139.

> TpHandleChannelContext:dbus-context (etc.) should probably be OBJECT rather
> than POINTER now.
>
> TpProtocol:avatar-requirements should be BOXED rather than POINTER?

Bug #77143 but not really critical tbh.

What's left after all those is a lot of service-side stuff.

*** Bug 75204 has been marked as a duplicate of this bug. ***

(In reply to comment #2)
> Properties and signals:
...
> TpAccount:connection-error-details is an a{sv}. TpAccount::status-changed
> contains the same.
> 
> TpBaseCallChannel:contents, TpBaseCallChannel:call-state-reason,
> TpBaseCallChannel:call-state-details, TpBaseCallChannel:call-members,
> TpBaseCallChannel:member-identifiers
> TpBaseCallContent:streams
> TpBaseCallStream:remote-members (but it would be fine with a different GType)
> TpBaseCallStream:remote-member-identifiers (but it would be fine with a
> different GType)
> TpBaseMediaCallContent:remote-media-descriptions,
> TpBaseMediaCallContent:local-media-descriptions,
> TpBaseMediaCallContent:media-description-offer,
> TpBaseMediaCallStream:local-credentials,
> TpBaseMediaCallStream:local-candidates, TpBaseMediaCallStream:stun-servers,
> TpBaseMediaCallStream:relay-info, TpBaseMediaCallStream:endpoints (but it
> would be OK with a different GType)
> TpBasePasswordChannel:sasl-error-details (but it's internal anyway)
> TpBaseConnectionManager:protocols (because it contains their properties)
> TpBaseConnection:requestable-channel-classes
> TpBaseConnection:dbus-status
> TpCallContentMediaDescription:codecs, TpCallContentMediaDescription:ssrcs,
> TpCallContentMediaDescription:header-extensions,
> TpCallContentMediaDescription:feedback-messages
> TpCallStreamEndpoint:remote-credentials,
> TpCallStreamEndpoint:remote-candidates,
> TpCallStreamEndpoint:selected-candidate-pairs,
> TpCallStreamEndpoint:endpoint-state,
> TpCallStreamEndpoint::candidate-selected,
> TpCallStreamEndpoint::candidate-accepted,
> TpCallStreamEndpoint::candidate-rejected

None of those are used in the "big 5" CMs.

> TpBaseMediaCallContent::local-media-description-updated

Rakia and Gabble do use that.

> TpBaseProtocol:immutable-properties

Gabble uses that, and overriding it is documented to be how you extend the Protocol. I think we should make it into a GVariant.

> TpExportableChannel:channel-properties

Doesn't appear to be used directly by CMs any more - they all rely on TpBaseChannel for it.

> TpExportableChannel's weird interaction with TpSvcChannel::closed

Realistically, I think we have to keep this, even if it becomes TpExportableChannel::closed.

(In reply to comment #5)
> Connection (all CMs):
>     "status-changed",

Used just about everywhere; needs special-casing

>     "self-contact-changed",
>     "connection-error",
>     "aliases-changed",
>     "anonymity-modes-changed",
>     "avatar-updated",
>     "avatar-retrieved",
>     "balance-changed",
>     "i-ms-ichanged",
>     "blocked-contacts-changed",
>     "contact-capabilities-changed",
>     "groups-changed",
>     "groups-created",
>     "group-renamed",
>     "groups-removed",
>     "contact-info-changed",
>     "contact-list-state-changed",
>     "contacts-changed",
>     "location-updated",
>     "mails-received",
>     "unread-mails-changed",
>     "power-saving-changed",
>     "presences-changed",
>     "renamed",
>     "new-channel",
>     "channel-closed",
>     "service-points-changed",
>     "client-types-updated",

Not used

(In reply to comment #5)
> Channel (all CMs):
>     "closed",

Used extensively, special case needed

>     "content-added",
>     "content-removed",
>     "call-state-changed",

Used in Gabble, special case probably needed

>     "chat-state-changed",
>     "channel-merged",
>     "channel-removed",
>     "handle-owners-changed",
>     "self-contact-changed",
>     "group-flags-changed",
>     "members-changed",
>     "hold-state-changed",
>     "password-flags-changed",
>     "s-as-lstatus-changed",
>     "new-challenge",
>     "s-ms-channel-changed",
>     "service-point-changed",
>     "tube-channel-state-changed",

Not used outside telepathy-glib itself

>     "call-members-changed",
>     "search-state-changed",
>     "search-result-received",
>     "d-bus-names-changed",
>     "file-transfer-state-changed",
>     "transferred-bytes-changed",
>     "initial-offset-defined",
>     "u-ri-defined",
>     "got-rooms",
>     "listing-rooms",
>     "new-remote-connection",
>     "new-local-connection",
>     "connection-closed",
>     "message-sent",
>     "pending-messages-removed",
>     "message-received",

> ConnectionManager (all CMs):
>     "new-connection",

Not used

... I think that's everything. So by making a relatively small number of special cases, we can get rid of signals from TpSvc interfaces.

I think this is now effectively a metabug.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/122.