Bug 49372

(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.