| Summary: | ChannelRequest.I.Hints — let requesters pass metadata through to handlers | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Will Thompson <will> |
| Component: | tp-spec | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
| Severity: | normal | ||
| Priority: | medium | CC: | guillaume.desmottes, vivek |
| Version: | unspecified | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| URL: | http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/undraft | ||
| Whiteboard: | undraft? | ||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 29457, 30000 | ||
See also Bug #25018, and discussion on the mailing list in thread "On spec additions to allow a really simple IM API". These additions make sense to me. I proposed on bug #25018 a way to get the channel back from MC when requesting. I implemented a proof of concept of this in http://git.Collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/request-channel Some tests regressed but the basic feature seems to work. I implemented this spec branch: http://git.Collabora.co.uk/?p=user/cassidy/telepathy-spec;a=shortlog;h=refs/heads/request-channel-25018 which a rebased version of Will's branch to which I added the SucceededWithChannel signal (bug #25018). (In reply to comment #0) > Handlers can already sign up to receive the immutable properties of channel > requests by implementing Client.Interface.Requests. I think we should still implement the "request-properties" in Handler_Info as well. It will keep the API more symetric and would make it easier to use as you get everything in HandleChannels(). > + <tp:member type="a{sv}" name="Immutable_Properties" tp:type="String_Variant_Map"> Strictly speaking this is a Qualified_Property_Value_Map. I'm still unsure about the RequestMetadata naming; there was a vague idea in my head that it could be called "Hints"? Or perhaps "RequestHints"? > Some tests regressed but the basic feature seems to work I don't like the sound of that. What failed? Can you fix them? :-P (Hopefully the failure mode is just that tests were expecting an empty dict of metadata, or whatever, and need to be adjusted to expect more information...) Regarding the proof-of-concept implementation: > +_mcd_channel_dup_properties (McdChannel *self) I have an old branch to make channel requests into independent objects, which Vivek said he was going to review. It'd probably be worth merging that first. Alternatively, this function should be called _mcd_channel_dup_request_properties, because it's the channel-request part of the McdChannel's split personality. > mcd-channel: store the channel request object in satisf... I haven't reviewed this, because I think merging and using smcv/requests is probably a better way forward. > + request_properties = g_hash_table_new_full (g_str_hash, g_str_equal, > + g_free, free_req_properties); Couldn't the free-function just be ((GDestroyNotify) g_hash_table_unref)? (In reply to comment #5) > > + <tp:member type="a{sv}" name="Immutable_Properties" tp:type="String_Variant_Map"> > > Strictly speaking this is a Qualified_Property_Value_Map. Fixed. I opened bug #29900 regarding this. > I'm still unsure about the RequestMetadata naming; there was a vague idea in my > head that it could be called "Hints"? Or perhaps "RequestHints"? Hints sounds good to me, if only because it's shorter :) > > Some tests regressed but the basic feature seems to work > > I don't like the sound of that. What failed? Can you fix them? :-P Yeah, I was planning to ask you info about those. I'll fix them once your refactoring branch has been merged. I created a rebased version of the branch based on you refactoring branch: http://git.Collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/request-channel-rebased I fixed your comments in separated commits. One of the test failing is dispatcher/dispatch-delayed-by-mini-plugin.py which hits this assertion: mcd:ERROR:mcd-account-requests.c:164:on_request_completed: assertion failed: (tp_chan != NULL) Any idea why a request could be completed without channel? (In reply to comment #8) > One of the test failing is dispatcher/dispatch-delayed-by-mini-plugin.py which > hits this assertion: > > mcd:ERROR:mcd-account-requests.c:164:on_request_completed: assertion failed: > (tp_chan != NULL) > > Any idea why a request could be completed without channel? I fixed this one (not sure if that's the best way, this code looks like a bit like a can of worms to me tbh :\ ). (In reply to comment #6) > > mcd-channel: store the channel request object in satisf... > > I haven't reviewed this, because I think merging and using smcv/requests is > probably a better way forward. How does this branch help? This commit hasn't changed after the rebase. (In reply to comment #9) > I fixed this one (not sure if that's the best way, this code looks like a bit > like a can of worms to me tbh :\ ) This code *is* a can of worms :-( See smcv/wip-req-export, in which I tried to split McdChannel into the bits that are really a channel and the bits that are a channel request, and have the latter be exported as the ChannelRequest; in particular, the top commit, which is called "WiP: doesn't work" and is something like my 5th attempt at it. Your code changes look good, except for the last commit where I need to read the code a bit more to work out wtf is going on. I suspect that what you did is necessary and correct, even though it makes little sense. Would you mind if I pick up this branch and make it use an in-tree extension instead of the patched telepathy-glib you're presumably using, so we can merge it before undrafting the spec, and continue to refactor? > (In reply to comment #6) > > > mcd-channel: store the channel request object in satisf... > > > > I haven't reviewed this, because I think merging and using smcv/requests is > > probably a better way forward. > > How does this branch help? This commit hasn't changed after the rebase. It turns out smcv/requests isn't actually sufficient; for the simplification, you'd need some of the commits from smcv/wip-req-export (moving all the ChannelRequest properties to McdRequest and adding _mcd_request_dup_immutable_properties()). With those added, my idea was to have a GList of McdRequest, rather than a GList of McdChannel. I'm not sure whether that will actually be enough... I hope so. (In reply to comment #10) > Would you mind if I pick up this branch and make it use an in-tree extension > instead of the patched telepathy-glib you're presumably using, so we can merge > it before undrafting the spec, and continue to refactor? Sure, go for it. > > (In reply to comment #6) > > > > mcd-channel: store the channel request object in satisf... > > > > > > I haven't reviewed this, because I think merging and using smcv/requests is > > > probably a better way forward. > > > > How does this branch help? This commit hasn't changed after the rebase. > > It turns out smcv/requests isn't actually sufficient; for the simplification, > you'd need some of the commits from smcv/wip-req-export (moving all the > ChannelRequest properties to McdRequest and adding > _mcd_request_dup_immutable_properties()). With those added, my idea was to have > a GList of McdRequest, rather than a GList of McdChannel. I'm not sure whether > that will actually be enough... I hope so. Humm sounds like it would be best to wait you have finished your refactoring before continuing this branch then. *** Bug 25018 has been marked as a duplicate of this bug. *** Hijacking this bug to work on it a bit. ---------------------- From Bug #25018: Guillaume Desmottes 2010-08-31 02:12:13 PDT Should SucceededWithChannel include the properties of the Connection and the Channel? I've stolen this branch: http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/request-channel-25018 http://people.freedesktop.org/~smcv/telepathy-spec-request_channel_25018/spec/ The *implementation* of this branch turns out to be quite a can of worms (not Guillaume's fault, it touches MC code that contains lurking horrors) so I think we need to get it merged relatively soon (before the spec is undrafted) and refactor until it actually makes sense. Accordingly, I've moved the new API to .FUTURE pseudo-interfaces, so we can do the refactoring without needing to compile MC against a patched telepathy-glib. I opened Bug #30000 (eek, 30k Freedesktop bugs) for the MC implementation. I've also made some other misc changes, for which see my git repository. ------------------------------------ Insta-stable changes ==================== Remaining changes to stable API in my version of the branch: * Client.I.Requests: Hints SHOULD be included among the ChannelRequest's immutable properties. Uncontroversial, because every immutable property SHOULD be included anyway :-) * request-properties added to ObserveChannels' dict of misc. Uncontroversial, I think. * Likewise for HandleChannels' dict of misc. I don't think any of those should block merging this. ------------------------------------ To-do list ========== Should SucceededWithChannel have the immutable properties of the channel? I think it probably should, but I haven't added this yet. Should SucceededWithChannel be plural? In principle, a ChannelRequest could operate on a hypothetical plural request, and return many channels... although plural requests are a tower of cans, each containing worms. Should SucceededWithChannel give you properties of the Connection? Probably not, IMO: connections don't have many (useful) properties that are actually immutable! Should we explicitly say that the CD will never interpret Hints? I think we should, and I've added a commit that says so. While we're adding new ways to request channels, should we add an a{sv} placeholder for extra arguments that the channel dispatcher *does* interpret? (Obviously, this is unnecessary if you convince me that my previous assertion was wrong.) (In reply to comment #14) > I've stolen this branch: > > http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/request-channel-25018 > http://people.freedesktop.org/~smcv/telepathy-spec-request_channel_25018/spec/ > > The *implementation* of this branch turns out to be quite a can of worms (not > Guillaume's fault, it touches MC code that contains lurking horrors) so I think > we need to get it merged relatively soon (before the spec is undrafted) and > refactor until it actually makes sense. ++ for merging earlier as a DRAFT/FUTURE. > Accordingly, I've moved the new API to .FUTURE pseudo-interfaces, so we can do > the refactoring without needing to compile MC against a patched telepathy-glib. > I opened Bug #30000 (eek, 30k Freedesktop bugs) for the MC implementation. > > I've also made some other misc changes, for which see my git repository. Your patches look good to me. > To-do list > ========== > > Should SucceededWithChannel have the immutable properties of the channel? I > think it probably should, but I haven't added this yet. I think so, we should encourage clients to always use tp_channel_new_from_properties() > Should SucceededWithChannel be plural? In principle, a ChannelRequest could > operate on a hypothetical plural request, and return many channels... although > plural requests are a tower of cans, each containing worms. Right, we're not going to solve the plural request problem now but making it plural would ensure that this new API is future ready. > Should SucceededWithChannel give you properties of the Connection? Probably > not, IMO: connections don't have many (useful) properties that are actually > immutable! I agree that's not really useful atm but, oth, the "path + immutable properties" pattern is an useful one so it would probably be good to generalise it. Maybe that could actually be a Telepathy 1.0 goal? I wouldn't be surprised that at some point Connection objects gain immutable properties at some point. > Should we explicitly say that the CD will never interpret Hints? I think we > should, and I've added a commit that says so. > > While we're adding new ways to request channels, should we add an a{sv} > placeholder for extra arguments that the channel dispatcher *does* interpret? > (Obviously, this is unnecessary if you convince me that my previous assertion > was wrong.) I don't have a strong opinion on this. Draft merged in git for 0.19.12, thanks. I'll adapt the MC implementation to use the .FUTURE pseudo-interfaces. Will, any thoughts on the to-do list in Comment #14? (In reply to comment #14) > To-do list > ========== > > Should SucceededWithChannel have the immutable properties of the channel? I > think it probably should, but I haven't added this yet. Yes. > Should SucceededWithChannel be plural? In principle, a ChannelRequest could > operate on a hypothetical plural request, and return many channels... although > plural requests are a tower of cans, each containing worms. I think it's way simpler to keep it singular. > Should SucceededWithChannel give you properties of the Connection? Probably > not, IMO: connections don't have many (useful) properties that are actually > immutable! Also probably not. > Should we explicitly say that the CD will never interpret Hints? I think we > should, and I've added a commit that says so. > > While we're adding new ways to request channels, should we add an a{sv} > placeholder for extra arguments that the channel dispatcher *does* interpret? > (Obviously, this is unnecessary if you convince me that my previous assertion > was wrong.) I think we can just say that Hints isn't interpreted by the CD for now; later, if we want, we can add properly-namespaced hints that it does interpret. I agree with all wjt said. Anything blocking this to move forward? I'd like to fix /query and /msg in Empathy. I implemented the "request and observe" tp-glib API (bug #29457); 2 observeations: We really need the channel properties to be able to use tp_channel_new_from_properties(). When ensuring a channel, if it already exists it would probably be useful to give the existing channel back to the user. Do we have API for that? (In reply to comment #17) > (In reply to comment #14) > > Should we explicitly say that the CD will never interpret Hints? I think we > > should, and I've added a commit that says so. > > > > While we're adding new ways to request channels, should we add an a{sv} > > placeholder for extra arguments that the channel dispatcher *does* interpret? > > (Obviously, this is unnecessary if you convince me that my previous assertion > > was wrong.) > > I think we can just say that Hints isn't interpreted by the CD for now; later, > if we want, we can add properly-namespaced hints that it does interpret. That means reverting my commit claiming that the CD will never interpret Hints, and instead leaving it undefined; but if you feel that that's better, OK. I'll try to sort out an updated branch for this on the next train. http://people.freedesktop.org/~smcv/telepathy-spec-hints/spec/ http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/hints One point of disagreement with wjt in Comment #17: I added an a{sv} for Connection properties too, on the basis that connection-observing will require some sort of "immutable" properties dict. If we never use it, so be it. Do we also want an a{sv} for future expansion? Answers on a postcard. If we do, we could drop the hypothetical connection properties on the basis that they can go in there when needed. (In reply to comment #22) > One point of disagreement with wjt in Comment #17: I added an a{sv} for > Connection properties too, on the basis that connection-observing will require > some sort of "immutable" properties dict. If we never use it, so be it. Fair enough. I think path + properties is a nice patent when announcing objects. > Do we also want an a{sv} for future expansion? Answers on a postcard. I can't think of anything that wouldn't work using Hints. Oth, the whole point of future expansion is to be future proof for cases we didn't think about. :) But I still think we shouldn't need it. > If we do, > we could drop the hypothetical connection properties on the basis that they can > go in there when needed. I wouldn't do that, as said explicit path + props are cool. branch++ Thanks, merged. Time to undraft this, I think, unless anyone has objections. Will? Guillaume? This branch also undrafts ServerAuthentication, SASLAuthentication and a very simple Securable interface (Bug #14003). http://people.freedesktop.org/~smcv/telepathy-spec-undraft/spec/ http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/undraft What about : (In reply to comment #19) > I implemented the "request and observe" tp-glib API (bug #29457); 2 > observeations: > > [...] > > When ensuring a channel, if it already exists it would probably be useful to > give the existing channel back to the user. Do we have API for that? Fixed in 0.21.5 |
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.
Currently, there's no way to relay additional information between a channel requester and the ultimate observers and handlers of the resulting channel. The main use case for which this poses a problem is associating contact IDs (in particular, phone numbers) with particular contacts in your address book. When I tell my N900 to call my mum, how does the call UI know that I'm trying to call her and not my dad (who has the same phone number¹)? The current answer is that it can't. So I have written a spec branch which adds: • A new immutable property, ChannelRequest.RequestMetadata: a{sv}, defined to contain arbitrary metadata, which clients MAY choose to interpret but MUST cope with the absence of [a]; • New methods on ChannelDispatcher to specify said metadata when making a channel request [b]; • A new key in ObserveChannels' Observer_Info dict containing immutable channel properties [c]. Handlers can already sign up to receive the immutable properties of channel requests by implementing Client.Interface.Requests. So this allows the call UI to know who you're calling, as well as any observers (perhaps a call logger?), assuming the various components know the format by which the platform-specific contact ID is included in the RequestMetadata. They have to be able to deal with it being missing — a third-party app might not specify this — but it allows this information to be passed if needed. This could also be used to allow one process to request an outgoing file transfer channel to be handled by another process, passing along the full path to the file to be sent, which would allow nautilus-send-to's Telepathy plugin to delegate the actual file transferring to Empathy, or vice-versa. Obviously this involves slightly more active collusion — you have to know that the handler will understand this key — but that's probably okay, we can specify that and maybe have some mechanism to discover it. [a] http://people.freedesktop.org/~wjt/telepathy-spec-channel_request_metadata/spec/Channel_Request.html#org.freedesktop.Telepathy.ChannelRequest.RequestMetadata [b] http://people.freedesktop.org/~wjt/telepathy-spec-channel_request_metadata/spec/Channel_Dispatcher.html [c] http://people.freedesktop.org/~wjt/telepathy-spec-channel_request_metadata/spec/Client_Observer.html#org.freedesktop.Telepathy.Client.Observer.ObserveChannels 1. actually in my case they don't, but the point stands. ☺