Bug 76111

(In reply to comment #0)
> Created internally by tp-glib without using TpClientFactory
> -----------------------------------------------------------
> - TpCallContent
> - TpCallStream
> 
> Should we change those to use the factory internally and so benefit from its
> cache?

I'm not sure what benefit there would be? They're scoped to a TpCallChannel so there should never be duplicates anyway? But, if you want, go ahead.

> - TpClient: actually it doesn't seem to be instantiated at all by tp-glib,
> only in its tests

Mission Control does create a subclass of TpClient directly. It could do that by registering a factory subclass if desired.

> Should these 2 use the factory? I guess we could?
> - TpChannelDispatcher
> - TpLogger

If you like.

> But actually, shouldn't we consider the factory as the ultimate "top level"
> object and force user to create them using a factory as well? It would make
> the API more symetric and would allow us to remove some weird API like
> tp_account_manager_set_default() and tp_account_manager_can_set_default().

Yeah, perhaps. The concept of the default account manager is pretty strange; if we have a default anything, it should be a default factory, associated with tp_dbus_daemon_dup().

I think there does need to be a way to "go behind the factory's back" for at least some of those objects, but it shouldn't be the primary API.

To summary IRC conversation, here is my opinion on this:

1) In next it would be nice to have the factory as the only top-level singleton. That means that we could replace tp_a_m_dup() by tp_client_factory_dup() + tp_client_factory_dup_am(). I think we would need tp_client_factory_set_default() for apps that want to create their factory subclass and set it as default. Maybe we can keep tp_a_m_dup() as helper that dup the default factory then dup the factory's am.

2) In next it would be nice if TpProxy:factory becomes a mandatory construct-only property. tp_proxy_constructed() should assert that's non-null and app can then rely on tp_proxy_get_factory() to never return NULL. That doesn't mean we want public tp_client_factory_dup_foo() for everything. For example TpCallContent is created only internally by TpCallChannel so _tp_call_content_new() can simply pass the channel's factory to g_object_new().

3) dup_features() vfuncs are probably useless. TpClientFactory subclasses could just call _add_foo_features() from its _constructed() method. One rational for those vfunc is that TpTextChannel doesn't need to get TpCallChannel features. But TpProxy will probably nicely filter that nicely anyway.

4) Some create() vfunc could be retracted. Their only purpose is to let create subclasses, and in practice we do that only for TpChannel. Maybe we should just forget about subclassing TpAccount, TpConnection and TpContact since nobody did it anyway and that would simplify a bit of code.


(In reply to comment #0)
> Created internally by tp-glib without using TpClientFactory
> -----------------------------------------------------------
> - TpCallContent
> - TpCallStream

Uniqueness is already guaranteed by TpCallChannel, so it's fine like that. A trivial patch (even in master) would be to pass the channel's factory in the g_object_new() properties to get closer to the goal "every TpProxy has a factory".

> - TpClient: actually it doesn't seem to be instantiated at all by tp-glib,
> only in its tests

Not even sure why we have that in tp-glib API at all, maybe it should be moved to MC since it's the only user. I think MC will always be the special case where relying on the lowlevel dbus API will always be needed anyway.

> Can be created by user using a specific API
> -------------------------------------------
> - TpProtocol: but bug#75881 is going to move it to the factory
> - TpTLSCertificate: I guess it could use the factory instead

I think those need public API to be created from a factory. No need of vfunc for them though.

> Should these 2 use the factory? I guess we could?
> - TpChannelDispatcher

Do we need TpChannelDispatcher constructor to be public? I would consider that object to be created internally in tp-glib only, no?

> - TpLogger

Same case as TpAM, I would have tp_client_factory_dup_logger() and maybe keep tp_logger_dup() as an helper that call tp_client_factory_dup_logger() on the default factory.

> - TpConnectionManager: as I said on bug#75881 the connection manager is not
> considered as a "top level object". Maybe it should be?

The factory could gain a CM-name -> TpConnectionManager table just like you did for TpProtocol.

> - TpAccountManager

See above.

Note: we don't have to implement all that in next if we want to make it happen soon. But if we have time I think it is good to have.

(In reply to comment #2)
> 1) In next it would be nice to have the factory as the only top-level
> singleton. That means that we could replace tp_a_m_dup() by
> tp_client_factory_dup() + tp_client_factory_dup_am(). I think we would need
> tp_client_factory_set_default() for apps that want to create their factory
> subclass and set it as default. Maybe we can keep tp_a_m_dup() as helper
> that dup the default factory then dup the factory's am.

Agreed, I never liked having 2 kind of top level objects anyway.

> 2) In next it would be nice if TpProxy:factory becomes a mandatory
> construct-only property. tp_proxy_constructed() should assert that's
> non-null and app can then rely on tp_proxy_get_factory() to never return
> NULL. That doesn't mean we want public tp_client_factory_dup_foo() for
> everything. For example TpCallContent is created only internally by
> TpCallChannel so _tp_call_content_new() can simply pass the channel's
> factory to g_object_new().

I like the idea but smcv seemed to be keen on still being able to create proxies behind the factory's back.

> 3) dup_features() vfuncs are probably useless. TpClientFactory subclasses
> could just call _add_foo_features() from its _constructed() method. One
> rational for those vfunc is that TpTextChannel doesn't need to get
> TpCallChannel features. But TpProxy will probably nicely filter that nicely
> anyway.

Not sure. Maybe?

> 4) Some create() vfunc could be retracted. Their only purpose is to let
> create subclasses, and in practice we do that only for TpChannel. Maybe we
> should just forget about subclassing TpAccount, TpConnection and TpContact
> since nobody did it anyway and that would simplify a bit of code.

I guess if we let enough padding we could always add them later if needed. I suspect MC is the only who may ever need subclassing something else than channels.

> (In reply to comment #0)
> > Created internally by tp-glib without using TpClientFactory
> > -----------------------------------------------------------
> > - TpCallContent
> > - TpCallStream
> 
> Uniqueness is already guaranteed by TpCallChannel, so it's fine like that. A
> trivial patch (even in master) would be to pass the channel's factory in the
> g_object_new() properties to get closer to the goal "every TpProxy has a
> factory".

Good point. I'll do that.

> > - TpClient: actually it doesn't seem to be instantiated at all by tp-glib,
> > only in its tests
> 
> Not even sure why we have that in tp-glib API at all, maybe it should be
> moved to MC since it's the only user. I think MC will always be the special
> case where relying on the lowlevel dbus API will always be needed anyway.

Because we generate low level D-Bus API for those and so need a TpProxy subclass.

> > Can be created by user using a specific API
> > -------------------------------------------
> > - TpProtocol: but bug#75881 is going to move it to the factory
> > - TpTLSCertificate: I guess it could use the factory instead
> 
> I think those need public API to be created from a factory. No need of vfunc
> for them though.

Agreed.

> > Should these 2 use the factory? I guess we could?
> > - TpChannelDispatcher
> 
> Do we need TpChannelDispatcher constructor to be public? I would consider
> that object to be created internally in tp-glib only, no?

Clients may need it to use tp_channel_dispatcher_present_channel_async().

> > - TpLogger
> 
> Same case as TpAM, I would have tp_client_factory_dup_logger() and maybe
> keep tp_logger_dup() as an helper that call tp_client_factory_dup_logger()
> on the default factory.

Agreed.

> > - TpConnectionManager: as I said on bug#75881 the connection manager is not
> > considered as a "top level object". Maybe it should be?
> 
> The factory could gain a CM-name -> TpConnectionManager table just like you
> did for TpProtocol.

I guess yeah.

> (In reply to comment #0)
> > Created internally by tp-glib without using TpClientFactory
> > -----------------------------------------------------------
> > - TpCallContent
> > - TpCallStream
> 
> Uniqueness is already guaranteed by TpCallChannel, so it's fine like that. A
> trivial patch (even in master) would be to pass the channel's factory in the
> g_object_new() properties to get closer to the goal "every TpProxy has a
> factory".

I've done this in bug #76168

(In reply to comment #0)
> - TpTLSCertificate: I guess it could use the factory instead

Bug #76175

(In reply to comment #4)
> (In reply to comment #2)
> > 2) In next it would be nice if TpProxy:factory becomes a mandatory
> > construct-only property. tp_proxy_constructed() should assert that's
> > non-null and app can then rely on tp_proxy_get_factory() to never return
> > NULL. That doesn't mean we want public tp_client_factory_dup_foo() for
> > everything. For example TpCallContent is created only internally by
> > TpCallChannel so _tp_call_content_new() can simply pass the channel's
> > factory to g_object_new().
> 
> I like the idea but smcv seemed to be keen on still being able to create
> proxies behind the factory's back.

You can always create a new factory for your proxy. So it's just making a corner case more complicated but not impossible for the benefit of having the normal code path more coherent. IMO that's good.

> > 3) dup_features() vfuncs are probably useless. TpClientFactory subclasses
> > could just call _add_foo_features() from its _constructed() method. One
> > rational for those vfunc is that TpTextChannel doesn't need to get
> > TpCallChannel features. But TpProxy will probably nicely filter that nicely
> > anyway.
> 
> Not sure. Maybe?

Maybe we can just write a unit test that prepare TP_TEXT_CHANNEL_FEATURE_SMS on a TpCallChannel and if it pass then we can rely on that and remove the vfuncs?

Note that tp_simple_client_factory_add_channel_features() doc does not speak about this case, so apps could already be adding some channel subclass specific features and they get passed to all types, so if we can't mix features like that then we need to fix something...

> > 4) Some create() vfunc could be retracted. Their only purpose is to let
> > create subclasses, and in practice we do that only for TpChannel. Maybe we
> > should just forget about subclassing TpAccount, TpConnection and TpContact
> > since nobody did it anyway and that would simplify a bit of code.
> 
> I guess if we let enough padding we could always add them later if needed. I
> suspect MC is the only who may ever need subclassing something else than
> channels.

Indeed, if we make sure to have enough padding, it's always safer to not have public API when in doubt.

> > (In reply to comment #0)
> > > Created internally by tp-glib without using TpClientFactory
> > > -----------------------------------------------------------
> > > - TpCallContent
> > > - TpCallStream
> > 
> > Uniqueness is already guaranteed by TpCallChannel, so it's fine like that. A
> > trivial patch (even in master) would be to pass the channel's factory in the
> > g_object_new() properties to get closer to the goal "every TpProxy has a
> > factory".
> 
> Good point. I'll do that.
> 
> > > - TpClient: actually it doesn't seem to be instantiated at all by tp-glib,
> > > only in its tests
> > 
> > Not even sure why we have that in tp-glib API at all, maybe it should be
> > moved to MC since it's the only user. I think MC will always be the special
> > case where relying on the lowlevel dbus API will always be needed anyway.
> 
> Because we generate low level D-Bus API for those and so need a TpProxy
> subclass.

Right, I think since MC is the only one to instantiate that, I would let MC handle it. MC will have to pass its factory to its TpClient subclass when it instantiate it.

I'm still tempted to remove that object from tp-glib. MC can do the codegen itself, especially after Simon merge its GDBus port it can be made easily with gdbus-codegen.

> > > Can be created by user using a specific API
> > > -------------------------------------------
> > > - TpProtocol: but bug#75881 is going to move it to the factory
> > > - TpTLSCertificate: I guess it could use the factory instead
> > 
> > I think those need public API to be created from a factory. No need of vfunc
> > for them though.
> 
> Agreed.
> 
> > > Should these 2 use the factory? I guess we could?
> > > - TpChannelDispatcher
> > 
> > Do we need TpChannelDispatcher constructor to be public? I would consider
> > that object to be created internally in tp-glib only, no?
> 
> Clients may need it to use tp_channel_dispatcher_present_channel_async().

Ok, so I think it needs to go through the factory as well to have unique TpChannelDispatcher proxy. Just like TpAM it could even have a tp_channel_dispatcher_dup() helper that return a singleton from the default factory. I would remove tp_channel_dispatcher_new() since in normal code path you always want the singleton. Right?

-- 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/121.