After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 599158 - Contact list and status icon should be an Approver
Contact list and status icon should be an Approver
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
2.28.x
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
empathy-maint
Depends on: 606429 610956 618993
Blocks: 578558 585914
 
 
Reported: 2009-10-21 10:24 UTC by Guillaume Desmottes
Modified: 2011-08-29 10:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-599158 (35.79 KB, patch)
2010-03-30 10:13 UTC, Guillaume Desmottes
rejected Details | Review
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-599158 (35.79 KB, patch)
2010-04-01 13:30 UTC, Guillaume Desmottes
rejected Details | Review
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/approvers-redone-599158 (30.84 KB, patch)
2010-06-16 13:21 UTC, Guillaume Desmottes
reviewed Details | Review

Description Guillaume Desmottes 2009-10-21 10:24:58 UTC
The Contact list and the status icon should both implement http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.Approver.html
Comment 1 André Klapper 2009-10-28 17:16:43 UTC
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.
Comment 2 Danielle Madeley 2009-10-29 05:38:06 UTC
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).
Comment 3 Guillaume Desmottes 2009-12-08 11:55:32 UTC
Ideally we should use TpApprover from tp-glib : https://bugs.freedesktop.org/show_bug.cgi?id=25513
Comment 4 Guillaume Desmottes 2010-01-08 16:41:43 UTC
We should turn the logger to an Observer before implementing this: bug #606429
Comment 5 Guillaume Desmottes 2010-03-02 17:40:27 UTC
This can't be implemented until we depend on tp-logger (bug #610956).
Comment 6 Guillaume Desmottes 2010-03-30 10:13:01 UTC
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(-)
Comment 7 Danielle Madeley 2010-04-01 10:14:11 UTC
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.
Comment 8 Guillaume Desmottes 2010-04-01 13:29:04 UTC
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!
Comment 9 Guillaume Desmottes 2010-04-01 13:30:59 UTC
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(-)
Comment 10 Guillaume Desmottes 2010-05-19 08:01:26 UTC
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.
Comment 11 Guillaume Desmottes 2010-06-16 13:21:17 UTC
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(-)
Comment 12 Guillaume Desmottes 2010-06-16 13:22:18 UTC
This is still blocked by bug #610956 but this branch can already be reviewed.
Comment 13 Danielle Madeley 2010-06-16 14:07:31 UTC
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() ?
Comment 14 Guillaume Desmottes 2010-06-17 13:59:44 UTC
(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.
Comment 15 Danielle Madeley 2010-06-18 03:13:23 UTC
(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.
Comment 16 Guillaume Desmottes 2010-06-18 09:24:21 UTC
I renamed and added a callback.
Comment 17 Guillaume Desmottes 2010-06-18 16:12:55 UTC
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.