+++ This bug was initially created as a clone of Bug #54879 +++ Xavier wrote: > Before starting next branch everywhere, please make master build with: > AC_DEFINE(TP_SEAL_ENABLE, [], [Prevent to use sealed variables]) > AC_DEFINE(TP_DISABLE_SINGLE_INCLUDE, [], [Disable single header include]) > AC_DEFINE(TP_VERSION_MIN_REQUIRED, TP_VERSION_0_20, [Ignore post 0.20 > deprecations]) > AC_DEFINE(TP_VERSION_MAX_ALLOWED, TP_VERSION_0_20, [Prevent post 0.20 APIs]) which I think is fair enough really. MC really hasn't been keeping up.
Created attachment 67778 [details] [review] make-valid test: make connection object paths distinct If these object paths are the same, the factory will use the same TpConnection (even though they are actually different connections) and dispatching will happen twice. --- From Jonny, Reviewed-by: me, attached here for completeness.
Created attachment 67779 [details] [review] exec-with-log.sh: add gdb wrapper Based on a patch from Jonny Lamb; changed to use ${abs_top_srcdir} to work out-of-tree, backtrace all threads, and put the gdb script in /tools.
Created attachment 67780 [details] [review] mc-debug-server: don't self-terminate when disconnected from GDBus It appears something in MC now connects to D-Bus for a second time, using GDBus this time. We don't want to exit-on-disconnect for this one either, again so we free all memory before exiting.
Created attachment 67781 [details] [review] add more telepathy-glib-dbus.h includes where appropriate --- From Jonny, Reviewed-by: me.
Created attachment 67782 [details] [review] WARNING, etc. macros: add --- I haven't used them everywhere yet, because life's too short.
Created attachment 67783 [details] [review] replace sealed struct members with getters and use TpProtocol Originally two patches for 'next' by Jonny Lamb.
Created attachment 67784 [details] [review] Enable TP_SEAL_ENABLE Tested with telepathy-glib 0.19.9. --- I'm pretty sure we didn't actually deprecate anything new in 0.19.10, which is a release candidate for 0.20.
Created attachment 67785 [details] [review] Update code generation tools from telepathy-glib 0.19.9 --- Similarly, I think they're the same in 0.19.10.
Created attachment 67786 [details] [review] Enable TP_DISABLE_SINGLE_INCLUDE Tested with telepathy-glib 0.19.9. --- ... but probably OK with 0.19.10 too.
Created attachment 67787 [details] [review] mc-tool: use tp_account_manager_dup_valid_accounts tp_account_manager_get_valid_accounts has the confusing (transfer container) transfer-mode, and is deprecated in telepathy-glib 0.19.9.
Created attachment 67788 [details] [review] Use tp_channel_get_connection instead of tp_channel_borrow_connection This requires telepathy-glib 0.19.9, so depend on it.
Created attachment 67789 [details] [review] McdChannel: use tp_channel_get_requested instead of reinventing it MC's reinvention was potentially better if the channel might lack the Requested property... but that property has been mandatory for years, so CMs that haven't caught up can just deal with it.
Created attachment 67790 [details] [review] Generally use GVariant, not GHashTable, for channels' properties This means we can avoid the deprecated tp_channel_borrow_immutable_properties().
Created attachment 67791 [details] [review] McdAccountManager: use tp_dbus_daemon_register_object dbus_g_connection_register_g_object isn't deprecated, but it's more code.
Created attachment 67792 [details] [review] McdAccountManager: have a TpSimpleClientFactory
Created attachment 67793 [details] [review] McdChannel: use the TpConnection's factory to make TpChannel objects
Created attachment 67794 [details] [review] McpDispatchOperation: use a new factory for each connection This avoids the deprecated tp_channel_new_from_properties(). It has the same disadvantages as that function, but is a bit more obvious about it. Also comment how to change things at the next ABI break.
Created attachment 67795 [details] [review] mc-tool: have a TpAutomaticClientFactory to manufacture accounts
Created attachment 67796 [details] [review] McdConnection: use TpWeakRef instead of replicating it --- Not strictly a deprecation fix, but the same general theme.
Created attachment 67797 [details] [review] _mcd_channel_depart: use tp_channel_leave_async if it's a Group --- Depends on Bug #55392.
That's all for now. Those patches are not enough to be able to turn on fatal deprecation warnings for 0.20 yet, but they're heading that way. This is firmly in 5.15 territory: too much churn for (what's about to be) a stable branch. Other things to do include: McdConnection has quite a lot of tp_connection_get_self_handle(). The simple fix would be to copy that function's implementation every time we call it - but it would be better if we used a TpContact for the self-contact, attached it to the McdAccount instead of the McdConnection, and used high-level APIs instead. This should be done separately for aliasing, presence, avatars, and any other offending interfaces. When we've done them all, that'll be several fewer things between here and deleting McdConnection altogether. (I don't like McdConnection - it and McdAccount have lots of circular dependencies, and its life-cycle is pretty weird, since the same one is used across multiple attempts to reconnect after an error, but deliberate disconnection does kill it off. Make your mind up...) Elsewhere, there's the emergency call stuff that Jonny looked at in next (use of deprecated functions + lack of a regression test + not actually working). I hoped that we could just delete it because it only affected mcd_* APIs that can no longer be called from out-of-tree, but in fact it does affect MC's behaviour: plugins aren't given the opportunity to reject or delay outgoing emergency calls. However, it's debatable how useful this is, given that plugins have to be 100% trusted anyway (they share our address space) and in any case, I don't think it would need to map IDs to handles at all if we just documented that if you make an emergency call and want it to get this special treatment, you must do so by putting TargetID and/or InitialServicePoint in the request.
(In reply to comment #21) > if we just documented that > if you make an emergency call and want it to get this special treatment, you > must do so by putting TargetID and/or InitialServicePoint in the request. ... as opposed to TargetHandle. (Given that you find out about service points by asking Conn.I.ServicePoint and receiving a Service_Point and a bunch of IDs, and the other likely way to call a service point is because the user typed 112 or whatever into a dialpad, you'd have to be pretty perverse to initiate a call to one in a handle-based way...)
Comment on attachment 67795 [details] [review] mc-tool: have a TpAutomaticClientFactory to manufacture accounts Review of attachment 67795 [details] [review]: ----------------------------------------------------------------- ::: util/mc-tool.c @@ +1429,5 @@ > app_name, command.common.name, error->message); > goto out; > } > + client_factory = TP_SIMPLE_CLIENT_FACTORY ( > + tp_automatic_client_factory_new (dbus)); we don't need an automatic factory here, the simple is enough. @@ +1450,1 @@ > indentation looks weird, is there a mix of spaces and tabs here?
(In reply to comment #23) > we don't need an automatic factory here, the simple is enough. True. I can switch it to the simpler version if you prefer. > @@ +1450,1 @@ > > > > indentation looks weird, is there a mix of spaces and tabs here? Yes. The coding style in that file is pretty weird all round; I didn't want to run it through indent(1) or anything, at least not right now. This particular patch band goes like this: ⇥⇥⇥⇥⇥⇥⇥⇥command.common.account = ensure_prefix (command.common.account); -⇥⇥⇥⇥⇥⇥⇥⇥a = tp_account_new (dbus, command.common.account, &error); + a = tp_simple_client_factory_ensure_account (client_factory, + command.common.account, NULL, &error); where ⇥⇥⇥⇥⇥⇥⇥⇥ represents a tab.
(In reply to comment #2) > Created attachment 67779 [details] [review] [review] > exec-with-log.sh: add gdb wrapper > > Based on a patch from Jonny Lamb; changed to use ${abs_top_srcdir} to > work out-of-tree, backtrace all threads, and put the gdb script in /tools. Should be documented in tests/twisted/README. run_and_bt.gdb could be moved to tests/twisted/tools to not be mixed with tp-glib tools (or it could be added to tp-glib as well actually)
Comment on attachment 67783 [details] [review] replace sealed struct members with getters and use TpProtocol Review of attachment 67783 [details] [review]: ----------------------------------------------------------------- ::: src/mcd-account.c @@ +2392,5 @@ > > callback (account, error, user_data); > g_clear_error (&error); > + g_list_free_full (params, > + (GDestroyNotify) tp_connection_manager_param_free); We do 4 spaces indent in this case nowadays. But I can accept if you say the coding style is broken everywhere in that file anyway :)
Comment on attachment 67785 [details] [review] Update code generation tools from telepathy-glib 0.19.9 Review of attachment 67785 [details] [review]: ----------------------------------------------------------------- Are there changes between 0.19.9 and 0.20.0? If not, just change commit msg to tell it is 0.20.0 stable generator.
Comment on attachment 67790 [details] [review] Generally use GVariant, not GHashTable, for channels' properties Review of attachment 67790 [details] [review]: ----------------------------------------------------------------- I'm not sure I like the double conversion gvalue->gvariant->gvalue, it adds extra source of potential bugs and wast of memory/cpu. I'm wondering if this isn't premature for C code. But we are not writing a kernel so I guess perf issue is irrelevant. ::: src/mcd-client.c @@ +1570,5 @@ > + /* FIXME: when Xavier's tp_vardict_get_*() functions have landed, > + * make _mcd_client_match_property use those on variant_properties > + * rather than doing this. But for now... */ > + dbus_g_value_parse_g_variant (channel_properties, &value); > + g_assert (G_VALUE_HOLDS (&value, TP_HASH_TYPE_STRING_VARIANT_MAP)); They landed now
Comment on attachment 67794 [details] [review] McpDispatchOperation: use a new factory for each connection Review of attachment 67794 [details] [review]: ----------------------------------------------------------------- ::: mission-control-plugins/dispatch-operation.c @@ +425,5 @@ > > if (ret_ref_channel != NULL) > { > + /* FIXME: in next, this method should take a TpClientFactory > + * argument, and pass it on here */ I'm not sure to understand why plugins needs to subclass McpDispatchOperation, but I think in next, we should have a mandatory construct-only "factory" property and mcp_dispatch_operation_ref_connection() can then use self->priv->factory. The McpDispatchOperation objects are created from plugins so we cannot guarantee that we got a factory? Won't it be easier to already create a new factory in constructed(), and maybe somehow share it wit the rest of MC, and use that in mcp_dispatch_operation_ref_connection() so at least all TpConnections comes from the same factory?
Comment on attachment 67797 [details] [review] _mcd_channel_depart: use tp_channel_leave_async if it's a Group Review of attachment 67797 [details] [review]: ----------------------------------------------------------------- I think it is handler's responsability to do clean group leave. If MC has to close the channel, it can be an abrupt Close(), no? I don't want MC to start preparing GROUP feature if we didn't have to so far. GROUP won't be core in next and will prepare all TpContact objects. So I think there is something wrong to start creating potentially thousands of TpContact just to actually QUIT that channel... ::: src/mcd-channel.c @@ +1214,5 @@ > d->message = g_strdup (message); > > + /* tp_channel_leave_async documents calling it without first preparing > + * GROUP as deprecated. */ > + tp_proxy_prepare_async (channel->priv->tp_chan, just_group_feature, The reason why I've made that behaviour deprecated is because it is not when you leave the channel that you want to start preparing more stuff. GROUP won't be CORE in next and will involve prepare all members TpContact... Shouldn't MC just Close() in the case the client did not properly leave?
(In reply to comment #30) > Comment on attachment 67797 [details] [review] [review] > _mcd_channel_depart: use tp_channel_leave_async if it's a Group > > Review of attachment 67797 [details] [review] [review]: > ----------------------------------------------------------------- > > I think it is handler's responsability to do clean group leave. If MC has to > close the channel, it can be an abrupt Close(), no? I don't want MC to start > preparing GROUP feature if we didn't have to so far. GROUP won't be core in > next and will prepare all TpContact objects. So I think there is something > wrong to start creating potentially thousands of TpContact just to actually > QUIT that channel... > > ::: src/mcd-channel.c > @@ +1214,5 @@ > > d->message = g_strdup (message); > > > > + /* tp_channel_leave_async documents calling it without first preparing > > + * GROUP as deprecated. */ > > + tp_proxy_prepare_async (channel->priv->tp_chan, just_group_feature, > > The reason why I've made that behaviour deprecated is because it is not when > you leave the channel that you want to start preparing more stuff. GROUP > won't be CORE in next and will involve prepare all members TpContact... > > Shouldn't MC just Close() in the case the client did not properly leave? Argh, seems I drafted that reply previously and bugzilla included both... Bah you get the idea :)
(In reply to comment #29) > I'm not sure to understand why plugins needs to subclass > McpDispatchOperation They don't. It's a GInterface, and MC implements it. The point is to make it completely obvious what API plugins are allowed to use to call back into Mission Control: they are allowed to use the public symbols from libmission-control-plugins, and nothing else. > but I think in next, we should have a mandatory > construct-only "factory" property and > mcp_dispatch_operation_ref_connection() can then use self->priv->factory. I feel as though it ought to be using a factory supplied (or at least chosen) by the plugin, rather than MC's internal TpSimpleClientFactory: the features MC chooses to prepare shouldn't affect plugins, and vice versa. This means plugins use distinct TpConnection objects (etc.) for the same remote object (duplication in memory), but also avoids MC and plugins unintentionally affecting each other's behaviour. (Or perhaps plugins that don't express any particular opinion on the matter should share a TpSimpleClientFactory with each other, but not with MC?) I don't particularly want plugins to be able to stall MC dispatching by mistake by insisting that it prepares TP_CONNECTION_FEATURE_SOME_SLOW_THING before it gets on with its work - TP_CONNECTION_FEATURE_CONNECTED is particularly nasty, because MC is one of the few applications that will do useful (and asynchronous!) things before the connection goes CONNECTED.
(In reply to comment #30) > I think it is handler's responsability to do clean group leave. If MC has to > close the channel, it can be an abrupt Close(), no? In general I would agree with you, and indeed MC itself always uses Close() or Destroy() as appropriate. However, this function is used to implement mcp_dispatch_operation_leave_channels() for plugins: on the N900 and N9, it was pretty important for a plugin to be able to terminate a StreamedMedia call as BUSY or whatever. You could reasonably argue that now that we have Chan.T.Call, which doesn't (ab)use Group for call-ending, nobody should actually be using mcp_dispatch_operation_leave_channels() any more; perhaps we should deprecate it, and remove it in next. (This points to a reason why plugins shouldn't share MC's factory: if a plugin wants to terminate incoming Call channels, for instance to apply the N9's "one active call + one call on hold" policy, it will want to use a TpCallChannel.) At the moment, it doesn't work particularly reliably anyway (see Bug #55392). (In reply to comment #30) > The reason why I've made that behaviour deprecated is because it is not when > you leave the channel that you want to start preparing more stuff. GROUP > won't be CORE in next and will involve prepare all members TpContact... Isn't this more like an argument for why tp_channel_leave_async() shouldn't require or prepare TP_CHANNEL_FEATURE_GROUP? If we happen to have prepared GROUP anyway, great, we can use the self-handle to leave; if we haven't, why don't we just Get(SelfHandle) and then leave that way?
(In reply to comment #24) > This particular patch band goes like this: > > ⇥⇥⇥⇥⇥⇥⇥⇥command.common.account = ensure_prefix (command.common.account); > -⇥⇥⇥⇥⇥⇥⇥⇥a = tp_account_new (dbus, command.common.account, &error); > + a = tp_simple_client_factory_ensure_account (client_factory, > + command.common.account, NULL, &error); > > where ⇥⇥⇥⇥⇥⇥⇥⇥ represents a tab. Any objection? (In reply to comment #25) > Should be documented in tests/twisted/README. Sure. > run_and_bt.gdb could be moved to tests/twisted/tools That's where Jonny put it, but I don't particularly like that split - hiding generally-useful things in a fairly obscure subdirectory seems undesirable. > (or it could be added to tp-glib as well actually) Sure, I can do that.
(In reply to comment #34) > (In reply to comment #24) > > This particular patch band goes like this: > > > > ⇥⇥⇥⇥⇥⇥⇥⇥command.common.account = ensure_prefix (command.common.account); > > -⇥⇥⇥⇥⇥⇥⇥⇥a = tp_account_new (dbus, command.common.account, &error); > > + a = tp_simple_client_factory_ensure_account (client_factory, > > + command.common.account, NULL, &error); > > > > where ⇥⇥⇥⇥⇥⇥⇥⇥ represents a tab. > > Any objection? I surely agree that fixing that shouldn't be part of this commit, or even branch/bug. But I would still appreciate some coding style armonization in MC (but that's orthogonal to this bug). +1
Created attachment 68263 [details] [review] mc-tool: have a TpSimpleClientFactory to manufacture accounts
(In reply to comment #36) > Created attachment 68263 [details] [review] [review] > mc-tool: have a TpSimpleClientFactory to manufacture accounts +1
(In reply to comment #26) > ::: src/mcd-account.c > @@ +2392,5 @@ > > > > callback (account, error, user_data); > > g_clear_error (&error); > > + g_list_free_full (params, > > + (GDestroyNotify) tp_connection_manager_param_free); > > We do 4 spaces indent in this case nowadays. But I can accept if you say the > coding style is broken everywhere in that file anyway :) I ignored this; the prevailing coding style in MC is unlike ordinary Telepathy style anyway (it's more like Qt/Java style than GNU style) so, yeah. In recent MC branches I've changed my policy from "new files get ordinary Telepathy style" to "new *functions* usually get ordinary Telepathy style" in an attempt to reach Telepathy style eventually, but I don't think mass reindentation is going to make merging between master and next any easier.
Created attachment 68265 [details] [review] _mcd_client_match_property: take channel properties as GVariant --- Follow-up for Attachment #67790 [details], reducing the round-tripping considerably and fixing the new FIXME.
(In reply to comment #39) > Created attachment 68265 [details] [review] [review] > _mcd_client_match_property: take channel properties as GVariant > > --- > > Follow-up for Attachment #67790 [details], reducing the round-tripping > considerably and fixing the new FIXME. +1
Created attachment 68267 [details] [review] Document MISSIONCONTROL_TEST_BACKTRACE in tests/twisted/README --- (In reply to comment #2) > exec-with-log.sh: add gdb wrapper Guillaume already reviewed and merged this, in fact.
Created attachment 68268 [details] [review] [tp-glib] Add a hook to gather backtraces from failing tests Vaguely based on Mission Control patches from me and Jonny. Also document check-valgrind while I'm looking at tests/README. --- As you requested, a copy of this tool in telepathy-glib.
(In reply to comment #20) > _mcd_channel_depart: use tp_channel_leave_async if it's a Group This and Bug #55392 are now: http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=leave-via-newer-api-55391 Xavier reviewed that patch and objected, but I objected to his objection: (In reply to comment #33) > (In reply to comment #30) > > I think it is handler's responsability to do clean group leave. If MC has to > > close the channel, it can be an abrupt Close(), no? > > In general I would agree with you ... However ... Anyone have an opinion either way? > You could reasonably argue that now that we have Chan.T.Call, which doesn't > (ab)use Group for call-ending, nobody should actually be using > mcp_dispatch_operation_leave_channels() any more; perhaps we should > deprecate it, and remove it in next. Xavier suggests having MC never do RemoveMembers, but I think not changing the functionality of mcp_dispatch_operation_leave_channels is more important; perhaps this deprecation is a reasonable compromise? See also Bug #59162 in which wjt also wants to get rid of deprecated things.
(In reply to comment #41) > Created attachment 68267 [details] [review] [review] > Document MISSIONCONTROL_TEST_BACKTRACE in tests/twisted/README This is <http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=gdb>.
(In reply to comment #42) > [tp-glib] Add a hook to gather backtraces from failing tests ... > As you requested, a copy of this tool in telepathy-glib. This is <http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdb>.
(In reply to comment #43) > Xavier suggests having MC never do RemoveMembers, but I think not changing > the functionality of mcp_dispatch_operation_leave_channels is more > important; perhaps this deprecation is a reasonable compromise? Sorry, I realise that was a bit ambiguous. What I meant is: apply my patch (avoiding deprecated API while preserving existing functionality, even if that functionality is not really desirable), then deprecate the function in favour of close_channels() and destroy_channels() (which hopefully addresses Xavier's objection).
(In reply to comment #43) > > You could reasonably argue that now that we have Chan.T.Call, which doesn't > > (ab)use Group for call-ending, nobody should actually be using > > mcp_dispatch_operation_leave_channels() any more; perhaps we should > > deprecate it, and remove it in next. > > Xavier suggests having MC never do RemoveMembers, but I think not changing > the functionality of mcp_dispatch_operation_leave_channels is more > important; perhaps this deprecation is a reasonable compromise? If you deprecate mcp_dispatch_operation_leave_channels() then I'm OK with the patch.
Created attachment 74761 [details] [review] _mcd_channel_depart: if it turns out not to implement GROUP, close it This is a long-standing bug (broken ever since the function was introduced in 5.2), but apparently nobody noticed. Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=55392 --- Also on Bug #55392. Prerequisite for the one I'm about to attach.
Created attachment 74762 [details] [review] _mcd_channel_depart: use tp_channel_leave_async if it's a Group Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=55391 --- Rebased from Attachment #67797 [details], requires Attachment #74761 [details]. Xavier has r+'d this in principle, conditional on deprecating (the public function that calls) this function.
Created attachment 74763 [details] [review] Deprecate mcp_dispatch_operation_leave_channels It was designed for StreamedMedia, and is the wrong thing for Call.
*** Bug 59162 has been marked as a duplicate of this bug. ***
Created attachment 85499 [details] [review] [1/6] _mcd_channel_depart: if it turns out not to implement GROUP, close it This is a long-standing bug (broken ever since the function was introduced in 5.2), but apparently nobody noticed. --- Also resolves Bug #55392.
Created attachment 85500 [details] [review] [2/6] _mcd_channel_depart: use tp_channel_leave_async if it's a Group --- r+ from Xavier, conditional on deprecating leave_channels, which I did.
Created attachment 85501 [details] [review] [3/6] Deprecate mcp_dispatch_operation_leave_channels It was designed for StreamedMedia, and is the wrong thing for Call. --- Resolves Xavier's objection to Attachment #85500 [details], I think.
Created attachment 85502 [details] [review] [4/6] McdConnection: use tp_simple_client_factory_ensure_connection This means we need to pass the client factory through the McdManager from the McdMaster, so, do.
Created attachment 85503 [details] [review] [5/6] Service points: use tp_connection_dup_contact_by_id_async tp_connection_request_handles is deprecated. Ideally, we should drop this whole chunk of code, but it seems best to save that for Telepathy 1.0.
Created attachment 85504 [details] [review] [6/6] Disable things deprecated in or before telepathy-glib 0.20
Comment on attachment 85499 [details] [review] [1/6] _mcd_channel_depart: if it turns out not to implement GROUP, close it Review of attachment 85499 [details] [review]: ----------------------------------------------------------------- I think this commit is not needed because your next patch changes that function to use tp_channel_leave_async() which already fallback to _close()
The rest looks good to me.
(In reply to comment #58) > Comment on attachment 85499 [details] [review] > [1/6] _mcd_channel_depart: if it turns out not to implement GROUP, close it ... > I think this commit is not needed because your next patch changes that > function to use tp_channel_leave_async() which already fallback to _close() """ Note that unlike tp_channel_join_async(), TP_CHANNEL_FEATURE_GROUP feature does not have to be prepared and will be prepared for you. But this is a deprecated behaviour. """ I avoided what is documented to be a deprecated behaviour. If it is not, in fact, deprecated, then we can make that simplification (but for a Telepathy 1.0 branch I'm going to drop leave_async from MC anyway).
*** Bug 55392 has been marked as a duplicate of this bug. ***
Comment on attachment 85501 [details] [review] [3/6] Deprecate mcp_dispatch_operation_leave_channels applied
Comment on attachment 85502 [details] [review] [4/6] McdConnection: use tp_simple_client_factory_ensure_connection applied
Comment on attachment 85503 [details] [review] [5/6] Service points: use tp_connection_dup_contact_by_id_async applied
Comment on attachment 68267 [details] [review] Document MISSIONCONTROL_TEST_BACKTRACE in tests/twisted/README any chance of a review?
Comment on attachment 68268 [details] [review] [tp-glib] Add a hook to gather backtraces from failing tests Unfortunately, this isn't going to work as-is with Automake >= 1.13, for which test wrappers have to go in LOG_COMPILER instead of TESTS_ENVIRONMENT.
(In reply to comment #60) > """ > Note that unlike tp_channel_join_async(), TP_CHANNEL_FEATURE_GROUP feature > does not have to be prepared and will be prepared for you. But this is a > deprecated behaviour. > """ Is what we intended more like this? Calling tp_channel_leave_async() without first waiting for TP_CHANNEL_FEATURE_GROUP to be prepared (if possible) is deprecated. If the channel is known not to implement the Group interface, calling tp_channel_leave_async() is OK. That seems pretty unwieldy to me... but if that's a correct interpretation, we can just leave_async(), yes. I'm tempted to add a new tp_channel_group_leave_async() which requires TP_CHANNEL_FEATURE_GROUP to have been prepared successfully, and recommend it as specifically a way to leave groups (only). empathy-chat would use it for rooms.
Created attachment 85543 [details] [review] _mcd_channel_depart: use tp_channel_leave_async or tp_channel_close_async This avoids some deprecated APIs, and is considerably simpler. --- If my re-interpretation of that vague deprecation wording is correct, then this replaces both Attachment #85499 [details] and Attachment #85500 [details].
The reasoning here is that it's weird to start preparing GROUP feature when we actually want to quit it. In 1.0 preparing GROUP means creating all TpContact as well since we won't expose handles anymore. So I think in MC the right thing is the Close() the channel without caring about GROUP because - if I understand correctly - it is a fallback in the case there were no Handler or the Handler did not properly leave the channel itself. So it's not a normal code path that MC needs to leave the channel. So I think using leave_async() here is correct because it will do the proper GROUP trick if it's free, but will just Close() otherwise.
(In reply to comment #69) > So I think in MC the right thing is the Close() the channel without caring > about GROUP because - if I understand correctly - it is a fallback in the > case there were no Handler or the Handler did not properly leave the channel > itself. So it's not a normal code path that MC needs to leave the channel. No, that's not what the "leave" code path is for. Closing undispatchable channels uses Destroy(), falling back to Close(). The "leave" code path is for the benefit of plugins, specifically plugins that dealt with StreamedMedia calls. For instance, the N900 UI only supported one active call plus one call on hold (in any combination of protocols). If a third StreamedMedia call came in, a MC plugin would insta-terminate it as Busy. In modern Telepathy, calls should be Call1 channels, which have a HangUp() method instead of (ab)using the Group interface. Accordingly, in Telepathy 1.0, the "leave" code path is inappropriate and will just go away (see Bug #69176).
(In reply to comment #69) > So I think using leave_async() here is correct because it will do the proper > GROUP trick if it's free, but will just Close() otherwise. So is Attachment #85543 [details] OK?
yep, looks good to me :-)
Fixed in git for 5.15.1, 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.