Bug 49215

Summary: Move chat state to TpTextChannel
Product: Telepathy Reporter: Xavier Claessens <xclaesse>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: xclaesse
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=chat-state
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 31668    
Attachments: Move ChatState to TpTextChannel
Move ChatState to TpTextChannel
Move ChatState to TpTextChannel
Move ChatState to TpTextChannel
TpMessageMixin: add helpers to implement ChatState
Implement ChatState in test echo channel
Add ChatState unit tests
Add comment explaining what it means if feature->prepare_async is NULL
Move ChatState to TpTextChannel
TpMessageMixin: add helpers to implement ChatState
Implement ChatState in test echo channel
Add ChatState unit tests
Add comment explaining what it means if feature->prepare_async is NULL

Description Xavier Claessens 2012-04-27 04:38:03 UTC
ChatState iface requires Type.Text, so tp_channel_get_chat_state() has nothing to do on TpChannel.

Also it should take a TpContact instead of TpHandle.
Comment 1 Xavier Claessens 2012-04-27 06:17:20 UTC
Created attachment 60669 [details] [review]
Move ChatState to TpTextChannel

API on TpChannel is now deprecated but still used to implement
the corresponding API on TpTextChannel.
Comment 2 Xavier Claessens 2012-04-27 06:43:34 UTC
Created attachment 60671 [details] [review]
Move ChatState to TpTextChannel

API on TpChannel is now deprecated but still used to implement
the corresponding API on TpTextChannel.
Comment 3 Xavier Claessens 2012-04-27 06:45:21 UTC
Here is patch for master, it deprecate chat state from TpChannel and add new API on TpTextChannel. Implementation still rely on the deprecated API to avoid code duplication. For 'next' branch the code should be actually moved.
Comment 4 Xavier Claessens 2012-04-27 06:48:02 UTC
Oh and in 'next' "contact-chat-state-changed" should be renamed back to "chat-state-changed" IMO.
Comment 5 Simon McVittie 2012-04-27 08:24:50 UTC
Comment on attachment 60671 [details] [review]
Move ChatState to TpTextChannel

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

::: telepathy-glib/automatic-client-factory.c
@@ +71,5 @@
>   *   </listitem>
>   *   <listitem>
> + *     <para>%TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES,
> + *     %TP_TEXT_CHANNEL_FEATURE_SMS and %TP_TEXT_CHANNEL_FEATURE_CHAT_STATES
> + *     for #TpTextChannel</para>

Should the automatic factory really prepare this by default? IMO, no - we don't on ordinary TpChannels, after all.

It's easy to imagine a client that doesn't care about chat states (naming no loggers in particular), and preparing this by default partially defeats the purpose of having it as a separate Feature rather than just part of CORE.

SMS and Password are also questionable, tbh... in next I'm tempted to say this shouldn't prepare them.

::: telepathy-glib/text-channel.c
@@ +860,5 @@
>  
> +  features[FEAT_CHAT_STATES].name =
> +    TP_TEXT_CHANNEL_FEATURE_CHAT_STATES;
> +  depends_chat_state[0] = TP_CHANNEL_FEATURE_CHAT_STATES;
> +  features[FEAT_CHAT_STATES].depends_on = depends_chat_state;

Nice trick.
Comment 6 Xavier Claessens 2012-04-27 10:21:36 UTC
Created attachment 60698 [details] [review]
Move ChatState to TpTextChannel

API on TpChannel is now deprecated but still used to implement
the corresponding API on TpTextChannel.
Comment 7 Xavier Claessens 2012-04-27 10:22:56 UTC
(In reply to comment #5)
> Comment on attachment 60671 [details] [review] [review]
> Move ChatState to TpTextChannel
> 
> Review of attachment 60671 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: telepathy-glib/automatic-client-factory.c
> @@ +71,5 @@
> >   *   </listitem>
> >   *   <listitem>
> > + *     <para>%TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES,
> > + *     %TP_TEXT_CHANNEL_FEATURE_SMS and %TP_TEXT_CHANNEL_FEATURE_CHAT_STATES
> > + *     for #TpTextChannel</para>
> 
> Should the automatic factory really prepare this by default? IMO, no - we don't
> on ordinary TpChannels, after all.
> 
> It's easy to imagine a client that doesn't care about chat states (naming no
> loggers in particular), and preparing this by default partially defeats the
> purpose of having it as a separate Feature rather than just part of CORE.
> 
> SMS and Password are also questionable, tbh... in next I'm tempted to say this
> shouldn't prepare them.

I agree tbh, it is easy enough to add features with tp_client_factory_add_channel_features() anyway.
Comment 8 Simon McVittie 2012-04-30 05:43:44 UTC
> +TpMessageMixingSetChatStateImpl

"Mixin" not "Mixing".

> + g_return_if_fail (state < NUM_TP_CHANNEL_CHAT_STATES);

TP_NUM in new code, please (this is not the only instance, please grep).

> + g_hash_table_insert (mixin->priv->chat_states,
> + GUINT_TO_POINTER (member),
> + GUINT_TO_POINTER (state));

Please remove it if it's INACTIVE, like Gabble does, since that's "the default state". Have you tried porting Gabble 0.17 to this? I would rather not add this functionality without a proof-of-concept of a CM still passing its tests.

There should be a way for the CM to tell the mixin "this contact has gone away, remove them from your hash table". Otherwise, a chatroom will build up a big hash table over time: everyone who has ever been a member will be in the hash table (with chat state GONE, if the CM is not broken).

I think Gabble might actually treat both INACTIVE and GONE as "emit signal and remove from hash table", which seems reasonable.

> + * TpMessageMixingSetChatStateImpl:
...
> + * Signature of a virtual method which may be implemented to allow messages
> + * to be sent. It must arrange for tp_message_mixin_sent() to be called when
> + * the message has submitted or when message submission has failed.

This text does not describe this method.

Why would this method ever fail, other than InvalidArgument? (NetworkError, I suppose, and telepathy-spec also says NotAvailable, but I'm not entirely sure why).

If it fails, should this method fail synchronously or asynchronously?

+ /* I no func is set, then subclass is supposed to call

"If no function".

I think this should go in the TpProxyFeature docs, perhaps this:

@prepare_async: if not %NULL, called when preparing the feature is requested.
 May be %NULL for features whose preparation starts automatically (for
 instance from the proxy's #GObjectClass.constructed virtual method);
 if so, the subclass is still expected to call FIXME when the feature
 has been prepared.

However, the "FIXME" there still needs replacing... because out-of-tree proxies still can't usefully add features, because the ability to say "it worked!" is still internal-only API! I'll (re)open a bug.
Comment 9 Simon McVittie 2012-04-30 06:10:43 UTC
(In reply to comment #8)
> I think this should go in the TpProxyFeature docs, perhaps this:
> 
> @prepare_async: if not %NULL, called when preparing the feature is requested.
>  May be %NULL for features whose preparation starts automatically (for
>  instance from the proxy's #GObjectClass.constructed virtual method);
>  if so, the subclass is still expected to call FIXME when the feature
>  has been prepared.
> 
> However, the "FIXME" there still needs replacing... because out-of-tree proxies
> still can't usefully add features, because the ability to say "it worked!" is
> still internal-only API! I'll (re)open a bug.

On (much) closer inspection of the implementation, it goes like this.

Things outside telepathy-glib must have a non-%NULL implementation. It can do any of these:

  * Succeed. Job done.

  * Fail with a GError. That feature is marked as failed.
    Nothing else really happens, and the GError is almost ignored.
    The overall tp_proxy_prepare_async() call still succeeds.

  * Invalidate the proxy, then fail with a GError (in that order).
    The entire proxy is useless now; tp_proxy_prepare_async() fails
    with the invalidation error, and the GError from 
    TpProxyFeature.prepare_async is ignored. Useful for TpChannel,
    TpConnection and anything else stateful.

It is not correct to fail with a GError, and then immediately invalidate the proxy: that would result in tp_proxy_prepare_async() succeeding just before the proxy was invalidated.

Things inside telepathy-glib can also have a %NULL implementation. They must do exactly one of these:

  * _tp_proxy_set_feature_prepared() (positively or negatively).
    Equivalent to TpProxyFeature.prepare_async() failing.

  * _tp_proxy_set_features_failed(). The tp_proxy_prepare_async() call
    fails with that error, but the proxy is not invalidated.
    Useful for stateless proxies like TpConnectionManager (and basically
    nothing else).

  * Invalidate the proxy. As above.
Comment 10 Xavier Claessens 2012-04-30 12:12:50 UTC
(In reply to comment #8)
> 
> > + g_hash_table_insert (mixin->priv->chat_states,
> > + GUINT_TO_POINTER (member),
> > + GUINT_TO_POINTER (state));
> 
> Please remove it if it's INACTIVE, like Gabble does, since that's "the default
> state". Have you tried porting Gabble 0.17 to this? I would rather not add this
> functionality without a proof-of-concept of a CM still passing its tests.
> 
> There should be a way for the CM to tell the mixin "this contact has gone away,
> remove them from your hash table". Otherwise, a chatroom will build up a big
> hash table over time: everyone who has ever been a member will be in the hash
> table (with chat state GONE, if the CM is not broken).
> 
> I think Gabble might actually treat both INACTIVE and GONE as "emit signal and
> remove from hash table", which seems reasonable.

Gabble does not implement the property, it does not keep the state.

> > + * TpMessageMixingSetChatStateImpl:
> ...
> > + * Signature of a virtual method which may be implemented to allow messages
> > + * to be sent. It must arrange for tp_message_mixin_sent() to be called when
> > + * the message has submitted or when message submission has failed.
> 
> This text does not describe this method.
> 
> Why would this method ever fail, other than InvalidArgument? (NetworkError, I
> suppose, and telepathy-spec also says NotAvailable, but I'm not entirely sure
> why).
> 
> If it fails, should this method fail synchronously or asynchronously?

I've made it like that to match gabble_message_util_send_chat_state()
Comment 11 Xavier Claessens 2012-05-01 03:13:07 UTC
Created attachment 60826 [details] [review]
Move ChatState to TpTextChannel

API on TpChannel is now deprecated but still used to implement
the corresponding API on TpTextChannel.
Comment 12 Xavier Claessens 2012-05-01 03:13:11 UTC
Created attachment 60827 [details] [review]
TpMessageMixin: add helpers to implement ChatState
Comment 13 Xavier Claessens 2012-05-01 03:13:14 UTC
Created attachment 60828 [details] [review]
Implement ChatState in test echo channel
Comment 14 Xavier Claessens 2012-05-01 03:13:18 UTC
Created attachment 60829 [details] [review]
Add ChatState unit tests
Comment 15 Xavier Claessens 2012-05-01 03:13:22 UTC
Created attachment 60830 [details] [review]
Add comment explaining what it means if feature->prepare_async is NULL
Comment 16 Xavier Claessens 2012-05-02 02:19:01 UTC
Created attachment 60884 [details] [review]
Move ChatState to TpTextChannel

API on TpChannel is now deprecated but still used to implement
the corresponding API on TpTextChannel.
Comment 17 Xavier Claessens 2012-05-02 02:19:06 UTC
Created attachment 60885 [details] [review]
TpMessageMixin: add helpers to implement ChatState
Comment 18 Xavier Claessens 2012-05-02 02:19:10 UTC
Created attachment 60886 [details] [review]
Implement ChatState in test echo channel
Comment 19 Xavier Claessens 2012-05-02 02:19:15 UTC
Created attachment 60887 [details] [review]
Add ChatState unit tests
Comment 20 Xavier Claessens 2012-05-02 02:19:19 UTC
Created attachment 60888 [details] [review]
Add comment explaining what it means if feature->prepare_async is NULL
Comment 22 Simon McVittie 2012-05-09 09:10:13 UTC
 TpChannelChatState tp_channel_get_chat_state (TpChannel *self,
-    TpHandle contact);
+    TpHandle contact) _TP_GNUC_DEPRECATED_FOR (tp_text_channel_get_chat_state);

Ideally prefixed with _TP_DEPRECATED_IN_0_20_FOR (...) instead.

(_TP_DEPRECATED_IN_0_20_FOR has to appear as a prefix like "static", ideally on the previous line, because that's the price we pay for portability to MSVC++.)

Otherwise, the telepathy-glib branch looks good - ship it!

After a telepathy-glib release with that branch has happened, the Gabble branch also looks OK - remember to bump the dependency in configure.ac too.
Comment 23 Xavier Claessens 2012-05-09 10:22:33 UTC
fixed and merged both tp-glib and gabble. Thanks

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.