Bug 45842

Summary: deprecate call_when_ready in master, delete it from 1.0
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: jonny.lamb
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/telepathy-glib/commit?h=next-cm-cwr-45842
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 31668    
Attachments: Don't use tp_channel_call_when_ready, except in its regression tests
Use tp_proxy_prepare_async() to prepare connections in example code
Deprecate tp_channel_call_when_ready, tp_connection_call_when_ready
Deprecate tp_channel_is_ready and TpChannel:channel-ready
TpContact: stop using about-to-be-deprecated Connection API
Deprecate tp_connection_is_ready, TpConnection:connection-ready
inspect-cm example: stop using call_when_ready
tests/dbus/cm: use g_assert_no_error for better diagnostics
Improve coverage of TpConnectionManager test
tp_list_connection_managers: use tp_proxy_prepare_async
Deprecate tp_connection_manager_call_when_ready and is_ready
[next] Remove TpChannel-specific readiness, just use TpProxy preparation
[next] Remove TpConnection-specific readiness, just use TpProxy preparation
[next] Remove tp_connection_manager_call_when_ready, is_ready

Description Simon McVittie 2012-02-09 07:46:03 UTC
tp_foo_call_when_ready() etc. can be superseded by tp_proxy_prepare_async() etc.
Comment 1 Simon McVittie 2012-03-02 11:27:59 UTC
Created attachment 57940 [details] [review]
Don't use tp_channel_call_when_ready, except in its  regression tests
Comment 2 Simon McVittie 2012-03-02 11:28:18 UTC
Created attachment 57941 [details] [review]
Use tp_proxy_prepare_async() to prepare connections in  example code
Comment 3 Simon McVittie 2012-03-02 11:28:48 UTC
Created attachment 57942 [details] [review]
Deprecate tp_channel_call_when_ready,  tp_connection_call_when_ready
Comment 4 Simon McVittie 2012-03-02 11:29:01 UTC
Created attachment 57943 [details] [review]
Deprecate tp_channel_is_ready and  TpChannel:channel-ready
Comment 5 Simon McVittie 2012-03-02 11:29:18 UTC
Created attachment 57944 [details] [review]
TpContact: stop using about-to-be-deprecated Connection  API
Comment 6 Simon McVittie 2012-03-02 11:29:35 UTC
Created attachment 57945 [details] [review]
Deprecate tp_connection_is_ready,  TpConnection:connection-ready
Comment 7 Simon McVittie 2012-03-02 11:29:55 UTC
Created attachment 57946 [details] [review]
inspect-cm example: stop using call_when_ready
Comment 8 Simon McVittie 2012-03-02 11:30:24 UTC
(Still to do: actually deprecate tp_connection_manager_call_when_ready())
Comment 9 Jonny Lamb 2012-03-02 14:29:55 UTC
All these patches so far look good.
Comment 10 Simon McVittie 2012-03-05 04:59:25 UTC
(In reply to comment #9)
> All these patches so far look good.

Thanks, merged. More to come...
Comment 11 Simon McVittie 2012-03-05 07:59:09 UTC
Created attachment 58024 [details] [review]
tests/dbus/cm: use g_assert_no_error for better  diagnostics

---

I have a branch in progress to do this to the other tests, but this bit seemed likely to conflict.
Comment 12 Simon McVittie 2012-03-05 08:00:43 UTC
Created attachment 58025 [details] [review]
Improve coverage of TpConnectionManager test

* test prepare_async in every situation where we previously tested
  call_when_ready
* test is_prepared (CORE) in the same situations
* test get_invalidated in the same situations

This brings us closer to being able to deprecate call_when_ready
and is_ready.
Comment 13 Simon McVittie 2012-03-05 08:01:05 UTC
Created attachment 58026 [details] [review]
tp_list_connection_managers: use tp_proxy_prepare_async

We're about to deprecate call_when_ready, so let's not call it.
Comment 14 Simon McVittie 2012-03-05 08:01:25 UTC
Created attachment 58027 [details] [review]
Deprecate tp_connection_manager_call_when_ready and  is_ready
Comment 15 Simon McVittie 2012-03-05 08:25:00 UTC
Created attachment 58031 [details] [review]
[next] Remove TpChannel-specific readiness, just use TpProxy  preparation
Comment 16 Simon McVittie 2012-03-05 08:25:20 UTC
Created attachment 58032 [details] [review]
[next] Remove TpConnection-specific readiness, just use TpProxy  preparation
Comment 17 Simon McVittie 2012-03-05 08:27:58 UTC
gitwebs:

http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=deprecate-when-ready-45842
http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=next-remove-cwr-45842

Still to do: remove tp_connection_manager_call_when_ready from next (after merging master, with the patches from this bug applied).
Comment 18 Jonny Lamb 2012-03-05 15:08:41 UTC
Comment on attachment 58026 [details] [review]
tp_list_connection_managers: use tp_proxy_prepare_async

Review of attachment 58026 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/connection-manager.c
@@ +1676,5 @@
>    size_t base_len;
>    gsize refcount;
>    gsize cms_to_ready;
>    unsigned getting_names:1;
> +  unsigned had_weak_object:1;

Are we really still using this instead of gboolean in new code?

@@ +1732,4 @@
>        g_ptr_array_add (list_context->arr, NULL);
>        cms = (TpConnectionManager **) list_context->arr->pdata;
>  
> +      if (!list_context->had_weak_object || list_context->weak_object != NULL)

I don't understand this. Call the callback if:

 * there was no weak object (regardless of whether it's been disposed already or not)
 * OR, there is a weak object around right now

So the only case where the callback won't get called is if:

 * had_weak_object is TRUE
 * AND, the weak object is not around.

Okay so this now makes sense: it'll only call the callback if the weak object is still around. Would it be possible to get a comment to explain this a bit more though please?
Comment 19 Jonny Lamb 2012-03-05 15:09:16 UTC
Every other patch looks good to me. If you could add another comment I'd appreciate it.
Comment 20 Simon McVittie 2012-03-06 02:44:25 UTC
(In reply to comment #18)
> >    unsigned getting_names:1;
> > +  unsigned had_weak_object:1;
> 
> Are we really still using this instead of gboolean in new code?

I was going for "if there's already a bitfield extend it, if not, add a gboolean".

> Okay so this now makes sense: it'll only call the callback if the weak object
> is still around. Would it be possible to get a comment to explain this a bit
> more though please?

      /* If we never had a weak object anyway, call the callback.
       * If we had a weak object when we started, only call the callback
       * if it hasn't died yet. */

I regret introducing the weak_object parameter, and look forward to the glorious GIO future, with async results, cancellables where we really really need them, and "shut up and use TpWeakRef as your user_data" for the few cases where the weak_object semantics are actually useful.
Comment 21 Simon McVittie 2012-03-06 02:58:25 UTC
Deprecation in 0.17.6, removal in next. Leaving the bug open to remove the CM readiness in next.
Comment 22 Simon McVittie 2012-03-06 03:14:52 UTC
Created attachment 58053 [details] [review]
[next] Remove tp_connection_manager_call_when_ready, is_ready
Comment 23 Jonny Lamb 2012-03-06 08:14:50 UTC
(In reply to comment #20)
> I was going for "if there's already a bitfield extend it, if not, add a
> gboolean".

I think we want to replace all these with gbooleans in next.

>       /* If we never had a weak object anyway, call the callback.
>        * If we had a weak object when we started, only call the callback
>        * if it hasn't died yet. */

Okay, thanks.
Comment 24 Jonny Lamb 2012-03-06 08:16:13 UTC
Comment on attachment 58053 [details] [review]
[next] Remove tp_connection_manager_call_when_ready, is_ready

Review of attachment 58053 [details] [review]:
-----------------------------------------------------------------

Perfecto!
Comment 25 Simon McVittie 2012-04-11 03:42:48 UTC
(In reply to comment #23)
> (In reply to comment #20)
> > I was going for "if there's already a bitfield extend it, if not, add a
> > gboolean".
> 
> I think we want to replace all these with gbooleans in next.

Bug #48544.

(In reply to comment #24)
> Perfecto!

Fixed in next.

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.