Created attachment 151761 [details] [review]
Check for append capability

* Adds support for the "x-canonical-append" hint from notify-osd, which appends notifications when possible.  
* Only attach the notification to the status icon if the status icon is displayed.

Created attachment 151773 [details] [review]
Use built-in empathy function for checking caps

 * Adds support for the "x-canonical-append" hint from notify-osd, which appends notifications when possible (using empathy_notify_manager_has_capability). 
 * Only attach the notification to the status icon if the status icon is displayed.

Review of attachment 151773 [details] [review]:

Hi Ken. Thanks for your patch; I inlined some comments.

::: src/empathy-status-icon.c
@@ +160,3 @@
 
+		if ((priv->notification) && (!empathy_notify_manager_has_capability (priv->notify_mgr,
+							EMPATHY_NOTIFY_MANAGER_CAP_X_CANONICAL_APPEND))) {

Shouldn't we check the standard "append" caps as well?

@@ +203,3 @@
+								EMPATHY_NOTIFY_MANAGER_CAP_X_CANONICAL_APPEND)) {
+			notify_notification_set_hint_string (priv->notification,
+				EMPATHY_NOTIFY_MANAGER_CAP_X_CANONICAL_APPEND, "");

Shouldn't you set this hint only if there is an existing notification regarding the same contact?
Btw, how is that different from the standard EMPATHY_NOTIFY_MANAGER_CAP_APPEND caps?

*** Bug 589851 has been marked as a duplicate of this bug. ***

Might this feature go into final 2.30?

Guillaume, I'll try to answer your questions:

> Shouldn't we check the standard "append" caps as well?

Where is the standard "append" definition and documentation? Which notification daemon supports it? 
I think that "append" was renamed to "x-canonical-append" and only notify-osd supports it.

> Shouldn't you set this hint only if there is an existing notification regarding
> the same contact?

What's strange is that this file contains only the status notification code. when users talk to each other, another file is used "empathy-chat-window.c" which has its own notification. So the real problem is that the patch needs to be completed, but nevermind, it's quite simple. I'll attach the complete patch soon

> Btw, how is that different from the standard 
> EMPATHY_NOTIFY_MANAGER_CAP_APPEND caps?

In my opinion the original name was "append", but it needed to be renamed to "x-canonical-append" because it was not standard. Did someone introduce it to the standard?

here is a link to a bug that might give credit to my opinion about "append": https://bugs.launchpad.net/ubuntu/+source/notify-osd/+bug/341565

Created attachment 156889 [details] [review]
x-canonical-append.patch

A few comments:
-) remove the support from empahty-status-icon.c since I don't think it's useful for status changes, but only in chat notifications
-) unref the old notification before creating a new one
-) add a GMutex to access priv->notification, since previously you had only 1 notification opened, but now there is more than one, and lots of threads that can do a mess :D
-) call notify_notification_set_hint_string only once per notification

Please review ASAP to go to 2.30 release.
I have briefly tested it, and it works (with notify-osd)

Created attachment 156950 [details] [review]
new patch

This new patch tries to address some problems detected by ken, and brings the same logic to empathy-status-icon.c which seems to be needed when the chat window is closed.

Still not heavily tested

(In reply to comment #6)
> Guillaume, I'll try to answer your questions:
> 
> > Shouldn't we check the standard "append" caps as well?
> 
> Where is the standard "append" definition and documentation? Which notification
> daemon supports it? 
> I think that "append" was renamed to "x-canonical-append" and only notify-osd
> supports it.


"append" is implemented in notify-osd see:
https://bugzilla.gnome.org/show_bug.cgi?id=601691#c2

I didn't know it was the old name, thanks for the info!

(In reply to comment #8)
> Created an attachment (id=156889) [details] [review]
> x-canonical-append.patch
> 
> A few comments:
> -) remove the support from empahty-status-icon.c since I don't think it's
> useful for status changes, but only in chat notifications
> -) unref the old notification before creating a new one
> -) add a GMutex to access priv->notification, since previously you had only 1
> notification opened, but now there is more than one, and lots of threads that
> can do a mess :D

Where are threads invovled here? This code is not threaded.

> -) call notify_notification_set_hint_string only once per notification
> 
> Please review ASAP to go to 2.30 release.

We are no in hardcode freeze so that's too late for 2.30

> I have briefly tested it, and it works (with notify-osd)

Would be great if you could use "git format-patch" when attaching patches btw.

> Where are threads invovled here? This code is not threaded.

Well, the notification closed callback is executed in a separate thread. Isn't it?

Created attachment 156954 [details] [review]
git formatted patch

This patch should be git formatted (tell me if I did wrong things)

(In reply to comment #12)
> > Where are threads invovled here? This code is not threaded.
> 
> Well, the notification closed callback is executed in a separate thread. Isn't
> it?

No, libnotify is not threaded.

So there can't be concurrent access to chat_window_notification_closed_cb and any other function in this file? (and vice-versa of course)

Indeed.

I'm sorry, I need to learn lots of things!

how can I obtain a single patch from 2 git commits?
git format-patch -2
gives me 2 patches, but I need only one to attach here

Created attachment 156965 [details] [review]
patch

I removed all the mutex variables and calls

I used "git diff master" I hope it's ok.

Review of attachment 156965 [details] [review]:

::: src/empathy-status-icon.c
@@ +148,3 @@
 	EmpathyStatusIconPriv *priv = GET_PRIV (icon);
 	GdkPixbuf *pixbuf = NULL;
+	gboolean has_x_canonical_append;

This variable should be defined in the if block.

@@ +162,3 @@
 			message_esc = g_markup_escape_text (priv->event->message, -1);
 
+		has_x_canonical_append = empathy_notify_manager_has_capability (priv->notify_mgr,

This line is longer than 80 chars.

@@ +165,3 @@
+					EMPATHY_NOTIFY_MANAGER_CAP_X_CANONICAL_APPEND);
+
+		if (priv->notification != NULL && ! has_x_canonical_append) {

I don't like how this changes the semantic priv->notification. It used the be "the running notification, if any". Now it's "the last notification created". When creating a new notification, we just replace priv->notification and so loose the reference of the older notification which isn't great.

How this x-cannonical-append work exactly? Is it documented somewhere?

How do you prefer the line to be splitted? after the = or before the ( ?

I thought exactly the same thing: in this way you loose the reference to all the previous notifications. I don't know how they work exactly, because I couldn't find any documentation. Only this site: https://wiki.ubuntu.com/NotificationDevelopmentGuidelines but it doesn't say anything special. I also looked at the source example in "bzr branch lp:notify-osd", the only different thing there is that they unref the notification immediately after showing it, which is different from what we do, that is unref it in the close callback.

(In reply to comment #20)
> How do you prefer the line to be splitted? after the = or before the ( ?

I'd say after the '='.

> I thought exactly the same thing: in this way you loose the reference to all
> the previous notifications. I don't know how they work exactly, because I
> couldn't find any documentation. Only this site:
> https://wiki.ubuntu.com/NotificationDevelopmentGuidelines but it doesn't say
> anything special. I also looked at the source example in "bzr branch
> lp:notify-osd", the only different thing there is that they unref the
> notification immediately after showing it, which is different from what we do,
> that is unref it in the close callback.

Humm, maybe we don't need to keep a ref actually, maybe libnotify does it for us. Could you check that in libnotify's code please?

Created attachment 157051 [details] [review]
git diff

I fixed the variable and line length problem.

I still need to verify the notify-osd code (now I don't have time), but I don't think you need to keep all references, since the g_object_unref is done in the callbak, to which you always give the correct pointer.

notify-osd automatically unrefs the CURRENT bubble. So the best thing would be to keep track of the first created bubble.

if (new_bubble && bubble_is_append_allowed(bubble)) {
  app_bubble = find_bubble_for_append(self, bubble);

  /* Appending to an old bubble */
    if (app_bubble != NULL) {
      g_object_unref(bubble);
      bubble = app_bubble;

Created attachment 157055 [details] [review]
git diff

done

You missunderstand how things work here. We have two separated processes: Empathy (linked on libnotify) and notify-osd (or the notification-daemon).
These 2 processes don't share any memory, they just communicate together over D-Bus (Empathy uses libnotify which offer a more convenient API hidding the D-Bus bits). So an object created in Empathy does *not* exist in the address space of notify-osd.
What you have to check here is how libnotify manages NotifyNotification objects.

I did it for you and it seems that unreffing the object in the closed_cb is the good thing to do.

Do we really have to create a new NotifyNotification object when using the append hint? Can't we just reuse the existing one?

> I did it for you and it seems that unreffing the object in the closed_cb is the
> good thing to do.

Thanks for having done this, I didn't understand, sorry.
After having analyzed the libnotify code, do you think it's better to keep track of the last created notification, or the first?

> Do we really have to create a new NotifyNotification object when using the
> append hint? Can't we just reuse the existing one?

Yes, that's how it is supposed to work. Here's a quote from the notify-osd ubuntu page [1]:
"Notify OSD should merge two notifications into a single notification if all of the following are true:
  [....]
  * the second notification is not specified as a replacement of any other notification."
Since there's no NotifyNotification method to change the body (unless _update which sets the replaces_id) the only way is to create new notifications.

[1]: https://wiki.ubuntu.com/NotifyOSD#merging

(In reply to comment #26)
> > I did it for you and it seems that unreffing the object in the closed_cb is the
> > good thing to do.
> 
> Thanks for having done this, I didn't understand, sorry.
> After having analyzed the libnotify code, do you think it's better to keep
> track of the last created notification, or the first?

We have to unref all the notifications, if not we leak them.

Could you check what happen if we always create a new notification rather than using notify_notification_update? Maybe we could just stop using priv->notification and unref notification in the closed_cb (assuming this callback is always called only once).

I didn't check, but I'm quite sure that notification-daemon creates another bubble which is "appended" under the current showing bubbles.
With notification-daemon the only way to handle this is using _update, otherwise your screen will be flooded by notifications.

I didn't understand the problem of priv->notification. It's used only for _update and to destroy the notification when the window is closed. It doesn't cause memory leaks, since the unref is done in the callback using an argument. If you prefer it is also possible to unref it immediately after having shown it (only if x-canonical-append is enabled).

I've tested it now and I confirm what I've said in the previous post.

Created attachment 157271 [details] [review]
git diff

I did some code refactoring, and returned to the old logic: keep the reference to the latest created notification.

While I was testing my patch I found a bizarre emapthy behavior about notifications (when the chat windows are closed): only the notifications from one contact are show.

Scenario:
- ALL chat windows closed
- message received from A: "test1" -> notification shown
- message received from B: "test2" -> NO notification
- message received from A: "test3" -> notification shown
[....]
The notifications from B are shown as soon as I click on the status icon.
Should I file a new bug or this is normal for you?

Guillaume, is there anything else I can do to improve the patch?

Review of attachment 157271 [details] [review]:

Sorry for the late reply, I've been pretty busy with the 2.30 release. :)
This looks much better to me.

The patch doesn't apply on top of master. Could you update it please?

::: src/empathy-status-icon.c
@@ +166,3 @@
+					EMPATHY_NOTIFY_MANAGER_CAP_X_CANONICAL_APPEND);
+
+		if (notification != NULL && ! has_x_canonical_append) {

Would be cool if you could add a comment here.

@@ +171,3 @@
 						    NULL);
 		} else {
+			notification = notify_notification_new_with_status_icon

and here (just to explain the 2 ways to handle notifications.

Created attachment 157589 [details] [review]
git diff

I added the comments, I hope they are clear (I'm not native English speaker).

Does the patch apply to master now? To tell the truth I didn't have any conflicts when running git pull to my branch (tracking master)...

I'm not really happy about the code duplication but that's not your fault. I hope to refactor that during this cycle.
I merged your branch to master; thanks a lot for your work on this.

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.

> I'm not really happy about the code duplication

I was about to open a new bug