|
Description
Xavier Claessens
2012-04-27 04:38:03 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. 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. 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. Oh and in 'next' "contact-chat-state-changed" should be renamed back to "chat-state-changed" IMO. 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. 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. (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. > +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. (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. (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() 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. Created attachment 60827 [details] [review] TpMessageMixin: add helpers to implement ChatState Created attachment 60828 [details] [review] Implement ChatState in test echo channel Created attachment 60829 [details] [review] Add ChatState unit tests Created attachment 60830 [details] [review] Add comment explaining what it means if feature->prepare_async is NULL 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. Created attachment 60885 [details] [review] TpMessageMixin: add helpers to implement ChatState Created attachment 60886 [details] [review] Implement ChatState in test echo channel Created attachment 60887 [details] [review] Add ChatState unit tests Created attachment 60888 [details] [review] Add comment explaining what it means if feature->prepare_async is NULL tp-glib: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=chat-state Gabble: http://cgit.collabora.com/git/user/xclaesse/telepathy-gabble.git/log/?h=chat-state 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. 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.