Created attachment 153017 [details] [review]
initial chat work

issues:

  - there's currently no way to get back to the notification window
    from the summary mode, so if it hides itself, you can't respond until
    the person IMs you again :-)

  - the UI is all kinds of ugly

  - you have to click into the entry before you can type

  - once you click into the entry, it grabs the keyboard/pointer and the
    only way to escape is to hit Return

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

Created attachment 154614 [details] [review]
Revert "Special-handle Empathy to have multiple sources and to queue notifications"

(entirely-boring commit; this is just the result of "git revert")

Created attachment 154615 [details] [review]
Use Telepathy for IM notifications

And suppress libnotify-based notifications from Empathy

depends on the dbus utils in bug 610859

Created attachment 154617 [details] [review]
Add support for chatting directly from IM notifications

This is not the final version of this patch, but it's ready for review.

There are two notable UI problems at the moment:

    1. if an IM arrives, and is still in banner mode, and then a second
       IM arrives, the notification updates immediately. I believe the
       spec says that the first IM should get its N seconds, then hide
       itself, and then the second IM should appear

    2. the animation when adding rows to the expanded notification
       (because of a newly-received IM, or when adding a row for what
       you just sent) is not quite right, but I wanted to get input
       on what "right" is before putting too much work into it, since
       it's probably going to be annoying :)

(In reply to comment #2)
> issues:
> 
>   - there's currently no way to get back to the notification window
>     from the summary mode, so if it hides itself, you can't respond until
>     the person IMs you again :-)

this is still true, but that's a different bug (610726), and affects all types of notifications.

>   - the UI is all kinds of ugly

it now matches the latest mockups more closely

>   - you have to click into the entry before you can type

still true. Should it grab focus when you expand it?

>   - once you click into the entry, it grabs the keyboard/pointer and the
>     only way to escape is to hit Return

You can click anywhere outside the tray to get out now. Need to make Esc work too. Alt-Tab? What if the user opens the overview?

oh... and, currently, my "solution" for dealing with Empathy is to implement a Telepathy approver, which causes Empathy to completely not even see the conversation at all. (As in, it doesn't even get logged.) This needs to be improved on, but it was nicer than having empathy constantly beeping and flashing at you.

the conversation not getting logged in empathy is fixed by bug 518414, which I think also solves the problem of letting us populate the notification with a bit of the logs from the previous conversation(s).

Review of attachment 154615 [details] [review]:

Looks very good overall and works too (with a small modification to pass an id to the Notification constructor)!

I noticed that Empathy is not getting the messages with this code (I suppose that's because we define the Telepathy client as an Approver and call Claim() on the ChannelDispatchOperation objects we get). What is our long term plan with that?

This code also doesn't open any application when the notification or tray icons are clicked. This can be added as a separate patch later.
 
It also looks like this patch will need to be updated to use whatever the patch in https://bugzilla.gnome.org/show_bug.cgi?id=610859 ends up looking like.

::: js/misc/telepathy.js
@@ +169,3 @@
+          access: 'read' }
+    ],
+    signals: [

Properties and signals are listed in the different order from ConnectionRequestsIface. Would be nice to make it consistent.

::: js/ui/main.js
@@ +43,3 @@
 let messageTray = null;
 let windowAttentionHandler = null;
+let telepathyClient = null;

It would be nice to declare and define notificationDaemon, windowAttentionHandler, telepathyClient, and messageTray, in the same order here and below with the messageTray first in the list since others use it. Some comment about the other three using the message tray and generating different types of notifications would be nice too.

@@ +123,3 @@
     windowAttentionHandler = new WindowAttentionHandler.WindowAttentionHandler();
     messageTray = new MessageTray.MessageTray();
+    telepathyClient = new TelepathyClient.Client();

Define in a consistent order.

::: js/ui/notificationDaemon.js
@@ +139,3 @@
 
+        // Filter out notifications from Empathy, since we
+        // handle that information from telepathy.js

in telepathyClient.js ?

::: js/ui/telepathyClient.js
@@ +12,3 @@
+
+let avatarManager;
+

Some big picture comments here would really help. Like that this is a Telepathy client implementing http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.html and well as Observer and Approver client interfaces. We currently use it for getting the text messages, etc.

@@ +25,3 @@
+                                  function (name) { /* FIXME: lost */ });
+
+        this._conns = {};

this._conns is not used.

@@ +28,3 @@
+        this._channels = {};
+
+        avatarManager = new AvatarManager();

Could use a comment like "Initialize avatarManager once on the start-up of the GnomeShell Telepathy client."

@@ +55,3 @@
+                                let [channelPath, props] = channels[i];
+                                if (this._channels[channelPath])
+                                    continue;

Is there a reason we expect multiple results for the same channelPath here for which we ignore the later ones, but not in ObserveChannels()? Can we rather have this check in this._addChannel()?

@@ +71,3 @@
+    get ApproverChannelFilter() {
+        return [
+            { 'org.freedesktop.Telepathy.Channel.ChannelType': Telepathy.CHANNEL_TEXT_NAME }

Should use Telepathy.CHANNEL_NAME + '.ChannelType' like you do it in _addChannel() below.

@@ +78,3 @@
+        let sender = DBus.getCurrentMessageContext().sender;
+        let op = new Telepathy.ChannelDispatchOperation(sender, dispatchOperationPath);
+        op.ClaimRemote();

What is the goal of this? Does it result in the Empathy not getting the messages? The docs say "Clients that call Claim on channels but do not immediately close them SHOULD implement the Handler interface and its HandledChannels property." However, we are not implementing the Handler interface or closing the channel.

@@ +83,3 @@
+    get ObserverChannelFilter() {
+        return [
+            { 'org.freedesktop.Telepathy.Channel.ChannelType': Telepathy.CHANNEL_TEXT_NAME }

Use Telepathy.CHANNEL_NAME + '.ChannelType

@@ +90,3 @@
+                              dispatch_operation, requests_satisfied,
+                              observer_info) {
+        let conn = new Telepathy.Connection(connPath);

conn is not used

@@ +146,3 @@
+
+        info.updatedId = connAvs.connect('AvatarUpdated',
+                                         Lang.bind(this, this._avatarUpdated));

Why are info.updatedId, info.retrievedId, and info.statusChangedId defined outside of the initial structure? Would info be better of as a separate class?

@@ +153,3 @@
+            function (status, reason) {
+                if (status == Telepathy.ConnectionStatus.DISCONNECTED)
+                    this.removeConnection(conn);

It's this._removeConnection()

@@ +181,3 @@
+            // signal for an avatar while there's already a
+            // RequestAvatars() call pending, so we don't need to do
+            // anything.

Or when we have never requested info for this particular handle, right? It doesn't appear that AvatarUpdated is only sent for the handles for which the avatars have been requested.

@@ +291,3 @@
+
+        this._channelText = new Telepathy.ChannelText(this._conn.name, channelPath);
+        this._receivedId = this._channelText.connect('Received', Lang.bind(this, this._receivedMessage));

Maybe this._messageReceived to make it sound more function-like :).

@@ +334,3 @@
+
+    _init: function(source, text) {
+        MessageTray.Notification.prototype._init.call(this, source, source.name, text, true);

Latest Notification constructor also takes id, so we need to pass it in from _receivedMessage() and supply it to the base class constructor.

Review of attachment 154614 [details] [review]:

This works of course!

(In reply to comment #10)
> I noticed that Empathy is not getting the messages with this code (I suppose
> that's because we define the Telepathy client as an Approver and call Claim()
> on the ChannelDispatchOperation objects we get). What is our long term plan
> with that?

bug 518414 changes how logging works, so that both empathy and gnome-shell will be able to see the logs of conversations done in either of them.

> +            { 'org.freedesktop.Telepathy.Channel.ChannelType':
> Telepathy.CHANNEL_TEXT_NAME }
> 
> Should use Telepathy.CHANNEL_NAME + '.ChannelType' like you do it in
> _addChannel() below.

Tried that originally, but it's a syntax error. The property name in an object constructor has to be either a symbol or a constant string.

I could do:

    filter = {};
    filter[Telepathy.CHANNEL_NAME + '.ChannelType'] = Telepathy.CHANNEL_TEXT_NAME;
    return filter;

but that seems less clear.

> +        op.ClaimRemote();
> 
> What is the goal of this? Does it result in the Empathy not getting the
> messages?

Actually, just *declaring* the Approver results in Empathy not getting the messages, even if we don't claim them, but that's a bug in empathy (I think), and will probably change as a result of bug 599158.

You're right that we should be implementing Handler, because we call AcknowledgePendingMessages, which only Handlers are supposed to do.

> Why are info.updatedId, info.retrievedId, and info.statusChangedId defined
> outside of the initial structure? Would info be better of as a separate class?

It doesn't need to be a class; it doesn't have any behavior of its own, it's just a thingy to store data in. The signals were put outside the initial definition because it looked better that way I guess? I could initialize info to just {} and put everything outside instead.

> @@ +181,3 @@
> +            // signal for an avatar while there's already a
> +            // RequestAvatars() call pending, so we don't need to do
> +            // anything.
> 
> Or when we have never requested info for this particular handle, right? It
> doesn't appear that AvatarUpdated is only sent for the handles for which the
> avatars have been requested.

It doesn't say that explicitly but I'm pretty sure it's true. Google isn't going to just send you information about random avatars. :) (I guess it's possible you might get AvatarUpdated signals if another application Observing the same channel requested an avatar...)

Created attachment 156002 [details] [review]
Use Telepathy for IM notifications

addresses comments, and moves the dbus utils into js/misc/telepathy.js
for now

Created attachment 156003 [details] [review]
Add support for chatting directly from IM notifications

rebase. no substantive changes

Created attachment 157121 [details] [review]
Add support for chatting directly from IM notifications

Now updated with scrolling (which required changing a bunch of code
in messageTray.js. Which I should update the commit message about, yes...)

The scrollbar will be slightly too short unless you apply the patch from
bug 613964 first.

The scrollbar isn't very visible here, we might want design input.

Created attachment 157197 [details] [review]
Limit the total number of scrollback messages.

The idea in the current code is that if you're still talking to the
person, it will keep up to 20 messages, but after 15 minutes of
silence it will throw away most of the old messages to clean things up.

to be squashed with previous patch

Review of attachment 156002 [details] [review]:

Looks great! Just a couple minor comments.

Needs a reason in the body of the commit message, not just an implementation detail.

::: js/misc/telepathy.js
@@ +199,3 @@
+const ChannelIface = {
+    name: CHANNEL_NAME,
+    properties: [

Should this also define ChannelType property? We use it in _addChannels() in telepathyClient.js

::: js/ui/telepathyClient.js
@@ +138,3 @@
+DBus.conformExport(Client.prototype, Telepathy.ClientIface);
+DBus.conformExport(Client.prototype, Telepathy.ClientObserverIface);
+DBus.conformExport(Client.prototype, Telepathy.ClientApproverIface);

Do we need DBus.conformExport(Client.prototype, Telepathy.ClientHandlerIface); too? Let's list them in the same order we return them for Interfaces and also define their properties and methods, which is Approver, Handler, Observer.

@@ +316,3 @@
+
+        this._channelText = new Telepathy.ChannelText(DBus.session, connName, channelPath);
+        this._receivedId = this._channelText.connect('Received', Lang.bind(this, this._receivedMessage));

Let's call it _messageReceived()

fixed, squashed, etc.

> Should this also define ChannelType property? We use it in _addChannels() in
> telepathyClient.js

actually we weren't using any of those as D-Bus properties. I cleaned up
telepathy.js to only use the bits we were actually using.