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 620416 - Port the telepathy client to telepathy-glib
Port the telepathy client to telepathy-glib
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: High normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks: 639943
 
 
Reported: 2010-06-02 23:25 UTC by Morten Mjelva
Modified: 2011-02-16 09:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adding telepathy-glib as a dependency (9.61 KB, patch)
2010-06-02 23:25 UTC, Morten Mjelva
none Details | Review
Wrap tp_connection_get_contacts_by_handle (6.26 KB, patch)
2010-06-02 23:26 UTC, Morten Mjelva
reviewed Details | Review
Refactoring telepathyClient to use telepathy-glib (30.03 KB, patch)
2010-06-02 23:27 UTC, Morten Mjelva
needs-work Details | Review
Removing unneeded proxies in telepathy library (9.61 KB, patch)
2010-06-02 23:28 UTC, Morten Mjelva
reviewed Details | Review
Adding telepathy-glib as a dependency (1.49 KB, patch)
2010-06-03 21:08 UTC, Morten Mjelva
reviewed Details | Review
Squashed diff (45.71 KB, patch)
2011-02-09 10:49 UTC, Guillaume Desmottes
reviewed Details | Review
new diff (45.64 KB, patch)
2011-02-10 14:21 UTC, Guillaume Desmottes
accepted-commit_now Details | Review

Description Morten Mjelva 2010-06-02 23:25:19 UTC
Created attachment 162590 [details] [review]
Adding telepathy-glib as a dependency

telepathy-glib is rapidly maturing, and it gives us a lot for free, simplifying our current code. it also allows things that would be very difficult to do without it.
Comment 1 Morten Mjelva 2010-06-02 23:26:21 UTC
Created attachment 162591 [details] [review]
Wrap tp_connection_get_contacts_by_handle
Comment 2 Morten Mjelva 2010-06-02 23:27:00 UTC
Created attachment 162592 [details] [review]
Refactoring telepathyClient to use telepathy-glib
Comment 3 Morten Mjelva 2010-06-02 23:28:20 UTC
Created attachment 162593 [details] [review]
Removing unneeded proxies in telepathy library

Using telepathy-glib, we no longer need most of the proxies we've been using.
Some are still left, due to telepathy-glib still being somewhat incomplete.
Comment 4 Morten Mjelva 2010-06-03 21:08:07 UTC
Created attachment 162692 [details] [review]
Adding telepathy-glib as a dependency
Comment 5 Marina Zhurakhinskaya 2010-06-05 00:40:58 UTC
Review of attachment 162692 [details] [review]:

My suggestion for the subject was "Add telepathy-glib as a dependency". We usually use the verb in the infinitive form for the subject.

Looks good and works like a charm!

::: tools/build/gnome-shell.modules
@@ +102,3 @@
+      <branch repo="git.collabora.co.uk" module="telepathy-glib"/>
+      <dependencies>
+          <dep package="gobject-introspection"/>

I realize we are not consistent about it throughout the file, but the places nearest to this use a two space indent, so this should too.
Comment 6 Marina Zhurakhinskaya 2010-06-07 21:05:13 UTC
Review of attachment 162591 [details] [review]:

Looks good. A lot of neatpicks in the comments, but nothing major. Would be good to have Owen or Colin look over the way you define GetTpContactsCb and the code in shell_global_get_tp_contacts_cb().

There are 4 trailing white spaces in this patch. Here are the warnings about them from applying it:

Applying: Wrap tp_connection_get_contacts_by_handle
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:88: trailing whitespace.
    
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:110: trailing whitespace.
 * transforming.
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:123: trailing whitespace.
    tp_connection_get_contacts_by_handle(self, n_handles, handles,
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:124: trailing whitespace.
            n_features, features, shell_global_get_tp_contacts_cb,
warning: 4 lines add whitespace errors.

Here is a simple method for removing whitespaces that Colin came up with:
emacs <filename> -f delete-trailing-whitespace -f save-buffer -f kill-emacs

Though the occasional problem with it is that if there were already trailing whitespaces in the file not introduced by your patch, it will remove them too. However, we don't like patches that mix in random fixes, so then you'd need to undo that :).

You can also test for whitespaces by doing 'git format-patch', reverting, and doing 'git am' which will show you the warnings if there are trailing whitespaces.

::: src/shell-global.c
@@ +6,3 @@
 #include "shell-perf-log.h"
 #include "shell-wm.h"
+#include "shell-global.h"

shell-global.h is included in shell-global-private.h, so don't need it here.

@@ +1617,3 @@
+ * shell_global_get_tp_contacts_cb: (skip)
+ */
+

Extra line here.

@@ +1625,3 @@
+                                 const TpHandle *failed,
+                                 const GError *error,
+                                 gpointer callback,

I think it's clearer to call this variable user_data or original_callback.

@@ +1629,3 @@
+{
+    int i;
+    GList *contact_list = NULL;

Should be a two space indentation.

@@ +1630,3 @@
+    int i;
+    GList *contact_list = NULL;
+    for(i = 0; i < n_contacts; i++) {

Space after for.

@@ +1634,3 @@
+    }
+
+    TpHandle* failed_list = g_new0 (TpHandle, n_failed+1);

should be "TpHandle *failed_list"; spaces around +.

@@ +1655,3 @@
+ *
+ * Wrap tp_connection_get_contacts_by_handle so we can transform the array
+ * into a null-terminated one, which gjs can handle

Period missing.

@@ +1656,3 @@
+ * Wrap tp_connection_get_contacts_by_handle so we can transform the array
+ * into a null-terminated one, which gjs can handle
+ * We send the original callback to get_contacts_by_handle as

tp_connection_get_contacts_by_handle

@@ +1673,3 @@
+    tp_connection_get_contacts_by_handle(self, n_handles, handles, 
+            n_features, features, shell_global_get_tp_contacts_cb, 
+            callback, NULL, NULL);

Align all lines of arguments with the beginning of the first one.

::: src/shell-global.h
@@ +12,3 @@
+#include <telepathy-glib/connection.h>
+#include <telepathy-glib/contact.h>
+#include <telepathy-glib/handle.h>

It looks like capabilities.h, connection.h, and handle.h are included in contact.h, so maybe don't need to include it here. Not sure what our policy on that is. At the very least, I don't see us using anything from capabilities.h in this patch

@@ +128,3 @@
+
+void shell_global_get_tp_contacts (TpConnection *self,
+                              guint n_handles,

Alignment needs to be fixed.
Comment 7 Marina Zhurakhinskaya 2010-06-09 05:23:19 UTC
Review of attachment 162592 [details] [review]:

Suggested commit title:
Refactor Telepathy client to use telepathy-glib

The patch works well, but there are many small things that need to be fixed.

Here are two regressions in behavior that I observed. Ideally, they should be fixed before we commit the changes:
1) Presence notifications about away/busy (and coming back from these states) should be suppressed, while that information should still be shown in the chat. Suggested improvements:
  - One problem with the previous implementation is that it was not showing someone signing in in a busy state, we should probably fix that. E.g. we can say "Marina is online", but then immediately print "Marina is busy" in the message log if one exists.
  - Should we say "available", rather than "online" when someone comes back from being "busy" or "away"?
2) Avatar was not appearing for a long time. When the icon in the notification finally updated, the icon in the message tray did not. There are some suggestions for improving how we handle avatar changes inlined in the comments below. Let's try that and see if it results in the icon loading more promptly.

Other problems that appear in both versions. These are here for future reference and should be addressed when the initial patches land.
- New message first shows up in the title line before showing up inline when you have a conversation that is already expanded.
- Message tray did not slide out when an icon was clicked to bring up a chat window, while it should slide out.
- Presence updates are only showing up while the conversation is in the tray (not before it was started or after it was closed). Can we do better now?
  - We are still using ConnectionSimplePresence from telepathy.js, but in an IRC conversation Morten said that there is a new contacts interface being developed in telepathy-glib.
- We are not getting updates on alias changes.

Whitespace warnings when applying:

Applying: Refactoring telepathyClient to use telepathy-glib
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:103: trailing whitespace.
        // The second argument, recover, means ObserveChannels will be run
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:209: trailing whitespace.
                    Lang.bind(this,
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:218: trailing whitespace.
 
/home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:719: trailing whitespace.
    
warning: 4 lines add whitespace errors.

::: js/ui/telepathyClient.js
@@ +47,3 @@
+        let dbus = Tp.DBusDaemon.dup();
+        this.observer = Tp.SimpleObserver.new(dbus, true, "GnomeShell", true,
+                Lang.bind(this, this.ObserveChannels));

Align with where the first line of arguments starts.

If we are no longer implementing any kind of an interface with the class itself, we should rename ObserveChannels into _observeChannels

@@ +52,3 @@
+        this.observer.add_observer_filter({
+            'org.freedesktop.Telepathy.Channel.ChannelType': Tp.IFACE_CHANNEL_TYPE_TEXT,
+            'org.freedesktop.Telepathy.Channel.TargetHandleType': Tp.HandleType.CONTACT,

Don't we still need a filter for
Telepathy.CHANNEL_TEXT_NAME, Telepathy.HandleType.NONE combination?

The comment in the original code says:
// Some protocols only support 'multi-user' chats, and single-user			
// chats are just treated as multi-user chats with only one other			
// participant. Telepathy uses HandleType.NONE for all chats in these			
// protocols; there's no good way for us to tell if the channel is			
// single- or multi-user.

@@ +63,3 @@
 
+    ObserveChannels: function(observer, account, conn, channels,
+                              dispatch_op, requests, context, user_data) {

If we are no longer implementing any kind of an interface with the class itself, we should rename ObserveChannels into _observeChannels

@@ +69,1 @@
+        if(!this._connections[connPath]) {

Space after if.

@@ +79,1 @@
+        for (let i in channels) {

We should use for (let i = 0; i < channels.length; i++) for arrays because that guarantees ordering (or consistency with places where we care about ordering) and guarantees that no other property of the Array object will get included in the loop.

@@ +84,2 @@
                 continue;
+            }

Let's put the logic of creating a sourceId into a helper function (see related comment below).

Also, we usually don't put curly braces around a single line body, unless other parts of the statement have multiple lines (e.g. there is an else with multiple lines).

@@ +91,3 @@
+                    Lang.bind(this, 
+                        function(connection, contacts, failed) {
+                            let contact = contacts[0];

We should test to make sure contacts.length > 0

@@ +94,3 @@
+
+                            let source = new Source(account, conn, contact, chan);
+                            source._destroyId = source.connect('destroy', Lang.bind(this, this._removeSource));

Underscore (_) is used to indicate that the field is private or protected, so this should be source.destroyId However, I think it is wrong to be using a Source field for that because what if something else was connecting to the 'destroy' signal and setting it to something else. So I think the right thing would be to keep a map {sourceId, destroyId} locally or not use destroyIds at all. this._removeSource() only gets called from this callback currently, so I don't think you need to disconnect distroyId in that case. Perhaps just moving _removeSource() implementation back here to be inlined will make things more clear. I know that would be an inlined function inside an inlined function...

@@ +95,3 @@
+                            let source = new Source(account, conn, contact, chan);
+                            source._destroyId = source.connect('destroy', Lang.bind(this, this._removeSource));
+                            this._sources[source.object_path] = source;

See comment about source.object_path below.

@@ +96,3 @@
+                            source._destroyId = source.connect('destroy', Lang.bind(this, this._removeSource));
+                            this._sources[source.object_path] = source;
+                        }), null);

null should be on a new line aligned with other get_tp_contacts() arguments.

@@ +104,3 @@
 
+    // Presence of a contact has changed.
+    _presencesChanged: function(proxy, presences, err) {

Is this signature right? err doesn't match up to the last two parameters here http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-connection-simple-presence.html#tp-cli-connection-interface-simple-presence-signal-callback-presences-changed

@@ +109,2 @@
         for (let handle in presences) {
+            let source = this._sources[connPath + ':' + handle];

Maybe makeSourceId needs to take connPath and handle in the light of this use case.

@@ +111,3 @@
+            if(source) {
+                source.changePresence(presences[handle]);
+            }

Space after if, no curly braces.

@@ +123,3 @@
+        source.disconnect(source._destroyId);
+        delete this._sources[source.object_path];
+        return;

Don't need this return statement.

@@ +127,3 @@
 
+    get Interfaces() {
+        return [ Tp.IFACE_CLIENT_OBSERVER ]

Is this still needed?

@@ +134,2 @@
+function Source(account, conn, contact, chan) {
+    this._init(account, conn, contact, chan);

I'd prefer to call the variable channel rather than chan.

@@ +147,1 @@
+        chan._invalidatedId = chan.connect('invalidated', Lang.bind(this, this._removeChannel));

Don't think you need to save _invalidatedId, but if you do, it's best to make it its own variable. this._channelInvalidatedId.

@@ +149,2 @@
+        this._channelText = new Telepathy.ChannelText(DBus.session, conn.get_bus_name(), chan.get_object_path());
+        this._channelText._receivedId = this._channelText.connect('Received', Lang.bind(this, this._messageReceived));

Please keep it as this._receivedId or this._channelTextReceivedId

@@ +153,1 @@
+        MessageTray.Source.prototype._init.call(this, contact.get_identifier());

Call to the base class constructor should always be the first thing in the subclass constructor.

@@ +155,1 @@
+        this._ensureNotification();

Is there a reason we need it here rather than in this._messageReceived() and this.changePresence()? If so, we can move this line up before connecting to the 'Received' signal and remove it from this._messageReceived() and this.changePresence().

@@ +163,1 @@
+        let file = this._contact.get_avatar_file();

You also need to set things up to listen to "notify::avatar-token" changes and get_avatar_file() again on change. You need to keep an array of all iconBox objects you create, update them on change, and remove them from the array when they are destroyed. See how that was done in ContactManager::createAvatar() and ContactManager::_updateIcons() in the original code.

@@ +163,2 @@
+        let file = this._contact.get_avatar_file();
+        if(file) {

Space after if.

@@ +165,3 @@
+            let uri = file.get_uri();
+            iconBox.child = textureCache.load_uri_sync(St.TextureCachePolicy.FOREVER,
+                                                       uri, size, size)

Can this be a call to load_uri_async() ?

@@ +178,3 @@
+                                              { 'org.freedesktop.Telepathy.Channel.ChannelType': Tp.IFACE_CHANNEL_TYPE_TEXT,
+                                                'org.freedesktop.Telepathy.Channel.TargetHandle': handle,
+                                                'org.freedesktop.Telepathy.Channel.TargetHandleType': Tp.HandleType.CONTACT },

Why not get a handle type from the handle? Based on the comment in the original code, it is possible for it to be Tp.HandleType.NONE

@@ +192,1 @@
     _gotChannelRequest: function (chanReqPath, ex) {

It's logical to have this function be right after clicked()

@@ +210,3 @@
+    _removeChannel: function(chan) {
+        chan.disconnect(chan._invalidatedId);
+        this._channelText.disconnect(this._channelText._receivedId);

See the comments about these ids in the constructor.

@@ +236,3 @@
     },
 
+    changePresence: function(presence) {

As noted in the main comment and discussed on IRC, we want to preserve the logic that the presence change notifications are only shown when the user goes online/offline, but are always appended to the chat log.

I also like presenceChanged() as a name a little better.

@@ +242,3 @@
+        switch(type) {
+            case Tp.ConnectionPresenceType.AVAILABLE:
+                msg = _("%s is online").format(this.name);

Notice that the original messages had periods in the end. Probably no reason to loose them.

@@ +269,1 @@
         this._ensureNotification();

If you have this in the constructor, we shouldn't need it here.

@@ +273,3 @@
+
+    get name() {
+        return this._contact.get_alias();

Can we just set this.name and this.object_path in the constructor? We also need to rename object_path to be in lowerCamelCase. I think sourceId would be a better name. How about putting the logic for creating sourceId into a makeSourceId static helper function that takes connection and contact, which we can use when setting sourceId and when testing is this._sources in Client already has the source for an observed channel?

Though it doesn't appear to be a part of the original code, we need to connect to "notify::alias" here and have a way of updating the actors that use it. This can be done as a separate patch later. (For future reference, here is a way to change the name in Google accounts http://mail.google.com/support/bin/answer.py?answer=8158 )
Comment 8 Morten Mjelva 2010-06-09 16:02:30 UTC
(In reply to comment #7)
> Review of attachment 162592 [details] [review]:
> 
> Suggested commit title:
> Refactor Telepathy client to use telepathy-glib
> 
> The patch works well, but there are many small things that need to be fixed.
> 
> Here are two regressions in behavior that I observed. Ideally, they should be
> fixed before we commit the changes:
> 1) Presence notifications about away/busy (and coming back from these states)
> should be suppressed, while that information should still be shown in the chat.
> Suggested improvements:
>   - One problem with the previous implementation is that it was not showing
> someone signing in in a busy state, we should probably fix that. E.g. we can
> say "Marina is online", but then immediately print "Marina is busy" in the
> message log if one exists.
>   - Should we say "available", rather than "online" when someone comes back
> from being "busy" or "away"?
> 2) Avatar was not appearing for a long time. When the icon in the notification
> finally updated, the icon in the message tray did not. There are some
> suggestions for improving how we handle avatar changes inlined in the comments
> below. Let's try that and see if it results in the icon loading more promptly.
> 
> Other problems that appear in both versions. These are here for future
> reference and should be addressed when the initial patches land.
> - New message first shows up in the title line before showing up inline when
> you have a conversation that is already expanded.
> - Message tray did not slide out when an icon was clicked to bring up a chat
> window, while it should slide out.

The existing functionality was to launch the conversation in the user's preferred client, when the source icon was clicked. We can move this to some other trigger, and make clicking the icon expand the notification. It seems redundant however, as the notification expands on hover. I think this needs design input.

> - Presence updates are only showing up while the conversation is in the tray
> (not before it was started or after it was closed). Can we do better now?

We can do better.

>   - We are still using ConnectionSimplePresence from telepathy.js, but in an
> IRC conversation Morten said that there is a new contacts interface being
> developed in telepathy-glib.

I have a patch for this. It uses a patch I've written for telepathy-glib which adds a presence-changed signal to the TpContact object. I'll wrap that up so it can be merged.

> - We are not getting updates on alias changes.

We can connect to the notify::alias signal on TpContact. All notify signals trigger the same callback, and you need to decide which action to take based on whatever property triggers the callback. Unfortunately, that information is in a gobject.GParamSpec, which Gjs doesn't support.

> 
> Whitespace warnings when applying:
> 
> Applying: Refactoring telepathyClient to use telepathy-glib
> /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:103:
> trailing whitespace.
>         // The second argument, recover, means ObserveChannels will be run
> /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:209:
> trailing whitespace.
>                     Lang.bind(this,
> /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:218:
> trailing whitespace.
> 
> /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:719:
> trailing whitespace.
> 
> warning: 4 lines add whitespace errors.
> 
> ::: js/ui/telepathyClient.js
> @@ +47,3 @@
> +        let dbus = Tp.DBusDaemon.dup();
> +        this.observer = Tp.SimpleObserver.new(dbus, true, "GnomeShell", true,
> +                Lang.bind(this, this.ObserveChannels));
> 
> Align with where the first line of arguments starts.
> 
> If we are no longer implementing any kind of an interface with the class
> itself, we should rename ObserveChannels into _observeChannels
> 
> @@ +52,3 @@
> +        this.observer.add_observer_filter({
> +            'org.freedesktop.Telepathy.Channel.ChannelType':
> Tp.IFACE_CHANNEL_TYPE_TEXT,
> +            'org.freedesktop.Telepathy.Channel.TargetHandleType':
> Tp.HandleType.CONTACT,
> 
> Don't we still need a filter for
> Telepathy.CHANNEL_TEXT_NAME, Telepathy.HandleType.NONE combination?
> 
> The comment in the original code says:
> // Some protocols only support 'multi-user' chats, and single-user            
> // chats are just treated as multi-user chats with only one other            
> // participant. Telepathy uses HandleType.NONE for all chats in these           
> // protocols; there's no good way for us to tell if the channel is            
> // single- or multi-user.
> 

This was true of Butterfly, the MSN connection manager. It's no longer true, so only actual multi-user chats use TargetHandleType.NONE. The MSN protocol has no concept of single user channels, but Telepathy now converts MSN channels to single or multi-user as needed.

> @@ +63,3 @@
> 
> +    ObserveChannels: function(observer, account, conn, channels,
> +                              dispatch_op, requests, context, user_data) {
> 
> If we are no longer implementing any kind of an interface with the class
> itself, we should rename ObserveChannels into _observeChannels
> 
> @@ +69,1 @@
> +        if(!this._connections[connPath]) {
> 
> Space after if.
> 
> @@ +79,1 @@
> +        for (let i in channels) {
> 
> We should use for (let i = 0; i < channels.length; i++) for arrays because that
> guarantees ordering (or consistency with places where we care about ordering)
> and guarantees that no other property of the Array object will get included in
> the loop.
> 
> @@ +84,2 @@
>                  continue;
> +            }
> 
> Let's put the logic of creating a sourceId into a helper function (see related
> comment below).
> 
> Also, we usually don't put curly braces around a single line body, unless other
> parts of the statement have multiple lines (e.g. there is an else with multiple
> lines).
> 
> @@ +91,3 @@
> +                    Lang.bind(this, 
> +                        function(connection, contacts, failed) {
> +                            let contact = contacts[0];
> 
> We should test to make sure contacts.length > 0
> 
> @@ +94,3 @@
> +
> +                            let source = new Source(account, conn, contact,
> chan);
> +                            source._destroyId = source.connect('destroy',
> Lang.bind(this, this._removeSource));
> 
> Underscore (_) is used to indicate that the field is private or protected, so
> this should be source.destroyId However, I think it is wrong to be using a
> Source field for that because what if something else was connecting to the
> 'destroy' signal and setting it to something else. So I think the right thing
> would be to keep a map {sourceId, destroyId} locally or not use destroyIds at
> all. this._removeSource() only gets called from this callback currently, so I
> don't think you need to disconnect distroyId in that case. Perhaps just moving
> _removeSource() implementation back here to be inlined will make things more
> clear. I know that would be an inlined function inside an inlined function...
> 

Occupational hazard from dealing with too many dbus proxies, I'm afraid. Those remain unless you disconnect all their signals. Source is just another gobject, so it will be cleaned up automatically, so this has been removed.

> @@ +95,3 @@
> +                            let source = new Source(account, conn, contact,
> chan);
> +                            source._destroyId = source.connect('destroy',
> Lang.bind(this, this._removeSource));
> +                            this._sources[source.object_path] = source;
> 
> See comment about source.object_path below.
> 
> @@ +96,3 @@
> +                            source._destroyId = source.connect('destroy',
> Lang.bind(this, this._removeSource));
> +                            this._sources[source.object_path] = source;
> +                        }), null);
> 
> null should be on a new line aligned with other get_tp_contacts() arguments.
> 
> @@ +104,3 @@
> 
> +    // Presence of a contact has changed.
> +    _presencesChanged: function(proxy, presences, err) {
> 
> Is this signature right? err doesn't match up to the last two parameters here
> http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-connection-simple-presence.html#tp-cli-connection-interface-simple-presence-signal-callback-presences-changed

fixed

> 
> @@ +109,2 @@
>          for (let handle in presences) {
> +            let source = this._sources[connPath + ':' + handle];
> 
> Maybe makeSourceId needs to take connPath and handle in the light of this use
> case.
> 
> @@ +111,3 @@
> +            if(source) {
> +                source.changePresence(presences[handle]);
> +            }
> 
> Space after if, no curly braces.
> 
> @@ +123,3 @@
> +        source.disconnect(source._destroyId);
> +        delete this._sources[source.object_path];
> +        return;
> 
> Don't need this return statement.
> 
> @@ +127,3 @@
> 
> +    get Interfaces() {
> +        return [ Tp.IFACE_CLIENT_OBSERVER ]
> 
> Is this still needed?
> 
> @@ +134,2 @@
> +function Source(account, conn, contact, chan) {
> +    this._init(account, conn, contact, chan);
> 
> I'd prefer to call the variable channel rather than chan.
> 
> @@ +147,1 @@
> +        chan._invalidatedId = chan.connect('invalidated', Lang.bind(this,
> this._removeChannel));
> 
> Don't think you need to save _invalidatedId, but if you do, it's best to make
> it its own variable. this._channelInvalidatedId.
> 
> @@ +149,2 @@
> +        this._channelText = new Telepathy.ChannelText(DBus.session,
> conn.get_bus_name(), chan.get_object_path());
> +        this._channelText._receivedId = this._channelText.connect('Received',
> Lang.bind(this, this._messageReceived));
> 
> Please keep it as this._receivedId or this._channelTextReceivedId
> 
> @@ +153,1 @@
> +        MessageTray.Source.prototype._init.call(this,
> contact.get_identifier());
> 
> Call to the base class constructor should always be the first thing in the
> subclass constructor.
> 
> @@ +155,1 @@
> +        this._ensureNotification();
> 
> Is there a reason we need it here rather than in this._messageReceived() and
> this.changePresence()? If so, we can move this line up before connecting to the
> 'Received' signal and remove it from this._messageReceived() and
> this.changePresence().
> 
> @@ +163,1 @@
> +        let file = this._contact.get_avatar_file();
> 
> You also need to set things up to listen to "notify::avatar-token" changes and
> get_avatar_file() again on change. You need to keep an array of all iconBox
> objects you create, update them on change, and remove them from the array when
> they are destroyed. See how that was done in ContactManager::createAvatar() and
> ContactManager::_updateIcons() in the original code.
> 

Storing the iconBoxes proved to be a problem, as they're being destroyed by messageTray.Notification.update(). It's really a non-problem though, as we're simply creating an actor for a texture we have in cache.
Source.createIcon() is called by Notification.update(), so this would refresh whenever the notification changes. The file URI we get from telepathy-glib will always be current, and St.TextureCache will return a texture from cache if we've requested the same URI previously.

> @@ +163,2 @@
> +        let file = this._contact.get_avatar_file();
> +        if(file) {
> 
> Space after if.
> 
> @@ +165,3 @@
> +            let uri = file.get_uri();
> +            iconBox.child =
> textureCache.load_uri_sync(St.TextureCachePolicy.FOREVER,
> +                                                       uri, size, size)
> 
> Can this be a call to load_uri_async() ?
> 

No. load_uri_async() doesn't cache.

> @@ +178,3 @@
> +                                              {
> 'org.freedesktop.Telepathy.Channel.ChannelType': Tp.IFACE_CHANNEL_TYPE_TEXT,
> +                                               
> 'org.freedesktop.Telepathy.Channel.TargetHandle': handle,
> +                                               
> 'org.freedesktop.Telepathy.Channel.TargetHandleType': Tp.HandleType.CONTACT },
> 
> Why not get a handle type from the handle? Based on the comment in the original
> code, it is possible for it to be Tp.HandleType.NONE

See my comments above.

> 
> @@ +192,1 @@
>      _gotChannelRequest: function (chanReqPath, ex) {
> 
> It's logical to have this function be right after clicked()
> 
> @@ +210,3 @@
> +    _removeChannel: function(chan) {
> +        chan.disconnect(chan._invalidatedId);
> +        this._channelText.disconnect(this._channelText._receivedId);
> 
> See the comments about these ids in the constructor.
> 
> @@ +236,3 @@
>      },
> 
> +    changePresence: function(presence) {
> 
> As noted in the main comment and discussed on IRC, we want to preserve the
> logic that the presence change notifications are only shown when the user goes
> online/offline, but are always appended to the chat log.

I think this needs design input.

> 
> I also like presenceChanged() as a name a little better.
> 
> @@ +242,3 @@
> +        switch(type) {
> +            case Tp.ConnectionPresenceType.AVAILABLE:
> +                msg = _("%s is online").format(this.name);
> 
> Notice that the original messages had periods in the end. Probably no reason to
> loose them.
> 
> @@ +269,1 @@
>          this._ensureNotification();
> 
> If you have this in the constructor, we shouldn't need it here.
> 
> @@ +273,3 @@
> +
> +    get name() {
> +        return this._contact.get_alias();
> 
> Can we just set this.name and this.object_path in the constructor? We also need
> to rename object_path to be in lowerCamelCase. I think sourceId would be a
> better name. How about putting the logic for creating sourceId into a
> makeSourceId static helper function that takes connection and contact, which we
> can use when setting sourceId and when testing is this._sources in Client
> already has the source for an observed channel?
> 
> Though it doesn't appear to be a part of the original code, we need to connect
> to "notify::alias" here and have a way of updating the actors that use it. This
> can be done as a separate patch later. (For future reference, here is a way to
> change the name in Google accounts
> http://mail.google.com/support/bin/answer.py?answer=8158 )

See my comments above. It's definitely something we want to do, but Gjs needs to be fixed first.

Whatever I haven't commented on here, consider it fixed.
Comment 9 Marina Zhurakhinskaya 2010-06-09 19:58:46 UTC
(In reply to comment #8)
> (In reply to comment #7)

> > Other problems that appear in both versions. These are here for future
> > reference and should be addressed when the initial patches land.

> > - Message tray did not slide out when an icon was clicked to bring up a chat
> > window, while it should slide out.
> 
> The existing functionality was to launch the conversation in the user's
> preferred client, when the source icon was clicked. We can move this to some
> other trigger, and make clicking the icon expand the notification. It seems
> redundant however, as the notification expands on hover. I think this needs
> design input.

I meant that the message tray doesn't disappear when you click on the icon. Clicking on the icon is supposed to both bring up a chat window AND make the message tray disappear.

> > 
> > @@ +163,1 @@
> > +        let file = this._contact.get_avatar_file();
> > 
> > You also need to set things up to listen to "notify::avatar-token" changes and
> > get_avatar_file() again on change. You need to keep an array of all iconBox
> > objects you create, update them on change, and remove them from the array when
> > they are destroyed. See how that was done in ContactManager::createAvatar() and
> > ContactManager::_updateIcons() in the original code.
> > 
> 
> Storing the iconBoxes proved to be a problem, as they're being destroyed by
> messageTray.Notification.update(). It's really a non-problem though, as we're
> simply creating an actor for a texture we have in cache.
> Source.createIcon() is called by Notification.update(), so this would refresh
> whenever the notification changes. The file URI we get from telepathy-glib will
> always be current, and St.TextureCache will return a texture from cache if
> we've requested the same URI previously.

I see what you are saying, but while the notification icon is updated in Notification::update(), the message tray icon that gets added in MessageTray::add() is never updated. Keeping iconBoxes in an array shouldn't be a problem if you connect to destoy() on them and remove them from the array when that happens.
Comment 10 Marina Zhurakhinskaya 2010-06-09 20:26:01 UTC
Review of attachment 162593 [details] [review]:

Suggested commit title: Remove unneeded proxies in Telepathy library

Looks good. Here are some other things we don't use that you can remove:
SetPresence() from ConnectionSimplePresence
AcknowledgePendingMessages() from ChannelText
Succeeded and Failed signals from ChannelRequest
Comment 11 Guillaume Desmottes 2011-01-19 12:31:49 UTC
I strongly suggest you to use TpBaseClient and/or TpSimple* it would be your code much simpler.
Comment 12 Guillaume Desmottes 2011-01-19 12:33:42 UTC
This is blocking you from using TpTextChannel (bug #639943).
Comment 13 Dan Winship 2011-01-26 22:33:45 UTC
(In reply to comment #12)
> This is blocking you from using TpTextChannel (bug #639943).

Right, but is that blocking us (or you?) from anything? We understand that porting to tp-glib will simplify things *in the long run*, but it all pretty much works now, and we've got lots of other stuff to work on before 3.0...
Comment 14 Morten Mjelva 2011-01-27 00:43:13 UTC
Right. I've kinda put off finalizing the port to tp-glib because it would still depend on the existing homebrewed telepathy library. I *think* tp-glib is now at the stage where all the existing functionality in the gnome shell client could be implemented using tp-glib alone, so I should probably have another look at it and make it use TpTextChannel.
Comment 15 Guillaume Desmottes 2011-01-27 09:33:22 UTC
Using tp-glib and TpTextChannel would really simplify your code and, hopefully, make it more robust as well as we've spend a lot of time implementing and testing it.

It will also allow us to improve the existing Telepathy integration; for example the Shell should be an Approver and a Handler so Empathy would stock blicking when the channel is displayed by the shell. It will also use ReDispatchChannels (on which I'm working on atm) to pass the channel to Empathy when needed.

If there are things blocking you from using tp-glib please let us and we'll fix it. :)
Comment 16 Dan Winship 2011-01-27 14:44:56 UTC
(In reply to comment #15)
> It will also allow us to improve the existing Telepathy integration; for
> example the Shell should be an Approver and a Handler so Empathy would stock
> blicking when the channel is displayed by the shell.

Right, but it can do that directly via D-Bus as well.

> If there are things blocking you from using tp-glib please let us and we'll fix
> it. :)

No, it's just time. It would take less time to implement the few remaining 3.0 features via the D-Bus interfaces than it would to port everything to tp-glib and then implement the new features on top of that.

Of course, now Morten is talking about updating the tp-glib-port patch himself, so maybe we can end up doing both.
Comment 17 Guillaume Desmottes 2011-01-28 15:21:23 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > It will also allow us to improve the existing Telepathy integration; for
> > example the Shell should be an Approver and a Handler so Empathy would stock
> > blicking when the channel is displayed by the shell.
> 
> Right, but it can do that directly via D-Bus as well.

In theory yes, but that would be a pain.

I'd be interested working on improving Telepathy integration into the shell but I won't do it without using tp-glib. I already wrote this code once, I'd prefer re-using it rather than rewriting it a second time. :)
Comment 18 Guillaume Desmottes 2011-02-07 16:58:47 UTC
I'm working on this.
Comment 19 Guillaume Desmottes 2011-02-09 10:48:24 UTC
So, I redid this based on master. I reused some of Morten's patches, but redid most of the changes myself as they had bitrotten. I also made more atomic commits. This is my first time with javascript so let me know if I did some things wrong.
http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/tp-glib-620416
Comment 20 Guillaume Desmottes 2011-02-09 10:49:34 UTC
Created attachment 180449 [details] [review]
Squashed diff

 configure.ac                    |    4 +-
 js/Makefile.am                  |    1 -
 js/misc/telepathy.js            |  361 ------------------------
 js/ui/telepathyClient.js        |  588 +++++++++------------------------------
 src/Makefile.am                 |    1 +
 src/shell-global.c              |   69 +++++
 src/shell-global.h              |   16 +
 tools/build/gnome-shell.modules |   10 +
 8 files changed, 230 insertions(+), 820 deletions(-)
Comment 21 Dan Winship 2011-02-09 16:47:27 UTC
Comment on attachment 180449 [details] [review]
Squashed diff

OK, this looks great and is almost ready to go in. Thanks to both of you (Guillaume and Morten).

I think we'll want to commit the atomic commits rather than the big squashed one. The commit summaries should mostly all be "telepathyClient: whatever", for consistency with other commits / to make it clearer what they're about. And each should have the URL of this bug as the last line of the commit message.

(Comments are a bit brusque due to the length of the review. Sorry :)


a7e1b04 Add telepathy-glib as a dependency

>+++ b/src/Makefile.am
> 		--include=Meta-2.91			\
>++		--include=TelepathyGLib-0.12 \
>  		--libtool="$(LIBTOOL)"			\

There is a stray "+" at the start of that line (apparently ignored by g-ir-scanner?), and the backslash is not aligned correctly



827c80e Wrap tp_connection_get_contacts_by_handle

>+/**
>+ * shell_global_get_tp_contacts_cb: (skip)
>+ */
>+static void

The annotation is unnecessary, since this is a static function.

>+shell_global_get_tp_contacts_cb (TpConnection *self,
>+shell_global_get_tp_contacts (TpConnection *self,

These should be "shell_get_...", not "shell_global_get_...". (They don't take the ShellGlobal object as an argument, and they have no need to.) They can still stay in shell-global.c.

shell(_global)_get_tp_contacts is ignoring its user_data and GDestroyNotify, which may result in gjs leaking things. You need to either use them correctly, or else remove the destroy notify and mark @callback '(scope async)'. Also, @weak_object seems unused and should be removed.



df57341 remove unused channel class definitions

Those definitions are still used at that point. This should be squashed with the following patch.



eda3d2e Use TpSimpleObserver rather than our own Observer implementation

> const _ = Gettext.gettext;
>+const Tp = imports.gi.TelepathyGLib;

Our import-sorting rules say that Gettext comes last because it's weird, so the Tp import should be immediately after St.

>+        let dbus = Tp.DBusDaemon.dup();

You should not need to explicitly call dup() on that. What's going on there?

>+        this.observer = Tp.SimpleObserver.new(dbus, true, "GnomeShell", true,

"this.observer" should be "this._observer" with an underscore, because it's private. And "GnomeShell" should be in single quotes, not double quotes. (We use single quotes everywhere except for translated strings, because xgettext doesn't recognize single quotes there.)

(Looking at the final telepathyClient.js, I see there are lots of other ""s that should be ''s that I missed in this review. So... fix those too.)

>+        // We only care about single-user text-based chats

Do the considerations about HandleType.NONE from the old code no longer apply?

>+    _observeChannels: function(observer, account, conn, channels,
>+                               dispatch_op, requests, context, user_data) {

There's no user_data in gjs callbacks; it's assumed that "this" contains everything you need (although if you need more, you can pass extra arguments to Lang.bind, and then those arguments will get appended to the normal callback arguments).

(Also, "dispatchOp", not "dispatch_op".)



> 4e000a3 stop looking for existing channels manually

This should either be squashed with the TpSimpleObserver patch, or else you should pass false for "recover" in the original patch and then change it here, so we're not redundant in the meantime.



1354c15 Source: store proper Telepathy objects

>     _removeConnection: function(conn) {
>-        let info = this._connections[conn.getPath()];
>+        let info = this._connections[conn.get_object_path()];

Pretty sure this (and the other change at the end of this method) don't belong with this patch, since _removeConnection is still being called with a gjs dbus object.



7b027b2 use TpAccountChannelRequest to re-ensure the channel

>+        req.ensure_channel_async("", null, null, this);

Does gjs even allow you to pass null for the callback when it's not annotated allow-none?

Either way, there is no user_data arg in js, so remove the ", this" from the end.

Also, this leaves _gotChannelRequest() around but unused. (ie, squash 68bb5d7 with this)



d8b35d6 remove channelDispatcher

squash with previous



aa3413e pass a TpContact object to Source

>+const Shell = imports.gi.Shell;

should be alphabetized

>+let contact_features = [Tp.ContactFeature.ALIAS,
>+                        Tp.ContactFeature.AVATAR_DATA,
>+                        Tp.ContactFeature.PRESENCE]

camelCase for variable names. Also, missing ";" at the end.

>+            Shell.Global.get_tp_contacts(conn, 1, [targetHandle],
>+                    contact_features.length, contact_features,
>+                    Lang.bind(this,  function(connection, contacts, failed) {

For very long functions, it's better to just put them into a private method, and then just pass "Lang.bind(this, this._myCallbackMethod)". It can be confusing to have a big block of code, followed by other code which actually runs before it, not after it, especially if your window is scrolled so that you don't see the part that's making the async call.

If you need to pass local variables from the calling function to the callback, you can either add them to the end of the Lang.bind call (in which case they'll get added to the end of the callback args) or else just do a small inline callback that just calls the callback method with the args you want to pass it.

(Also also, there's an extra space before the word "function" there.)

>+    _got_contact: function(connection, contacts, failed) {

Aha. Appears to be leftover from your initial attempt to do it as suggested above? :). Should be camelCase.



443d74c ensure that we just observe contact text channels
96639d3 get alias from TpContact

could be squashed with previous



c6ffaad use TpContact to get the avatar

>+    _update_avatar_icon: function() {

camelCase

>+            this._iconBox.child = new St.Icon({ icon_name: 'stock_person',
>+                                          icon_type: St.IconType.FULLCOLOR,
>+                                          icon_size: this._iconBox._size });

the 2nd and 3rd lines should be aligned with "icon_name"



c706291 connect on avatar and alias changes

could also split this patch and squash the two parts into 96639d3 and c6ffaad

>+        this._update_alias()

missing ";"

>+        this._contact.connect("notify::avatar-file", Lang.bind(this, this._update_avatar_icon));
>+        this._contact.connect("notify::alias", Lang.bind(this, this._update_alias));

The contact is likely to outlast the Source, so you need to store the signal handler IDs and disconnect from them in Source.destroy. (Look at Source.notify for an example of how to override a parent class method.)

Actually, hm. The existing code is broken. _channelClosed() should just call this.destroy(), and the disconnections there should be in destroy().

(See further discussion of GC-vs-signal-handlers below at 58504c2.)


>+    _update_alias: function() {

camelCase




14b3dc3 get presence changes from TpContact

>         this._contact.connect("notify::alias", Lang.bind(this, this._update_alias));
>+        this._presenceChangedId = contact.connect('presence-changed', Lang.bind(this, this._presenceChanged));

here you're saving the handler ID, but you still need to add the code to disconnect on destroy. Also, you should be consistent about whether you say "this._contact.connect" or just "contact.connect".

Removing _presenceChanged() leaves an erroneous trailing "," after _got_contact(). (The last function definition in the prototype needs to NOT have a "," after it. Yup, it's annoying.)



47b042a use TpTextChannel

>+        this._displayPendingMessages()

missing ";"

>+        this._channel.send_message_async(msg, 0, null, null);

can't figure out what tp-glib method that corresponds to, but if the second "null" is a user_data, then remove it.



faf3398 use tp_message_to_text()

squash with above



7d8c30d use tp_channel_get_handle()
ec28b3b remove unused subscribedContactsChannel
ebd459c more use of tp_channel_get_handle()
8d901e9 no need to get handle in Source's init function
49c3fbc coding style fix

Please squash to appropriate previous patches



159ecdd check if channel is actually a TpTextChannel

Likewise please squash, but also

>+            if (!chan instanceof Tp.TextChannel ||

I think the order of operations is wrong there. Need "!(chan instanceof Tp.TextChannel)"



179db85 factor out _create_source

>+    _create_source: function(account, conn, channel, contact) {

camelCase



58504c2 No need to disconnect signals on the channel

> We are owning the only ref on it.

Do not meddle in the affairs of garbage collectors. It's best to assume that the channel could still be referenced even after the Source is destroyed. (Perhaps by a stray function closure that can't be GCed yet.) However, if there's something you can do to the channel to ensure that it won't emit any further signals (a la gtk_widget_destroy()), then you could do that, and then not need to disconnect (but you should get rid of sentId and receivedId entirely in that case).
Comment 22 Guillaume Desmottes 2011-02-10 14:21:03 UTC
(In reply to comment #21)
> (From update of attachment 180449 [details] [review])
> I think we'll want to commit the atomic commits rather than the big squashed
> one. The commit summaries should mostly all be "telepathyClient: whatever", for
> consistency with other commits / to make it clearer what they're about. And
> each should have the URL of this bug as the last line of the commit message.

Ok, I'll do some squashing/amending once you'll have reviewed everything (will
be easier for you to just review the new commits).

> (Comments are a bit brusque due to the length of the review. Sorry :)

Np, I'm used to code review. :)

> a7e1b04 Add telepathy-glib as a dependency
> 
> >+++ b/src/Makefile.am
> > 		--include=Meta-2.91			\
> >++		--include=TelepathyGLib-0.12 \
> >  		--libtool="$(LIBTOOL)"			\
> 
> There is a stray "+" at the start of that line (apparently ignored by
> g-ir-scanner?), and the backslash is not aligned correctly

Ooops, fixed (squashed).

> 827c80e Wrap tp_connection_get_contacts_by_handle
> 
> >+/**
> >+ * shell_global_get_tp_contacts_cb: (skip)
> >+ */
> >+static void
> 
> The annotation is unnecessary, since this is a static function.

removed (squashed).

> >+shell_global_get_tp_contacts_cb (TpConnection *self,
> >+shell_global_get_tp_contacts (TpConnection *self,
> 
> These should be "shell_get_...", not "shell_global_get_...". (They don't take
> the ShellGlobal object as an argument, and they have no need to.) They can
> still stay in shell-global.c.

renamed.

> shell(_global)_get_tp_contacts is ignoring its user_data and GDestroyNotify,
> which may result in gjs leaking things. You need to either use them correctly,
> or else remove the destroy notify and mark @callback '(scope async)'. Also,
> @weak_object seems unused and should be removed.

I removed them all.

> df57341 remove unused channel class definitions
> 
> Those definitions are still used at that point. This should be squashed with
> the following patch.

squashed

> eda3d2e Use TpSimpleObserver rather than our own Observer implementation
> 
> > const _ = Gettext.gettext;
> >+const Tp = imports.gi.TelepathyGLib;
> 
> Our import-sorting rules say that Gettext comes last because it's weird, so the
> Tp import should be immediately after St.

Moved.

> >+        let dbus = Tp.DBusDaemon.dup();
> 
> You should not need to explicitly call dup() on that. What's going on there?

See http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-dbus.html#tp-dbus-daemon-dup
that's a conveninent function to ensure to have a singleton and so share the
same TpDBusDaemon in all the process.

> >+        this.observer = Tp.SimpleObserver.new(dbus, true, "GnomeShell", true,
> 
> "this.observer" should be "this._observer" with an underscore, because it's
> private. And "GnomeShell" should be in single quotes, not double quotes. (We
> use single quotes everywhere except for translated strings, because xgettext
> doesn't recognize single quotes there.)

done.

> (Looking at the final telepathyClient.js, I see there are lots of other ""s
> that should be ''s that I missed in this review. So... fix those too.)

done.

> >+        // We only care about single-user text-based chats
> 
> Do the considerations about HandleType.NONE from the old code no longer apply?

No, we don't have those any more.

> >+    _observeChannels: function(observer, account, conn, channels,
> >+                               dispatch_op, requests, context, user_data) {
> 
> There's no user_data in gjs callbacks; it's assumed that "this" contains
> everything you need (although if you need more, you can pass extra arguments to
> Lang.bind, and then those arguments will get appended to the normal callback
> arguments).

removed.
> (Also, "dispatchOp", not "dispatch_op".)

renamed.

> > 4e000a3 stop looking for existing channels manually
> 
> This should either be squashed with the TpSimpleObserver patch, or else you
> should pass false for "recover" in the original patch and then change it here,
> so we're not redundant in the meantime.

I did the recover=false -> true dance.

> 1354c15 Source: store proper Telepathy objects
> 
> >     _removeConnection: function(conn) {
> >-        let info = this._connections[conn.getPath()];
> >+        let info = this._connections[conn.get_object_path()];
> 
> Pretty sure this (and the other change at the end of this method) don't belong
> with this patch, since _removeConnection is still being called with a gjs dbus
> object.

Right, fixed.

> 7b027b2 use TpAccountChannelRequest to re-ensure the channel
> 
> >+        req.ensure_channel_async("", null, null, this);
> 
> Does gjs even allow you to pass null for the callback when it's not annotated
> allow-none?

It seems it does.

> Either way, there is no user_data arg in js, so remove the ", this" from the
> end.

removed.

> Also, this leaves _gotChannelRequest() around but unused. (ie, squash 68bb5d7
> with this)

done.

> d8b35d6 remove channelDispatcher

> squash with previous

done

> aa3413e pass a TpContact object to Source
> 
> >+const Shell = imports.gi.Shell;
> 
> should be alphabetized

done.

> >+let contact_features = [Tp.ContactFeature.ALIAS,
> >+                        Tp.ContactFeature.AVATAR_DATA,
> >+                        Tp.ContactFeature.PRESENCE]
> 
> camelCase for variable names. Also, missing ";" at the end.

fixed.

> >+            Shell.Global.get_tp_contacts(conn, 1, [targetHandle],
> >+                    contact_features.length, contact_features,
> >+                    Lang.bind(this,  function(connection, contacts, failed) {
> 
> For very long functions, it's better to just put them into a private method,
> and then just pass "Lang.bind(this, this._myCallbackMethod)". It can be
> confusing to have a big block of code, followed by other code which actually
> runs before it, not after it, especially if your window is scrolled so that you
> don't see the part that's making the async call.
> 
> If you need to pass local variables from the calling function to the callback,
> you can either add them to the end of the Lang.bind call (in which case they'll
> get added to the end of the callback args) or else just do a small inline
> callback that just calls the callback method with the args you want to pass it.

I agree that's it too long, that's why I factor out _create_source()
afterward. I keep it as it as the final code is right and I'm getting tired of
rebasing/ammending/fixing conflicts.

> (Also also, there's an extra space before the word "function" there.)

fixed.

> >+    _got_contact: function(connection, contacts, failed) {
> 
> Aha. Appears to be leftover from your initial attempt to do it as suggested
> above? :). Should be camelCase.

removed.

> 443d74c ensure that we just observe contact text channels
> 96639d3 get alias from TpContact
> 
> could be squashed with previous

done.

> c6ffaad use TpContact to get the avatar
> 
> >+    _update_avatar_icon: function() {
> 
> camelCase

changed.

> >+            this._iconBox.child = new St.Icon({ icon_name: 'stock_person',
> >+                                          icon_type: St.IconType.FULLCOLOR,
> >+                                          icon_size: this._iconBox._size });
> 
> the 2nd and 3rd lines should be aligned with "icon_name"

I hate this coding style, that's the perfect way to waste loads of space;
changed.

> c706291 connect on avatar and alias changes
> 
> could also split this patch and squash the two parts into 96639d3 and c6ffaad

done.

> >+        this._update_alias()
> 
> missing ";"

fixed; why doesn't js complain about that?

> >+        this._contact.connect("notify::avatar-file", Lang.bind(this, this._update_avatar_icon));
> >+        this._contact.connect("notify::alias", Lang.bind(this, this._update_alias));
> 
> The contact is likely to outlast the Source, so you need to store the signal
> handler IDs and disconnect from them in Source.destroy. (Look at Source.notify
> for an example of how to override a parent class method.)
> 
> Actually, hm. The existing code is broken. _channelClosed() should just call
> this.destroy(), and the disconnections there should be in destroy().

I tried doing that:

    destroy: function() {
        MessageTray.Source.prototype.destroy.call(this);

        this._channel.disconnect(this._closedId);
        this._channel.disconnect(this._receivedId);
        this._channel.disconnect(this._sentId);
    },

but this function was called twice for some reason, raising such errors:
  gsignal.c:2392: instance `0x1aa0170' has no handler with id `3907'

So I kept the disconnecing in the closed callback for now.
Btw, you should *really* have an equivalent of http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-util.html#tp-g-signal-connect-object
it makes life much easier.

> (See further discussion of GC-vs-signal-handlers below at 58504c2.)
> 
> 
> >+    _update_alias: function() {
> 
> camelCase

fixed.

> 14b3dc3 get presence changes from TpContact
> 
> >         this._contact.connect("notify::alias", Lang.bind(this, this._update_alias));
> >+        this._presenceChangedId = contact.connect('presence-changed', Lang.bind(this, this._presenceChanged));
> 
> here you're saving the handler ID, but you still need to add the code to
> disconnect on destroy. Also, you should be consistent about whether you say
> "this._contact.connect" or just "contact.connect".

fixed.

> Removing _presenceChanged() leaves an erroneous trailing "," after
> _got_contact(). (The last function definition in the prototype needs to NOT
> have a "," after it. Yup, it's annoying.)

WTF, this is really retarded... Why isn't js complaining then?
Anyway, fixed.

> 47b042a use TpTextChannel
> 
> >+        this._displayPendingMessages()
> 
> missing ";"

added.

> >+        this._channel.send_message_async(msg, 0, null, null);
> 
> can't figure out what tp-glib method that corresponds to, but if the second
> "null" is a user_data, then remove it.

tp_text_channel_send_message_async()
removed.

> faf3398 use tp_message_to_text()
> 
> squash with above

done.

> 7d8c30d use tp_channel_get_handle()
> ec28b3b remove unused subscribedContactsChannel
> ebd459c more use of tp_channel_get_handle()
> 8d901e9 no need to get handle in Source's init function
> 49c3fbc coding style fix
> 
> Please squash to appropriate previous patches

done.

> 159ecdd check if channel is actually a TpTextChannel
> 
> Likewise please squash, but also
> 
> >+            if (!chan instanceof Tp.TextChannel ||
>
> I think the order of operations is wrong there. Need "!(chan instanceof
> Tp.TextChannel)"

fixed

> 179db85 factor out _create_source
> 
> >+    _create_source: function(account, conn, channel, contact) {
> 
> camelCase

changed

> 58504c2 No need to disconnect signals on the channel
> 
> > We are owning the only ref on it.
> 
> Do not meddle in the affairs of garbage collectors. It's best to assume that
> the channel could still be referenced even after the Source is destroyed.
> (Perhaps by a stray function closure that can't be GCed yet.) However, if
> there's something you can do to the channel to ensure that it won't emit any
> further signals (a la gtk_widget_destroy()), then you could do that, and then
> not need to disconnect (but you should get rid of sentId and receivedId
> entirely in that case).

Ok I removed this commit.
Comment 23 Guillaume Desmottes 2011-02-10 14:21:54 UTC
Created attachment 180581 [details] [review]
new diff

 configure.ac                    |    4 +-
 js/Makefile.am                  |    1 -
 js/misc/telepathy.js            |  361 -----------------------
 js/ui/telepathyClient.js        |  596 +++++++++------------------------------
 src/Makefile.am                 |    1 +
 src/shell-global.c              |   60 ++++
 src/shell-global.h              |   13 +
 tools/build/gnome-shell.modules |   10 +
 8 files changed, 224 insertions(+), 822 deletions(-)
Comment 24 Dan Winship 2011-02-10 15:37:25 UTC
(In reply to comment #22)
> > >+        let dbus = Tp.DBusDaemon.dup();
> > 
> > You should not need to explicitly call dup() on that. What's going on there?
> 
> See
> http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-dbus.html#tp-dbus-daemon-dup

Ah... "tp_dbus_daemon_get_default()" would be a more glib-esque name for that.

> I agree that's it too long, that's why I factor out _create_source()
> afterward. I keep it as it as the final code is right and I'm getting tired of
> rebasing/ammending/fixing conflicts.

ah, didn't notice that. That's fine.

> > missing ";"
> 
> fixed; why doesn't js complain about that?

It's syntactically correct to omit the semicolon, but it's "considered harmful" because there are a small number of cases where leaving them out can cause the code to be parsed other than how you intended. So, while gjs won't complain about it, emacs js2-mode (and various other editors) will.

> but this function was called twice for some reason, raising such errors:
>   gsignal.c:2392: instance `0x1aa0170' has no handler with id `3907'
> 
> So I kept the disconnecing in the closed callback for now.

OK. I'll look into that.

> Btw, you should *really* have an equivalent of
> http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-util.html#tp-g-signal-connect-object
> it makes life much easier.

Yeah, we occasionally talk about simplifying signal connection, but we've never gotten around to it.

> > Removing _presenceChanged() leaves an erroneous trailing "," after
> > _got_contact(). (The last function definition in the prototype needs to NOT
> > have a "," after it. Yup, it's annoying.)
> 
> WTF, this is really retarded...

keep in mind that the prototype definition is actually just a very large object literal, so it's the same as "{ a: 1, b: 2 }" vs "{ a: 1, b: 2, }".

> Why isn't js complaining then?

Netscape-derived JS interpreters accept trailing commas in object/array literals, but it's wrong according to the ECMAScript spec, so it might cause problems if we switched from gjs to seed some day.
Comment 25 Guillaume Desmottes 2011-02-15 14:11:39 UTC
Any chance to get this merged soon? I spent lot of time on this branch so I'd rather not see it bit rotting again.
Comment 26 Dan Winship 2011-02-15 16:25:06 UTC
Comment on attachment 180581 [details] [review]
new diff

ok, marking this commit-now, but I really mean to merge the branch, not the squashed commit.

we generally like our merges "flat", so please rebase to master before merging (and make sure the two telepathyClient.js commits since then get merged in correctly).
Comment 27 Guillaume Desmottes 2011-02-16 09:15:40 UTC
Merged to master; thanks for the review.

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.