GNOME Bugzilla – Bug 599158
Contact list and status icon should be an Approver
Last modified: 2011-08-29 10:12:37 UTC
The Contact list and the status icon should both implement http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.Approver.html
Currently 17 Empathy tickets are set as GNOME 2.30 blockers, hence mass-removing. Guillaume: Please use normal Target Milestones instead. If you really think that this specific issue here is a 2.30 blocker then please restore the GNOME target and set corresponding importance values.
I was wondering if the approver would be better suited to a daemon like gnome-settings-daemon, and hooked up to GConf keys defining preferred handlers (which could possibly be exposed in a settings capplet).
Ideally we should use TpApprover from tp-glib : https://bugs.freedesktop.org/show_bug.cgi?id=25513
We should turn the logger to an Observer before implementing this: bug #606429
This can't be implemented until we depend on tp-logger (bug #610956).
Created attachment 157457 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-599158 libempathy/Makefile.am | 2 + libempathy/empathy-approver.c | 356 ++++++++++++++++++++++++++++ libempathy/empathy-approver.h | 79 ++++++ libempathy/empathy-dispatch-operation.c | 10 +- libempathy/empathy-dispatch-operation.h | 3 + src/empathy-event-manager.c | 366 ++++++++++++++++++++++++----- tests/interactive/.gitignore | 1 + tests/interactive/Makefile.am | 4 +- tests/interactive/test-empathy-approver.c | 100 ++++++++ 9 files changed, 850 insertions(+), 71 deletions(-)
Review of attachment 157457 [details] [review]: I wonder whether having an EmpathyApprover separate from EmpathyHandler is the best way forward. This causes a lot of code duplication, and requires us to register another Client on the bus. It seems like EmpathyHandler (possible rename?) could easily implement the Approver interface, this allows for reuse of the channels filter (since we want the same channels, right?), D-Bus registration, etc. Here's an initial list of things: ::: src/empathy-event-manager.c @@ +183,3 @@ + + for (l = op->approvals; l != NULL; l = g_slist_next (l)) + event_manager_approval_free (l->data); What frees op->approvals? @@ +572,3 @@ + + chan = empathy_dispatch_operation_get_channel (approval->operation); + g_object_get (chan, "object-path", &path, NULL); tp_proxy_get_object_path()? @@ +580,3 @@ + remove_event_for_approval (self, approval); + event_manager_approval_free (approval); + g_free (path); Removes need for this g_free() @@ +583,3 @@ + break; + } + g_free (path); And this one. @@ +990,3 @@ + break; + default: + g_assert_not_reached (); Is an assertion correct here? Not easier just to throw an error and ignore it? @@ +1137,3 @@ g_slist_free (priv->events); + g_slist_foreach (priv->ongoing_operations, + (GFunc) event_manager_cd_operation_free, NULL); Nothing calls approval_done(), does that mean the phone keeps ringing forever? @@ +1209,3 @@ + + /* Create the Approver */ + filters = g_ptr_array_new (); g_ptr_array_new_with_free_func() is awesome here. @@ +1223,3 @@ + manager); + + g_ptr_array_foreach (filters, (GFunc) g_hash_table_destroy, NULL); Saves on this foreach.
Review of attachment 157457 [details] [review]: This branch is just a first step. Plan is to split the contact-list and status icon to their own approver. ::: src/empathy-event-manager.c @@ +183,3 @@ + + for (l = op->approvals; l != NULL; l = g_slist_next (l)) + event_manager_approval_free (l->data); It was leaked; good catch! @@ +572,3 @@ + + chan = empathy_dispatch_operation_get_channel (approval->operation); + g_object_get (chan, "object-path", &path, NULL); Good point; done. @@ +990,3 @@ + break; + default: + g_assert_not_reached (); If we get another status then something is wrong in the EmpathyDispatchOperation logic. That's a code error, not an input one. @@ +1137,3 @@ g_slist_free (priv->events); + g_slist_foreach (priv->ongoing_operations, + (GFunc) event_manager_cd_operation_free, NULL); empathy_sound_stop is called earlier in this function. @@ +1209,3 @@ + + /* Create the Approver */ + filters = g_ptr_array_new (); Didn't know that; thanks!
Created attachment 157693 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-599158 libempathy/Makefile.am | 2 + libempathy/empathy-approver.c | 356 ++++++++++++++++++++++++++++ libempathy/empathy-approver.h | 79 +++++++ libempathy/empathy-dispatch-operation.c | 10 +- libempathy/empathy-dispatch-operation.h | 3 + src/empathy-event-manager.c | 365 ++++++++++++++++++++++++----- tests/interactive/.gitignore | 1 + tests/interactive/Makefile.am | 4 +- tests/interactive/test-empathy-approver.c | 100 ++++++++ 9 files changed, 849 insertions(+), 71 deletions(-)
I'm redoing this branch using TpSimpleApprover and by dropping code from empathy's internal dispatcher rather than adding it. I'm not sure about how the approver should handle multi channels in the same channel dispatch operation. Empathy used to (internally) approve channels one by one but that's no longer possible as approving a CDO approve all the channels. So, how should Empathy's Approver handle CDO containing more than one channel? a) Treat each channel separately and approve all of them when one has been approved by the user. b) Don't approve CDO containing more than one channel (by returning a D-Bus error in AddDispatchOperation()). Empathy isn't really designed to approve/handle more than one channel so I don't know what's the best thing to do here. AFAIK, currently the only case where we have more than one channel is with tubes which are not approved/handled by Empathy any way.
Created attachment 163819 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-redone-599158 libempathy/empathy-dispatch-operation.c | 50 +---- libempathy/empathy-dispatch-operation.h | 4 - libempathy/empathy-dispatcher.c | 53 +---- libempathy/empathy-tp-chat.c | 5 +- libempathy/empathy-tp-chat.h | 3 +- src/empathy-event-manager.c | 385 ++++++++++++++++++++++--------- src/empathy.c | 2 +- 7 files changed, 286 insertions(+), 216 deletions(-)
This is still blocked by bug #610956 but this branch can already be reviewed.
Review of attachment 163819 [details] [review]: This looks mostly fine. The Claim->callback->reject bits are a bit confusing (I originally thought we were claiming to handle, not claiming to reject). It would be nice to wrap all rejection in a reject_channel() that calls claim_async() with a common callback, that then does channel appropriate rejection in a switch block. ::: src/empathy-event-manager.c @@ +766,3 @@ + continue; + + channel_type = tp_channel_get_channel_type_id (channel); Is a switch block better here? Allows all of the "main" cases to be easily collated. @@ +798,3 @@ + { + GError error = { TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, + "Uknown channel type" }; "Unknown" @@ +814,1 @@ + channel_type = tp_channel_get_channel_type (channel); Could use the quarks for direct comparison here, like in find_main_channel() ?
(In reply to comment #13) > Review of attachment 163819 [details] [review]: > > This looks mostly fine. > > The Claim->callback->reject bits are a bit confusing (I originally thought we > were claiming to handle, not claiming to reject). It would be nice to wrap all > rejection in a reject_channel() that calls claim_async() with a common > callback, that then does channel appropriate rejection in a switch block. done. > ::: src/empathy-event-manager.c > @@ +766,3 @@ > + continue; > + > + channel_type = tp_channel_get_channel_type_id (channel); > > Is a switch block better here? Allows all of the "main" cases to be easily > collated. Seems gcc isn't happy with quarks in case: empathy-event-manager.c:772: error: case label does not reduce to an integer constant > @@ +798,3 @@ > + { > + GError error = { TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, > + "Uknown channel type" }; > > "Unknown" fixed. > @@ +814,1 @@ > + channel_type = tp_channel_get_channel_type (channel); > > Could use the quarks for direct comparison here, like in find_main_channel() ? done.
(In reply to comment #14) Looks good. One small issue, but doesn't need more review IMO. > > The Claim->callback->reject bits are a bit confusing (I originally thought we > > were claiming to handle, not claiming to reject). It would be nice to wrap all > > rejection in a reject_channel() that calls claim_async() with a common > > callback, that then does channel appropriate rejection in a switch block. > > done. I would still give this callback a clearer name. Like reject_channel_claim_cb. So that you know it's for rejection. Alternatively (or in addition) a comment saying what you're doing so that people don't assume you're claiming in order to handle. > > ::: src/empathy-event-manager.c > > @@ +766,3 @@ > > + continue; > > + > > + channel_type = tp_channel_get_channel_type_id (channel); > > > > Is a switch block better here? Allows all of the "main" cases to be easily > > collated. > > Seems gcc isn't happy with quarks in case: > empathy-event-manager.c:772: error: case label does not reduce to an integer > constant Oh, because they're secretly functions. This bugs me.. I've wondered if making them all const gulongs (that get assigned in some GOnce func) or whatever would break ABI, because it would make the bindings nicer too. Another bug.
I renamed and added a callback.
Merged to master. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.