Simon mentioned for example that TP_CHANNEL_FEATURE_PASSWORD or TP_TEXT_CHANNEL_FEATURE_CHAT_STATE shouldn't be there. What we are missing is probably a clear definition of what we want there. Note that it is super easy for applications to add their own features on the factory. Let's take TpTextChannel and TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES: I think either we think that EVERY app using a TpTextChannel surely needs that feature and it should be marked as CORE and so will always be prepared, or we think that some app could use TpTextChannel without it and then probably automatic client factory shouldn't ask it neither. Note that MC should use TpClientFactory (not the automatic) and then won't get a TpTextChannel subclass, it it won't be a problem for it to set INCOMING_MESSAGES as CORE. So I think we should say that factories only require CORE features, and the question becomes: "what is CORE features?". Another example is TP_CHANNEL_FEATURE_GROUP: should it really be CORE of TpChannel? does MC care about preparing it? Given that it's not easy for app to tell the features they want now, maybe we should reconsider this.
(In reply to comment #0) > TpChannel? does MC care about preparing it? Given that it's not easy for app to ------------------------------------------------------------> now
(In reply to comment #0) > What we are missing is probably a clear definition of what we want there. IMO: "features that every application understanding the channel type will want"... > Note > that it is super easy for applications to add their own features on the > factory. ... with this (and the usual principle of "better to be conservative now and add more guarantees later") meaning that we should err on the side of fewer features. > Let's take TpTextChannel and TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES: > I think either we think that EVERY app using a TpTextChannel surely needs > that feature and it should be marked as CORE and so will always be prepared, Counter-example: an application which sends messages, but delegates to someone else for receiving. (Admittedly, the MessageSender stuff in MC makes that unnecessary.) If we think a feature should always be prepared, then it shouldn't usually be a separate feature at all, it should just be part of CORE. I think the only exceptions are for historical reasons - TpConnectionManager/TpProtocol have a feature that is always attempted but doesn't always succeed (to support old CMs which only have Parameters and not Protocols, which is irrelevant in next), and TpChannel has GROUP (as you already mentioned). In next, we should get rid of any support for pre-Protocol CMs. > or > we think that some app could use TpTextChannel without it and then probably > automatic client factory shouldn't ask it neither. Right. > Note that MC should use > TpClientFactory (not the automatic) and then won't get a TpTextChannel > subclass Indeed. For "MC" read "applications which don't understand specific channel types", in fact. > Another example is TP_CHANNEL_FEATURE_GROUP: should it really be CORE of > TpChannel? No, it shouldn't. When I made the Group stuff always be prepared (which I think was before features even existed), it was because I thought Group was going to remain as important as it was then. In fact, Group's importance is reducing over time - ContactList channels are no more, and unlike StreamedMedia, Calls aren't (usually) Groups, so Group effectively only appears on chatrooms. I've actually wondered whether to flatten it into Room... > does MC care about preparing it? It sort of does, because it uses the equivalent of leave_async() in places, but again, that's a relic of Group being more important (particularly for calls) than it is now. MC/next (when it exists) shouldn't care about Group, IMO.
(In reply to comment #0) > Note > that it is super easy for applications to add their own features on the > factory. Is this equally true for the formerly-Simple base class? For instance, if MC wants to prepare GROUP on Channels even though it's no longer flagged as always-prepared, is that a one-line change?
(In reply to comment #3) > (In reply to comment #0) > > Note > > that it is super easy for applications to add their own features on the > > factory. > > Is this equally true for the formerly-Simple base class? For instance, if MC > wants to prepare GROUP on Channels even though it's no longer flagged as > always-prepared, is that a one-line change? Was thinking about MC case in bug #49372 too... My guess is that MC currently do: new_conn = tp_connection_new(path); tp_proxy_prepare_async(new_conn, NULL, cb); My suggestions would be to have a TpClientFactory singleton in MC, then creating a connection/channel is: factory = mc_factory_dup_singleton(); new_conn = tp_client_factory_ensure_connection(factory, path); features = tp_client_factory_dup_connection_features(factory); tp_proxy_prepare_async (new_conn, features, cb); g_object_unref (factory); g_array_unref (features): I was already thinking about adding tp_client_factory_ensure_*_async() that does the extra prepare. That would already make it a little bit nicer.
Speaking about features: TP_ACCOUNT_FEATURE_CONNECTION: - That feature delay notify of TpAccount:connection until the TpConnection has been prepared with features set on factory. - I suggest killing that feature and making that behaviour as part of CORE. TP_CHANNEL_FEATURE_CONTACTS: - That feature is used to ensure CM is new enough to support all tp_channel_*_contact() functions. If CM is too old preparing that feature fails. That behaviour is not needed anymore in next. - That feature also upgrade all TpContact we had for free because CM gave handle+id, to gain factory features. This include GROUP contacts but also target/self contact. - I suggest having prepared target/self contact being part of CORE, and having prepared GROUP contacts part of TP_CHANNEL_FEATURE_GROUP. Then kill TP_CHANNEL_FEATURE_CONTACTS. Also making TP_CHANNEL_FEATURE_GROUP non-CORE.
(In reply to comment #5) > - I suggest having prepared target/self contact being part of CORE, and having > prepared GROUP contacts part of TP_CHANNEL_FEATURE_GROUP. Then kill > TP_CHANNEL_FEATURE_CONTACTS. Also making TP_CHANNEL_FEATURE_GROUP non-CORE. I agree. That would match the old experience as the handle variants of these API have now been replaced by the TpContact one.
(In reply to comment #2) > > Another example is TP_CHANNEL_FEATURE_GROUP: should it really be CORE of > > TpChannel? > > No, it shouldn't. Actually... 20:00 < smcv> xclaesse: I just realised a problem with your plan for groups :-( 20:00 < smcv> if you want to have tp_contact_is_self() 20:00 < smcv> then you have to prepare at least the self-contact of every channel 20:00 < smcv> (and keep up with change notifications, which come as RENAMED MembersChanged signals) 20:01 < smcv> otherwise tp_contact_is_self() is a lie Having said that, Bug #22965 means that tp_contact_is_self() is already potentially a lie...
Created attachment 61177 [details] [review] channel-contacts.c: reorder members-changed code Use the same order than in channel-group.c, it will be important in a future change
Created attachment 61178 [details] [review] channel-contacts.c: stop depending on channel-group.c Do our own introspection of contact sets, in preparation of removing channel-group.c.
Created attachment 61179 [details] [review] TpChannel: move group_remove_error handling to channel-contacts.c It means that TP_CHANNEL_FEATURE_CONTACTS must be prepared to get the group error.
Created attachment 61180 [details] [review] Move TpChannel::group-flags to channel-contacts.c It means TP_CHANNEL_FEATURE_CONTACTS must now be prepared to get it
Created attachment 61181 [details] [review] tp_channel_join/leave_async: require TP_CHANNEL_FEATURE_CONTACTS to work properly That feature is needed to ensure we have a self contact.
Created attachment 61182 [details] [review] Port unit tests to contacts API of TpChannel
Created attachment 61183 [details] [review] remove channel-group.c This removes TP_CHANNEL_FEATURE_GROUP and all APIs relatede to it. TP_CHANNEL_FEATURE_CONTACTS replaces it.
Created attachment 61184 [details] [review] TpChannel: rename a few Group APIs: TP_CHANNEL_FEATURE_CONTACTS -> TP_CHANNEL_FEATURE_GROUP tp_channel_group_dup_members_contacts -> tp_channel_group_dup_members tp_channel_group_dup_local_pending_contacts -> tp_channel_group_dup_local_pending tp_channel_group_get_local_pending_contact_info -> tp_channel_group_get_local_pending_info tp_channel_group_dup_remote_pending_contacts -> tp_channel_group_dup_remote_pending "group-contacts-changed" -> "group-members-changed"
Created attachment 61185 [details] [review] Update next-NEWS for TpChannel changes
Created attachment 61186 [details] [review] Rename channel-contacts.c to channel-group.c
Created attachment 61187 [details] [review] Rename channel-contacts.c to channel-group.c
Created attachment 61338 [details] [review] text-channel unit test: no need to prepare TP_CHANNEL_FEATURE_CONTACTS We are using only channel's self/target contacts which are prepared as part of CORE now
Created attachment 61339 [details] [review] channel-contacts.c: reorder members-changed code Use the same order than in channel-group.c, it will be important in a future change
Created attachment 61340 [details] [review] channel-contacts.c: stop depending on channel-group.c Do our own introspection of contact sets, in preparation of removing channel-group.c.
Created attachment 61341 [details] [review] TpChannel: move group_remove_error handling to channel-contacts.c It means that TP_CHANNEL_FEATURE_CONTACTS must be prepared to get the group error.
Created attachment 61342 [details] [review] Move TpChannel::group-flags to channel-contacts.c It means TP_CHANNEL_FEATURE_CONTACTS must now be prepared to get it
Created attachment 61343 [details] [review] tp_channel_join/leave_async: require TP_CHANNEL_FEATURE_CONTACTS to work properly That feature is needed to ensure we have a self contact.
Created attachment 61344 [details] [review] Port unit tests to contacts API of TpChannel
Created attachment 61345 [details] [review] remove channel-group.c This removes TP_CHANNEL_FEATURE_GROUP and all APIs relatede to it. TP_CHANNEL_FEATURE_CONTACTS replaces it.
Created attachment 61346 [details] [review] TpChannel: rename a few Group APIs: TP_CHANNEL_FEATURE_CONTACTS -> TP_CHANNEL_FEATURE_GROUP tp_channel_group_dup_members_contacts -> tp_channel_group_dup_members tp_channel_group_dup_local_pending_contacts -> tp_channel_group_dup_local_pending tp_channel_group_get_local_pending_contact_info -> tp_channel_group_get_local_pending_info tp_channel_group_dup_remote_pending_contacts -> tp_channel_group_dup_remote_pending "group-contacts-changed" -> "group-members-changed"
Created attachment 61347 [details] [review] Update next-NEWS for TpChannel changes
Created attachment 61348 [details] [review] Rename channel-contacts.c to channel-group.c
Comment on attachment 61338 [details] [review] text-channel unit test: no need to prepare TP_CHANNEL_FEATURE_CONTACTS Review of attachment 61338 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61339 [details] [review] channel-contacts.c: reorder members-changed code Review of attachment 61339 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61340 [details] [review] channel-contacts.c: stop depending on channel-group.c Review of attachment 61340 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61341 [details] [review] TpChannel: move group_remove_error handling to channel-contacts.c Review of attachment 61341 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61342 [details] [review] Move TpChannel::group-flags to channel-contacts.c Review of attachment 61342 [details] [review]: ----------------------------------------------------------------- ++ ::: telepathy-glib/channel-contacts.c @@ +1180,5 @@ > + * > + * <!-- --> > + * > + * Returns: the value of #TpChannel:group-flags > + * Since: 0.UNRELEASED "Since: UNRELEASED" as this won't be part of the 0.x series?
Comment on attachment 61343 [details] [review] tp_channel_join/leave_async: require TP_CHANNEL_FEATURE_CONTACTS to work properly Review of attachment 61343 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/channel.c @@ +2011,5 @@ > > result = g_simple_async_result_new (G_OBJECT (self), callback, > user_data, tp_channel_leave_async); > > + if (!tp_proxy_is_prepared (self, TP_CHANNEL_FEATURE_CONTACTS)) What happen if CONTACTS is prepared but channel doesn't implement Group? Looks like we'll use Group anyway now.
Comment on attachment 61344 [details] [review] Port unit tests to contacts API of TpChannel Review of attachment 61344 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61345 [details] [review] remove channel-group.c Review of attachment 61345 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61346 [details] [review] TpChannel: rename a few Group APIs: Review of attachment 61346 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/channel.c @@ +156,2 @@ > * > + * - the initial value of the #TpChannel:group-self-handle property will do we still have group-self-handle? ::: telepathy-glib/channel.h @@ +140,5 @@ > TpChannelGroupFlags tp_channel_group_get_flags (TpChannel *self); > > _TP_AVAILABLE_IN_0_16 > TpContact *tp_channel_group_get_self_contact (TpChannel *self); > _TP_AVAILABLE_IN_0_16 Shouldn't we update the version macro as that's basically a new method with the old name?
Comment on attachment 61347 [details] [review] Update next-NEWS for TpChannel changes Review of attachment 61347 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 61348 [details] [review] Rename channel-contacts.c to channel-group.c Review of attachment 61348 [details] [review]: ----------------------------------------------------------------- ++
(In reply to comment #34) > Comment on attachment 61342 [details] [review] [review] > Move TpChannel::group-flags to channel-contacts.c > > Review of attachment 61342 [details] [review] [review]: > ----------------------------------------------------------------- > > ++ > > ::: telepathy-glib/channel-contacts.c > @@ +1180,5 @@ > > + * > > + * <!-- --> > > + * > > + * Returns: the value of #TpChannel:group-flags > > + * Since: 0.UNRELEASED > > "Since: UNRELEASED" as this won't be part of the 0.x series? Jonny starting using 0.UNRELEASED, maybe in case we do 0.99? anyway that will be fixed at release time I guess.
(In reply to comment #35) > Comment on attachment 61343 [details] [review] [review] > tp_channel_join/leave_async: require TP_CHANNEL_FEATURE_CONTACTS to work > properly > > Review of attachment 61343 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-glib/channel.c > @@ +2011,5 @@ > > > > result = g_simple_async_result_new (G_OBJECT (self), callback, > > user_data, tp_channel_leave_async); > > > > + if (!tp_proxy_is_prepared (self, TP_CHANNEL_FEATURE_CONTACTS)) > > What happen if CONTACTS is prepared but channel doesn't implement Group? Looks > like we'll use Group anyway now. Since attachment #61340 [details] [review], CONTACTS fails to prepare if there is no GROUP iface.
(In reply to comment #38) > Comment on attachment 61346 [details] [review] [review] > TpChannel: rename a few Group APIs: > > Review of attachment 61346 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-glib/channel.c > @@ +156,2 @@ > > * > > + * - the initial value of the #TpChannel:group-self-handle property will > > do we still have group-self-handle? No, fixed. > ::: telepathy-glib/channel.h > @@ +140,5 @@ > > TpChannelGroupFlags tp_channel_group_get_flags (TpChannel *self); > > > > _TP_AVAILABLE_IN_0_16 > > TpContact *tp_channel_group_get_self_contact (TpChannel *self); > > _TP_AVAILABLE_IN_0_16 > > Shouldn't we update the version macro as that's basically a new method with the > old name? True, reset them all to UNRELEASED
pushed all.
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.