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 645932 - Hard to accept incoming calls and file transfer
Hard to accept incoming calls and file transfer
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks: 646061
 
 
Reported: 2011-03-28 12:00 UTC by Guillaume Desmottes
Modified: 2011-04-01 22:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
diff (1.52 KB, patch)
2011-03-29 13:18 UTC, Guillaume Desmottes
reviewed Details | Review
notificationDaemon: only ignore 'chat' notifications from Empathy (#645932) (1.09 KB, patch)
2011-03-30 08:14 UTC, Guillaume Desmottes
needs-work Details | Review
notificationDaemon: only ignore 'chat' and 'presence' notifications from Empathy (#645932) (1.23 KB, patch)
2011-04-01 10:02 UTC, Guillaume Desmottes
accepted-commit_after_freeze Details | Review

Description Guillaume Desmottes 2011-03-28 12:00:12 UTC
When you receive an incoming call or file transfer in Empathy you get a notification bubble which quickly disappear and then have to go to the bottom bar and click on Empathy's status icon to accept it.

We should have a proper tray asking the user if he wants to accept/decline the request.
Comment 1 Giovanni Campagna 2011-03-28 13:34:10 UTC
Maybe a duplicate of bug 645753 (or can be fixed as part of it)
Comment 2 Owen Taylor 2011-03-28 14:07:59 UTC
I think it has been discussed that incoming phone calls should be "urgent" notifications while they are going on, so they are permanently onscreen until interacted with. Not sure if the same should apply to file transfers.
h
Comment 3 Guillaume Desmottes 2011-03-28 17:13:21 UTC
I think the shell should handle those itself as the user can always disable notification bubbles in Empathy's preferences and so end up with an unusable system.
Comment 4 Guillaume Desmottes 2011-03-28 17:58:30 UTC
(In reply to comment #2)
> I think it has been discussed that incoming phone calls should be "urgent"
> notifications while they are going on, so they are permanently onscreen until
> interacted with. Not sure if the same should apply to file transfers.

What's the proper way to mark a notification as "urgent" btw?
Comment 5 Dan Winship 2011-03-28 18:12:47 UTC
notify_notification_set_urgency (notification, NOTIFY_URGENCY_CRITICAL);
Comment 6 Guillaume Desmottes 2011-03-29 07:34:11 UTC
I investigated this a bit and think I found the issue.

Atm the Shell blacklists all the notifications coming from Empathy.
In notificationDaemon.js we have:
        // Filter out notifications from Empathy, since we
        // handle that information from telepathyClient.js
        if (appName == 'Empathy') {

But this is wrong as, atm, telepathyClient.js only handle 1-1 chats.

We have two options here:

A) improve telepathyClient.js to deal with other events (calls, file
transfers, muc invitations, etc)

B) only ignore 1-1 chat notifications and display the other one.


A) is pretty big change at this point, so I'd suggest to do B) for 3.0 and
progressively move to A) in the next cycle if we find the need to.


B) can be done pretty easily:
- Patch Empathy to set the urgent hint on most notifications. This will ensure
that the notifications stays visible in the Shell and that actions buttons
will be displayed.

- Only filter 1-1 chat in Shell's notification daemon. One way to do that
would be to add a 'x-empathy-event' hint or something that would describe the
kind of event which raised the notification. So the Shell would just have to
filter out the 'chat' ones.
Comment 8 Giovanni Campagna 2011-03-29 11:44:49 UTC
(In reply to comment #6)
> - Only filter 1-1 chat in Shell's notification daemon. One way to do that
> would be to add a 'x-empathy-event' hint or something that would describe the
> kind of event which raised the notification. So the Shell would just have to
> filter out the 'chat' ones.

You don't need 'x-empathy-event', setting 'category' should be enough and compatible with other notification servers.
Comment 9 Guillaume Desmottes 2011-03-29 12:20:32 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > - Only filter 1-1 chat in Shell's notification daemon. One way to do that
> > would be to add a 'x-empathy-event' hint or something that would describe the
> > kind of event which raised the notification. So the Shell would just have to
> > filter out the 'chat' ones.
> 
> You don't need 'x-empathy-event', setting 'category' should be enough and
> compatible with other notification servers.

Is there a list somewhere of well known keys? http://www.galago-project.org/specs/notification/0.9/x344.html doesn't mention any.
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-03-29 12:27:04 UTC
How about: http://people.gnome.org/~mccann/docs/notification-spec/notification-
spec-latest.html#categories
Comment 11 Florian Müllner 2011-03-29 12:28:32 UTC
The category list on http://www.galago-project.org/specs/notification/0.9/x211.html has 'im.received', which looks like a good match.
Comment 12 Guillaume Desmottes 2011-03-29 12:47:56 UTC
Cool, I updated both branches:
http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/shell-notif-645932
http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/empathy-notif-645932

I just set the category for the chat notifications as that's the one we care here and the only one which reallty fits with the existing category anyway.
We can add more categories later if needed.
Comment 13 Guillaume Desmottes 2011-03-29 13:18:19 UTC
Created attachment 184581 [details] [review]
diff

 js/ui/notificationDaemon.js |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
Comment 14 Owen Taylor 2011-03-29 13:31:41 UTC
Review of attachment 184581 [details] [review]:

::: js/ui/notificationDaemon.js
@@ +193,3 @@
         // handle that information from telepathyClient.js
         if (appName == 'Empathy') {
+            if (hints['category'] == 'im.received') {

I think it's just slightly weird to access hints before we call Params.parse() on it, though fine here.

My main concern with this patch is that it seems to break operation with Empathy without the Empathy patch applied. That could be considered a bit harsh.

Would it make sense for Empathy to set hints for everything, so we could do if (hints['category'] != null && hints['category'] == 'im.received') ?
Comment 15 Guillaume Desmottes 2011-03-29 13:43:31 UTC
(In reply to comment #14)
> My main concern with this patch is that it seems to break operation with
> Empathy without the Empathy patch applied. That could be considered a bit
> harsh.

I think that's fine tbh. We should ask for grouped hard code freeze exception to the RT and be sure to merge both branches for 3.0.
Gnome-shell testers shouldn't have troubles to update their Empathy and 'normal' users will have the whole GNOME 3.0 from their distributions.

> Would it make sense for Empathy to set hints for everything, so we could do if
> (hints['category'] != null && hints['category'] == 'im.received') ?

We'll have to define our own x-empathy categories then. I don't think it's worth it.
Comment 16 Dan Winship 2011-03-29 15:20:22 UTC
Comment on attachment 184581 [details] [review]
diff

patch looks fine, although I'd probably do

    if (appName == 'Empathy' && hints['category'] == 'im.received')

to avoid having to reindent everything else

Owen said:
> My main concern with this patch is that it seems to break operation with
> Empathy without the Empathy patch applied. That could be considered a bit
> harsh.

"Break" meaning that the user would get double notifications, not that they'd get no notifications. And that can be fixed if the user turns off "Enable bubble notifications" in the Empathy preferences.

There is also a second failure mode, which is that if the user has new Empathy and new gnome-shell, and has "Enable notifications when a contact comes online/goes offline" enabled in the Empathy prefs, then they will get double notification of (some) presence changes (because we won't be suppressing presence notifications from empathy any more).
Comment 17 Florian Müllner 2011-03-29 15:26:11 UTC
(In reply to comment #16)
> There is also a second failure mode, which is that if the user has new Empathy
> and new gnome-shell, and has "Enable notifications when a contact comes
> online/goes offline" enabled in the Empathy prefs, then they will get double
> notification of (some) presence changes (because we won't be suppressing
> presence notifications from empathy any more).

Would it be appropriate to use 'presence.online'/'presence.offline' for those and filter them as well?
Comment 18 Guillaume Desmottes 2011-03-30 08:10:10 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > There is also a second failure mode, which is that if the user has new Empathy
> > and new gnome-shell, and has "Enable notifications when a contact comes
> > online/goes offline" enabled in the Empathy prefs, then they will get double
> > notification of (some) presence changes (because we won't be suppressing
> > presence notifications from empathy any more).
> 
> Would it be appropriate to use 'presence.online'/'presence.offline' for those
> and filter them as well?

I updated the empathy branch to set those categories as well; see http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/shell-notif-645932

(In reply to comment #16)
> (From update of attachment 184581 [details] [review])
> patch looks fine, although I'd probably do
> 
>     if (appName == 'Empathy' && hints['category'] == 'im.received')
> 
> to avoid having to reindent everything else

done.
http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/empathy-notif-645932

> There is also a second failure mode, which is that if the user has new Empathy
> and new gnome-shell, and has "Enable notifications when a contact comes
> online/goes offline" enabled in the Empathy prefs, then they will get double
> notification of (some) presence changes (because we won't be suppressing
> presence notifications from empathy any more).

That's right. I see 3 options here:

a) Add a 'x-empathy-contact' hint in presence notifications and check in
notificationDaemon.js if we should discard the notification or not (by asking
to the telepathyClient).
That's s definitely the best solution but requiers non-trivial code changes,
which is probably a bit late for 3.0.

b) Ignore all presence notifications; keeping the current behaviour (atm all
Empathy notifications are ignored).

c) Display all presence notifications. I don't think it's that bad tbh, users
can always disable presence notifications if he wants to.

So I'd vote for b) and c); I don't really care which one, your call.
Comment 19 Guillaume Desmottes 2011-03-30 08:14:46 UTC
Created attachment 184672 [details] [review]
notificationDaemon: only ignore 'chat' notifications from Empathy (#645932)
Comment 20 Guillaume Desmottes 2011-03-31 13:15:19 UTC
For the record, I requested a hard code freeze exception to merge the empathy patchs.
Comment 21 Dan Winship 2011-03-31 21:57:59 UTC
Comment on attachment 184672 [details] [review]
notificationDaemon: only ignore 'chat' notifications from Empathy (#645932)

Let's go for (b) (filter out both chat and presence notifications). I do think (c) would be good, but at this point I think it's best to not change the user experience any more than we have to.

(Perhaps for 3.0.1 we could watch the relevant empathy gsettings key, and if empathy is configured to show presence notifications, then let them through and suppress our own.)
Comment 22 Guillaume Desmottes 2011-04-01 10:02:19 UTC
Created attachment 184857 [details] [review]
notificationDaemon: only ignore 'chat' and 'presence' notifications from Empathy (#645932)
Comment 23 Guillaume Desmottes 2011-04-01 10:03:14 UTC
Here we go, this patch implements b).
Comment 24 Dan Winship 2011-04-01 15:32:30 UTC
Comment on attachment 184857 [details] [review]
notificationDaemon: only ignore 'chat' and 'presence' notifications from Empathy (#645932)

tested and this works fine. marking commit-after-freeze now. Needs ok from Owen and release-team to be commit-now.
Comment 25 Marina Zhurakhinskaya 2011-04-01 17:22:49 UTC
So the patch filters out online and offline presence changes. What happens with other types of presence changes, such as busy or idle? Does Empathy notify on these if the user has presence notifications enabled? If so, this will cause some Empathy presence notifications to be showing up.
Comment 26 Guillaume Desmottes 2011-04-01 17:48:29 UTC
No, Empathy only notify online/offline presence changes so no problem here.
Comment 27 Guillaume Desmottes 2011-04-01 22:06:10 UTC
Approved by the RT. I merged the g-s and empathy branches.

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.