TpAccount, TpConnection and TpChannel (+subclasses) are created through the factory now, to ensure uniqueness. I think their _new() constructors should be deprecated and they should assert that TpProxy:factory property is non-NULL in constructed(), instead of creating a new factory in that case.
(In reply to comment #0) > I think their _new() constructors should be deprecated and they should assert > that TpProxy:factory property is non-NULL in constructed(), instead of creating > a new factory in that case. The assert() can be added in 'next' but not in master.
Speaking about those proxy construction, I made those helpers: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factory-async They could be useful to MC.
The commits should ideally reference this bug number (git rebase --interactive and use reword - I usually follow Debian's DEP-3 and use "Bug: https://bugs.freedesktop.org/show_bug.cgi?id=49372" as a footer in the same way as Signed-off-by). In "Deprecate tp_account_parse_object_path()" you seem to have incorporated the switch from tp_account_ensure_connection to tp_simple_client_factory_ensure_connection, which should ideally have been in the following commit. (Not a merge blocker, obviously.) > +_TP_DEPRECATED_IN_UNRELEASED > TpConnection *tp_account_ensure_connection (TpAccount *account, Shouldn't this be _TP_DEPRECATED_IN_UNRELEASED_FOR (tp_simple_client_factory_ensure_connection)? In the Automatic factory: > - * @dbus: a #TpDBusDaemon > + * @dbus: (allow-none): a #TpDBusDaemon, or %NULL Should document the equivalence to tp_dbus_daemon_dup() like you did for the Simple version. In tests/lib: > + factory = (TpSimpleClientFactory *) tp_automatic_client_factory_new (dbus); For regression tests I think we should be using the Simple version unless there's a specific reason not to. (Perhaps have a nullable parameter?) (In reply to comment #0) > TpAccount, TpConnection and TpChannel (+subclasses) are created through the > factory now, to ensure uniqueness. > > I think their _new() constructors should be deprecated Doesn't this mean that it's impossible (or unnecessarily hard - they have to use g_object_new and remember which properties are mandatory) for library users to implement their own factories without calling deprecated methods? That seems undesirable. I agree that the documentation of those functions should say something like: This function does not ensure that duplicate objects are not created, and should only be used when implementing #TpSimpleClientFactory subclasses. In normal client code, you should construct new #TpChannel objects from a #TpSimpleClientFactory object, or from an instance of a subclass like #TpAutomaticClientFactory, using the tp_simple_client_factory_ensure_channel() method. (Adapt as necessary for non-Channel classes.) Deprecating tp_channel_new() is fine, though, since there's a better alternative.
(In reply to comment #3) > (In reply to comment #0) > > TpAccount, TpConnection and TpChannel (+subclasses) are created through the > > factory now, to ensure uniqueness. > > > > I think their _new() constructors should be deprecated > > Doesn't this mean that it's impossible (or unnecessarily hard - they have to > use g_object_new and remember which properties are mandatory) for library users > to implement their own factories without calling deprecated methods? That seems > undesirable. If a library implement its own factory subclass, and override create_connection() then it is to return a TpConnection subclass anyway. So it will have to make its own my_connection_subclass_new() internally, just like we have _tp_connection_new_with_factory().
Created attachment 61954 [details] [review] Deprecate tp_account_parse_object_path()
Created attachment 61955 [details] [review] Deprecate tp_account_ensure_connection()
Created attachment 61956 [details] [review] tp_simple/automatic_client_factory_new: allow NULL TpDBusDaemon
Created attachment 61957 [details] [review] Examples: stop using tp_account/connection/channel_new()
Created attachment 61958 [details] [review] tests/lib: Stop using tp_connection_new()
Created attachment 61959 [details] [review] Deprecate tp_account_new(), tp_connection_new() and tp_*_channel_new() Those proxies should be constructed using TpSimpleClientFactory
(In reply to comment #3) > The commits should ideally reference this bug number (git rebase --interactive > and use reword - I usually follow Debian's DEP-3 and use "Bug: > https://bugs.freedesktop.org/show_bug.cgi?id=49372" as a footer in the same way > as Signed-off-by). Fixed by using git bz. > In "Deprecate tp_account_parse_object_path()" you seem to have incorporated the > switch from tp_account_ensure_connection to > tp_simple_client_factory_ensure_connection, which should ideally have been in > the following commit. (Not a merge blocker, obviously.) Fixed > > +_TP_DEPRECATED_IN_UNRELEASED > > TpConnection *tp_account_ensure_connection (TpAccount *account, > > Shouldn't this be _TP_DEPRECATED_IN_UNRELEASED_FOR > (tp_simple_client_factory_ensure_connection)? Fixed > In the Automatic factory: > > - * @dbus: a #TpDBusDaemon > > + * @dbus: (allow-none): a #TpDBusDaemon, or %NULL > > Should document the equivalence to tp_dbus_daemon_dup() like you did for the > Simple version. Fixed > In tests/lib: > > + factory = (TpSimpleClientFactory *) tp_automatic_client_factory_new (dbus); > > For regression tests I think we should be using the Simple version unless > there's a specific reason not to. (Perhaps have a nullable parameter?) This is currently used by test-dbus-tube, because it does a tp_simple_client_factory_ensure_channel() with connection's factory to get a TpDBusTubeChannel. It's currently the only test doing that, because other tests are still using the now deprecated constructors. I think this is the right thing to do. Note that tp_connection_new() already creates an automatic factory internally.
Comment on attachment 61957 [details] [review] Examples: stop using tp_account/connection/channel_new() Review of attachment 61957 [details] [review]: ----------------------------------------------------------------- ::: examples/client/extended-client.c @@ +211,5 @@ > > if (die_if (error, "RequestConnection()")) > return; > > /* FIXME: there should be convenience API for this */ Is this still applicable? Probably not, because real applications (except MC) shouldn't be using RequestConnection anyway. @@ +212,5 @@ > if (die_if (error, "RequestConnection()")) > return; > > /* FIXME: there should be convenience API for this */ > + factory = tp_simple_client_factory_new (NULL); Isn't this leaked?
Comment on attachment 61958 [details] [review] tests/lib: Stop using tp_connection_new() Review of attachment 61958 [details] [review]: ----------------------------------------------------------------- Good, except: ::: tests/lib/util.c @@ +205,5 @@ > &name, &conn_path, &error)); > g_assert_no_error (error); > > + *client_conn = tp_simple_client_factory_ensure_connection (factory, > + conn_path, NULL, &error);; Double semicolon (empty statement) will warn in some compilers
Comment on attachment 61959 [details] [review] Deprecate tp_account_new(), tp_connection_new() and tp_*_channel_new() Review of attachment 61959 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/automatic-proxy-factory.c @@ +115,5 @@ > G_IMPLEMENT_INTERFACE (TP_TYPE_CLIENT_CHANNEL_FACTORY, > client_proxy_factory_iface_init)) > > +/* Deprecated module can use deprecated APIs */ > +G_GNUC_BEGIN_IGNORE_DEPRECATIONS Is this module really deprecated? I thought this one was the non-deprecated version? I would prefer these guards to be around smaller blocks of code (i.e. the actual constructor). Even better would be to have: /* internal, called by the proxy factory */ TpStreamTubeChannel *_tp_stream_tube_channel_new (...); /* extern, deprecated, to be deleted in next */ TpStreamTubeChannel *tp_stream_tube_channel_new (...); TpStreamTubeChannel * tp_stream_tube_channel_new (...) { _tp_stream_tube_channel_new (...); } since we're presumably going to want _tp_stream_tube_channel_new in next anyway. ::: telepathy-glib/client-channel-factory.c @@ +75,5 @@ > G_DEFINE_INTERFACE(TpClientChannelFactory, tp_client_channel_factory, > G_TYPE_OBJECT) > > +/* Deprecated module can use deprecated APIs */ > +G_GNUC_BEGIN_IGNORE_DEPRECATIONS See above, although at least this one is actually deprecated...
(In reply to comment #12) > Comment on attachment 61957 [details] [review] [review] > Examples: stop using tp_account/connection/channel_new() > > Review of attachment 61957 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: examples/client/extended-client.c > @@ +211,5 @@ > > > > if (die_if (error, "RequestConnection()")) > > return; > > > > /* FIXME: there should be convenience API for this */ > > Is this still applicable? Probably not, because real applications (except MC) > shouldn't be using RequestConnection anyway. True, lots of examples are/contains not really exemplary code. I reported bug #51687 for that, let's consider it out of scope for this bug. > @@ +212,5 @@ > > if (die_if (error, "RequestConnection()")) > > return; > > > > /* FIXME: there should be convenience API for this */ > > + factory = tp_simple_client_factory_new (NULL); > > Isn't this leaked? Good catch.
(In reply to comment #14) > Comment on attachment 61959 [details] [review] [review] > Deprecate tp_account_new(), tp_connection_new() and tp_*_channel_new() > > Review of attachment 61959 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-glib/automatic-proxy-factory.c > @@ +115,5 @@ > > G_IMPLEMENT_INTERFACE (TP_TYPE_CLIENT_CHANNEL_FACTORY, > > client_proxy_factory_iface_init)) > > > > +/* Deprecated module can use deprecated APIs */ > > +G_GNUC_BEGIN_IGNORE_DEPRECATIONS > > Is this module really deprecated? I thought this one was the non-deprecated > version? TpAutomaticClientFactory is the new one, TpAutomaticProxyFactory has been deprecated for a long time now. > I would prefer these guards to be around smaller blocks of code (i.e. the > actual constructor). > > Even better would be to have: > > /* internal, called by the proxy factory */ > TpStreamTubeChannel *_tp_stream_tube_channel_new (...); > > /* extern, deprecated, to be deleted in next */ > TpStreamTubeChannel *tp_stream_tube_channel_new (...); > > TpStreamTubeChannel * > tp_stream_tube_channel_new (...) > { > _tp_stream_tube_channel_new (...); > } > > since we're presumably going to want _tp_stream_tube_channel_new in next > anyway. We already have _tp_foo_new_with_factory() that are used with the new factory system. Exactly in that idea ;-) > ::: telepathy-glib/client-channel-factory.c > @@ +75,5 @@ > > G_DEFINE_INTERFACE(TpClientChannelFactory, tp_client_channel_factory, > > G_TYPE_OBJECT) > > > > +/* Deprecated module can use deprecated APIs */ > > +G_GNUC_BEGIN_IGNORE_DEPRECATIONS > > See above, although at least this one is actually deprecated... See above :)
Fixed review comments and merged. Thanks.
Reopening, there is also tp_channel_dispatch_operation_new() and tp_channel_request_new() which could disappear as well.
patch to deprecate them in master: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=deprecate-ctor
Merged.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.