[Bug 76111] [next] Make TpProxy subclasses API more coherent

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=76111

--- Comment #7 from Xavier Claessens <xclaesse at gmail.com> ---
(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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.