|
Description
Guillaume Desmottes
2011-06-13 06:43:10 UTC
Ignore that. Created attachment 48376 [details] [review] simple-channel-request: export tp_tests_simple_channel_request_dup_immutable_props() Created attachment 48377 [details] [review] TpBaseClient: add org.freedesktop.Telepathy.ChannelRequest.DelegateToPreferredHandler support (#38249) Created attachment 48378 [details] [review] add tp_account_channel_request_set_delegate_to_preferred_handler() This is all good, but you forgot support for ::re-delegated in TpAccountChannelRequest for when someone has called *_and_handle_async(). Created attachment 48760 [details] [review] TpAccountChannelRequest: add org.freedesktop.Telepathy.ChannelRequest.DelegateToPreferredHandler support (#38249) Review of attachment 48760 [details] [review]: Did you forget to add account-channel-request-internal ? ::: docs/reference/Makefile.am @@ +65,3 @@ IGNORE_HFILES=\ add-dispatch-operation-context-internal.h \ + account-channel-request--internal.h \ Double dash? ::: telepathy-glib/account-channel-request.c @@ +851,3 @@ + g_assert (TP_IS_CHANNEL (channel)); + g_assert_cmpstr (tp_proxy_get_object_path (channel), ==, + Are assertions appropriate here, or would g_return_if_fail() be better? @@ +1673,3 @@ + * channels. + * + * Is there a word missing here? @@ +1682,3 @@ + * channel. + * + * When receiving a request containing this hint, @self will automatically Consider a See Also: to the TpBaseClient description? (In reply to comment #9) > Review of attachment 48760 [details] [review]: > > Did you forget to add account-channel-request-internal ? I did :( > ::: docs/reference/Makefile.am > @@ +65,3 @@ > IGNORE_HFILES=\ > add-dispatch-operation-context-internal.h \ > + account-channel-request--internal.h \ > > Double dash? Ooops, fixed; > ::: telepathy-glib/account-channel-request.c > @@ +851,3 @@ > + g_assert (TP_IS_CHANNEL (channel)); > + g_assert_cmpstr (tp_proxy_get_object_path (channel), ==, > + > > Are assertions appropriate here, or would g_return_if_fail() be better? g_return_if_fail() is probably better indeed; changed. > @@ +1673,3 @@ > + * channels. > + * > + * > > Is there a word missing here? ?? Btw, I renamed it to tp_account_channel_request_set_delegated_channel_callback (singular) as there is only one channel being delegated in this case. > @@ +1682,3 @@ > + * channel. > + * > + * When receiving a request containing this hint, @self will automatically > > Consider a See Also: to the TpBaseClient description? done. Created attachment 48843 [details] [review] TpAccountChannelRequest: add org.freedesktop.Telepathy.ChannelRequest.DelegateToPreferredHandler support (#38249) Review of attachment 48843 [details] [review]: ::: telepathy-glib/account-channel-request.c @@ +1676,3 @@ + * tp_account_channel_request_create_and_handle_channel_async () or + * tp_account_channel_request_ensure_and_observe_channel_async() has been + * the org.freedesktop.Telepathy.ChannelRequest.DelegateToPreferredHandler This is really awkwardly worded, it took me 4 rereadings to work out what you were saying. Perhaps simply, "@callback may be called any time after (and only after) requesting and handling the channel (i.e. you have called create_and_handle or ensure_and_handle). N.B. you have written ensure_and_observe, which is wrong. Agreed that's better. I used that wording and merged the branch. Thanks for all the reviewing! |
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.