_mcd_connection_set_tp_connection() uses tp_connection_new() whereas it should allegedly use tp_simple_client_factory_ensure_connection(). mcd-connection-service-points.c uses tp_connection_request_handles() but it needs to use tp_connection_dup_contact_by_id_async() which is singular so we need to call it n times and then coalesce the results and blah blah blah I lost the will to live. Relatedly, this code has no tests, and by two minutes of inspection will g_critical() if http://telepathy.freedesktop.org/spec/Connection_Interface_Service_Point.html#Property:KnownServicePoints contains more than one Emergency service point: • parse_services_list() will call _mcd_connection_clear_emergency_data() (which frees and NULLS priv->emergency.handles); • then call tp_connection_request_handles() more than once; • so service_handles_fetched_cb() will be called more than once; • it calls _mcd_connection_take_emergency_handles() which criticals if priv->emergency.handles is non-NULL, then sets priv->emergency.handles to the new set of handles.
tp_connection_dup_contact_by_id_async() is meant to be used by user-entered ids (e.g. add contact dialog), so only one at a time. There is a 2nd usecase which is to get the contact after a contact search result, but again it should get the contact only for the user selected one, so again one at a time. That API can result in server round-trip (MSN-XMPP special case). In every other cases, the ID is coming from the server, so the CM should have given a (handle, id) pair. If some iface does not give the pair, fix the iface. Unless there are other use cases that I did not though about?
I have some half-finished branches for this stuff somewhere. I'll check whether they pass tests or anything...
Checking the MC code, it looks like it needs the TpHandle for those service IDs in the case a ChannelRequest arrives with only TP_PROP_CHANNEL_TARGET_HANDLE and not TP_PROP_CHANNEL_TARGET_ID. With our goal to kill handles, I would say that _mcd_request_proceed() should just assume the TP_PROP_CHANNEL_TARGET_ID is set in the request props. Which should really be the case anyway when dialing 911, since it's user-entered id. From an API POV, we have tp_account_channel_request_set_target_id/contact and both just set the target ID in properties. So the TARGET_HANDLE is never used when using the proper high-level API. Now it's just a question of how much we care about keeping MC backward compatible in this case.
See also Bug #55391.
(In reply to comment #3) > Checking the MC code, it looks like it needs the TpHandle for those service > IDs in the case a ChannelRequest arrives with only > TP_PROP_CHANNEL_TARGET_HANDLE and not TP_PROP_CHANNEL_TARGET_ID. > > With our goal to kill handles, I would say that _mcd_request_proceed() > should just assume the TP_PROP_CHANNEL_TARGET_ID is set in the request > props. I basically agree with this reasoning. > Now it's just a question of how much we care about keeping MC backward > compatible in this case. For Telepathy 1.0, I think we should certainly spec (in ServicePoints) that anything wanting special treatment on outgoing calls to a service point MAY assume that it is requested with either TargetID or InitialServicePoint, and make MC behave as such. For Telepathy 0.x, perhaps the same would be OK. Given its emergency-related nature, I would hope that anyone to whom ServicePoints actually matter (does Jolla use Telepathy?) would talk to us and/or do rather thorough QA of their own...
Created attachment 72712 [details] [review] Require GLib 2.32, for GHashTable set APIs (add, contains)
Created attachment 72713 [details] [review] McdConnection: build up our sets of emergency numbers over time To facilitate this, use nicer data structures. Most importantly, this means we don't critical whenever we get more than one distinct service point, as Jonny noticed while porting to Telepathy 1.0. It might slightly defeat the intention of the ServicePointsChanged signal, but in practice the list is probably never going to shrink, and if it does, it's probably wise to keep considering its old members to be an emergency number (the effect that that has is that plugins aren't allowed to influence channel requests). Also, don't leak the sets of emergency numbers when finalized.
Created attachment 72714 [details] [review] Add a regression test for service-points (matched by identifier) The side-effect they have within MC is that plugins aren't allowed to delay or reject channel requests; we ought to test that, really.
Those three are <http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=emergency-59162>. They only partially fix this bug.
"_mcd_channel_depart: use tp_channel_leave_async if it's a Group" on Bug #55391 also partially fixes this bug. It requires the patch from Bug #55392 first. I have pushed that as <http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=leave-via-newer-api-55391>.
Review of wjt's WiP: > -AC_DEFINE([TP_VERSION_MIN_REQUIRED], [TP_VERSION_0_18], > - [Ignore post-0.18 deprecations]) > +AC_DEFINE([TP_VERSION_MIN_REQUIRED], [TP_VERSION_0_20], > + [Ignore post-0.20 deprecations]) This is obviously fine as soon as it actually compiles :-) Also please define TP_SEAL_ENABLE and TP_DISABLE_SINGLE_INCLUDE, if/when they work (make distcheck is the safest way to test this stuff). If you like this sort of thing, I recently did similar updates on Salut, Bug #54119. > +service_handles_fetched_cb ( This is the work that is in progress, I assume? My patches here should hopefully fix this. > - self_handle = tp_connection_get_self_handle (proxy); > + self_handle = tp_contact_get_handle (tp_connection_get_self_contact (proxy)); As I mentioned on Bug #55391, I would rather make MC use higher-level APIs for presence (like I did for Aliasing and Avatars on Bug #55668). I'd be OK with doing this quick change for now to avoid the deprecations, and splitting out "use TP_CONTACT_FEATURE_PRESENCE" into a separate bug, if you'd prefer.
Comment on attachment 72712 [details] [review] Require GLib 2.32, for GHashTable set APIs (add, contains) (In reply to comment #6) > Require GLib 2.32, for GHashTable set APIs (add, contains) No longer needed: merging one of my branches also required this change.
Hijacking this bug for the emergency number stuff. wjt's work-in-progress is: http://cgit.collabora.com/git/user/wjt/telepathy-mission-control/log/?h=post-0.18-deprecations
*** This bug has been marked as a duplicate of bug 55391 ***
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.