Don't forget to check for capabilities before adding actions to notifications

Perhaps we should present the chat window when the user clicks on the bubble?  This is done by setting a "default" action.

Created attachment 144587 [details] [review]
proposed fix: http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/notify-button-589408

 src/empathy-status-icon.c |   39 +++++++++++++++++++++++++++++++++++++++
 src/empathy.c             |    4 +++-
 2 files changed, 42 insertions(+), 1 deletions(-)

Comment on attachment 144587 [details] [review]
proposed fix: http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/notify-button-589408

Looks fine to me, except for these things:

>+			if (priv->notify_supports_actions)
>+				{

Indentation is wrong here, empathy-status-icon.c uses another stile for braces.

>@@ -538,6 +564,7 @@ empathy_status_icon_init (EmpathyStatusIcon *icon)

>+  GList *list, *l;

Indentation.

>+	for (l = list; l != NULL; l = g_list_next (l))
>+		{

See previous comment for wrt braces indentation.

>+			if (!tp_strdiff (caps, "actions"))
>+				priv->notify_supports_actions = TRUE;

You should break the cycle when this is found.

Created attachment 144594 [details] [review]
tabs fixed

 src/empathy-status-icon.c |   37 +++++++++++++++++++++++++++++++++++++
 src/empathy.c             |    3 ++-
 2 files changed, 39 insertions(+), 1 deletions(-)

(In reply to comment #4)
> You should break the cycle when this is found.

No because all the caps strings have to be freed.

Comment on attachment 144594 [details] [review]
tabs fixed

Looks good.

Is there a reason why we can't just use the default action?

William: What do you mean by default action? Didn't see anything about that in the libnotify doc.

http://www.galago-project.org/specs/notification/0.9/x81.html (Table 1):
"The default action (usually invoked my clicking the notification) should have a key named "default". The name can be anything, though implementations are free not to display it."

(In reply to comment #7)
> (From update of attachment 144594 [details] [review])
> Looks good.

Actually this patch is wrong. As Xavier mentioned it, this "respond" button only make sense for new incoming chat events; not for tubes, invites, subscriptions requests etc.

It could make sense for the audio/video events as well but with current code it just display the incoming call dialog re-asking if we want to accept the call or not.

A simple hackish solution could be to add a boolean attribute to EmpathyEvent saying if we want to display this button or not.
It would be better to have a more generic mechanism but that probably involves some non trivial refactoring.

I think best way is to mimick GtkDialog's api, to add buttons. In EmpathyEventManager:

 - event_manager_add can be modified to get a list of button text and respond id.
 - EventFunc can have an extra param which is respond id.

All that is private functions, so it does not need to change any API.

(In reply to comment #10)
> http://www.galago-project.org/specs/notification/0.9/x81.html (Table 1):
> "The default action (usually invoked my clicking the notification) should have
> a key named "default". The name can be anything, though implementations are
> free not to display it."

Thanks, I was looking in the libnotify API. We are not convinced that's the right way to do it. Most people click on the notification bubble to make them dissappear so accepting the chat sounds like a bad idea.

After discussions, we agreed to add this feature for now (as Moblin really needs it) and do it properly during the 2.29 cycle (bug #597124).

Merged to master

commit ce8d523beb33cd57df62715ae1cf1fad6a601366
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Fri Oct 2 14:12:34 2009 +0100

    fix coding styles

commit 817bde504ebb05cfefb136ba77450eab0c374dfa
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Fri Oct 2 13:02:25 2009 +0100

    empathy-status-icon: check if notification daemon supports actions

commit d734b80c282ebb9120c630e1d6e7a8bad289daff
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Fri Oct 2 12:50:48 2009 +0100

    initialize libnotify earlier in main()
    
    The status icon will have to query the notify daemon during its
    construction so libnotify has to be already initialized.

commit ea995fbc6cc94712a4d10d931f48121c939db437
Author: Rob Bradford <rob@linux.intel.com>
Date:   Fri Oct 2 12:27:23 2009 +0100

    Add a "Respond" button to notifications to provide a positive action (#589408)
    
    The positive action is the same positive action that would be
    accomplished by clicking on the status icon.

I don't understand how it can be good for moblin if it is obviously the bad thing to do...

Moblin doesn't have any status icon so notifications are the only way to go.