Bug 25183 - HandleWith should have a User_Action_Time argument
Summary: HandleWith should have a User_Action_Time argument
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-spec (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Guillaume Desmottes
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ca...
Whiteboard: adds stable API, review+
Keywords: patch
Depends on:
Blocks: 28239
  Show dependency treegraph
 
Reported: 2009-11-19 05:31 UTC by Will Thompson
Modified: 2010-05-25 06:54 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Will Thompson 2009-11-19 05:31:49 UTC
HandleWith() doesn't have a User_Action_Time argument so MC can't pass it on to HandleChannels() properly. Currently MC uses the time it got the HandleWith() call, but that's broken for noninteractive approvers.

We could add a HandleWithTime() variant.
Comment 1 Simon McVittie 2010-03-31 06:46:35 UTC
This also breaks interactive approvers: on X, UserActionTime needs to be a timestamp obtained from the X server, which doesn't necessarily match wallclock time.
Comment 2 Guillaume Desmottes 2010-05-19 06:52:56 UTC
Simple branch adding this method:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-spec;a=shortlog;h=refs/heads/handle-time
Comment 3 Simon McVittie 2010-05-24 05:53:28 UTC
Looks good to me, but this insta-adds stable API, so I think we should have a MC patch that implements it ready to go, and have review from another of the spec developers. wjt, you reported this - does this method look good to you?
Comment 4 Will Thompson 2010-05-24 07:26:47 UTC
Looks fine to me.

HandleWithMisc(s: handler, a{sv}: misc) would be another option but I think it would be overkill. :)
Comment 5 Guillaume Desmottes 2010-05-25 04:53:52 UTC
Implemented in http://git.collabora.co.uk/?p=user/cassidy/telepathy-mission-control;a=shortlog;h=refs/heads/handle-time

This is blocked by a spec and tp-glib release.
Comment 6 Simon McVittie 2010-05-25 06:33:24 UTC
Please merge this to the spec, and clone to MC for the implementation.

Quick review on the implementation:

-                       tp_svc_channel_dispatch_operation_return_from_handle_with (
-                            approval->context);
+
+                        if (!approval->handle_with_time)
+                         tp_svc_channel_dispatch_operation_return_from_handle_with (
+                              approval->context);
+                        else
+                         tp_svc_channel_dispatch_operation_return_from_handle_with_time (
+                              approval->context);

You don't actually need this complexity (or the boolean), because HandleWith and HandleWithTime both return the same thing: you can just put a comment "HandleWith and HandleWithTime both return void, so it's OK to not distinguish" above the call to tp_svc_channel_dispatch_operation_return_from_handle_with.

Similarly, with a similar comment it would be OK (and, IMO, clearer) to implement dispatch_operation_handle_with as a simple call to dispatch_operation_handle_with_time.
Comment 7 Guillaume Desmottes 2010-05-25 06:44:38 UTC
spec merged. I opened bug  #28239 for the implementation.


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.