Created attachment 186669 [details] [review]
patch to add pidgin integration extension

Created attachment 186670 [details] [review]
patch to add pidgin integration extension

shouldnt add TODO.txt , and stylesheet.css should be empty

Review of attachment 186669 [details] [review]:

::: configure.ac
@@ +73,3 @@
   extensions/xrandr-indicator/Makefile
   extensions/user-theme/Makefile
+  extensions/pidgin/Makefile

Please keep the list sorted alphabetically.

::: extensions/pidgin/TODO.txt
@@ +1,3 @@
+Known bugs
+===========
+

Is this empty file needed?

::: extensions/pidgin/extension.js
@@ +10,3 @@
+
+const DBus = imports.dbus;
+const Gettext = imports.gettext.domain('gnome-shell');

Gettext import goes at the just before _, and the right domain is 'gnome-shell-extensions'.

@@ +15,3 @@
+const Signals = imports.signals;
+const St = imports.gi.St;
+const Tp = imports.gi.TelepathyGLib;

I understand reusing TelepathyClient code, but is it necessary to import TelepathyGLib?

@@ +56,3 @@
+        styles.push(message.direction);
+
+        if (message.messageType == Tp.ChannelTextMessageType.ACTION) {

There doesn't seem anything generating these kind of messages.

@@ +92,3 @@
+        this._initialMessage = initialMessage;
+        // FIXME: get some sort of default icon properly, not this hardcoded
+        // path

Can you do with avatar-default?

@@ +100,3 @@
+
+        if (!this._initialMessage) {
+            let history = proxy.PurpleConversationGetMessageHistorySync(this._conversation);

(Here and in other places). Is it absolutely necessary to use synchronous methods? Gnome Shell is a compositor, its main loop must be always free to run drawing code, or the whole experience will be severely degraded.

@@ +125,3 @@
+        }
+
+        if (!Main.messageTray.contains(this))

Can it happen that the source is initialized more than once (or added outside _init)?

@@ +138,3 @@
+
+    destroy: function () {
+        let proxy = this._client.proxy();

Property access, not function call here.

@@ +144,3 @@
+        proxy.disconnect(this._messageSentId);
+        proxy.disconnect(this._messageReceivedId);
+        MessageTray.Source.prototype.destroy.call(this);

destroy has a parameter (reason).

@@ +193,3 @@
+
+        if (new_status == 'dnd') new_status = 'busy';
+        this._notification.appendPresence('<i>' + this.title + ' is now ' + new_status + '</i>', false);

Concatenating strings won't work with translations. Use a switch statement and printf-like strings (eg. "%s is now busy", "%s is now online").
Markup, on the other hand, must be kept outside translatable strings. Also try to share strings with gajim extension if possible.

@@ +201,3 @@
+
+        this._presence = 'offline';
+        this._notification.appendPresence('<i>' + this.title + ' have signed off</i>', false);

"%s has signed off", or even better "%s is offline" (which is what telepathy and gajim show)

@@ +369,3 @@
+
+
+function main() {

You need "imports.gettext.bindtextdomain('gnome-shell-extensions', metadata.localedir)", or translations won't work.

::: extensions/pidgin/stylesheet.css
@@ +1,1 @@
+/* Example stylesheet */

You don't need this.

Review of attachment 186670 [details] [review]:

The subject should start with an action: "Add Pidgin integration extension"

This is supplemental to Giovanni's review.

::: extensions/pidgin/extension.js
@@ +18,3 @@
+
+const Main = imports.ui.main;
+const Mainloop = imports.mainloop;

Mainloop and Signals imports aren't used.

@@ +25,3 @@
+const _ = Gettext.gettext;
+
+const DEBUG=true;

Take this, and the usage of it out.

@@ +27,3 @@
+const DEBUG=true;
+
+function wrappedText(text, sender, timestamp, direction) {

You never use 'timestamp' with a valid value.

See http://pidgin.im/pipermail/support/2011-January/009456.html for some information on how to get it.

@@ +90,3 @@
+        this._account = account;
+        this._conversation = conversation;
+        this._initialMessage = initialMessage;

Any reason this is an instance property? It doesn't seem to be used after the constructor.

@@ +186,3 @@
+
+        // XXX: this looks wrong. should get string?
+        let old_status = proxy.PurpleStatusGetIdSync(old_status_id);

You don't use old_status.

@@ +325,3 @@
+        this._proxy = new Pidgin(DBus.session, 'im.pidgin.purple.PurpleService', '/im/pidgin/purple/PurpleObject');
+        patchSynchronousMethods(this._proxy, PidginIface);
+        this._proxy.connect('ReceivedImMsg', Lang.bind(this, this._messageReceived));

Is there any reason you need this signal handler? Shouldn't conversationCreated create the source with no initial message, which will look through the history and display the most recent message?

See: http://tech.shantanugoel.com/2008/09/23/tip-a-post-for-pidgin-plugin-developers.html

(In reply to comment #3)
> @@ +92,3 @@
> +        this._initialMessage = initialMessage;
> +        // FIXME: get some sort of default icon properly, not this hardcoded
> +        // path
> 
> Can you do with avatar-default?

what functions i can call to get icon path of current theme?

> 
> @@ +100,3 @@
> +
> +        if (!this._initialMessage) {
> +            let history =
> proxy.PurpleConversationGetMessageHistorySync(this._conversation);
> 
> (Here and in other places). Is it absolutely necessary to use synchronous
> methods? Gnome Shell is a compositor, its main loop must be always free to run
> drawing code, or the whole experience will be severely degraded.

I'm not sure how to keep the code easily readable with async, my original attempt ended up with a huge callback mess due to the need to execute many dbus calls to convert object ref to string/integer values (such as title, icon path, name, status id). any suggestions?

> 
> @@ +138,3 @@
> +
> +    destroy: function () {
> +        let proxy = this._client.proxy();
> 
> Property access, not function call here.

gajim extension also uses _client.proxy();

> 
> @@ +144,3 @@
> +        proxy.disconnect(this._messageSentId);
> +        proxy.disconnect(this._messageReceivedId);
> +        MessageTray.Source.prototype.destroy.call(this);
> 
> destroy has a parameter (reason).
> 

javascript's call() takes current object as first parameter,  followed with the parameters to the destroy function. Gajim extension also uses this.


cleaning up to use DisplayedImMsg instead of ReceivedImMsg/SentImMsg .. was notified that that signal works better.

(In reply to comment #5)
> (In reply to comment #3)
> > @@ +92,3 @@
> > +        this._initialMessage = initialMessage;
> > +        // FIXME: get some sort of default icon properly, not this hardcoded
> > +        // path
> > 
> > Can you do with avatar-default?
> 
> what functions i can call to get icon path of current theme?

eh.. just realized that createNotificationIcon i copied from gajim already did that >.<

Created attachment 186709 [details] [review]
use DisplayedImMsg instead of ReceivedImMsg/SentImMsg and use translation for presence

Created attachment 186711 [details] [review]
removed unknown presence id logging

Can you use something like git rebase -i to squash these three patches into one big one, and then upload that?

Created attachment 186714 [details] [review]
patch to add pidgin integration extension

DisplayedImMsg reduced the need to call dbus methods frequently .. updated to use only async calls, and removed the patchSynchronousMethods function

I just tried the patch and got few whitespace issues:

$ git apply 0001-added-pidgin-integration-extension.patch
0001-added-pidgin-integration-extension.patch:218: trailing whitespace.
    
0001-added-pidgin-integration-extension.patch:266: trailing whitespace.
    
0001-added-pidgin-integration-extension.patch:278: trailing whitespace.
    
0001-added-pidgin-integration-extension.patch:280: trailing whitespace.
    
0001-added-pidgin-integration-extension.patch:283: trailing whitespace.
    
warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.

Created attachment 186795 [details] [review]
patch to add pidgin integration extension

removed whitespaces

notification not wont notify for sent messages, except if the sent message is first message in conversation.

Two more things I found meanwhile:

1. In the bubble that you get when you click on the contact icon in the message tray you can only scroll upwards with the mouse wheel, but not down again.

2. The header with contact icon and name in the same bubble is scrolled too and runs out of sight, whereas in the empathy bubbles the contact icon and name remains always visible on top.

(In reply to comment #13)
> Two more things I found meanwhile:
> 
> 1. In the bubble that you get when you click on the contact icon in the message
> tray you can only scroll upwards with the mouse wheel, but not down again.
> 
> 2. The header with contact icon and name in the same bubble is scrolled too and
> runs out of sight, whereas in the empathy bubbles the contact icon and name
> remains always visible on top.

And a third:
3. The input field also scrolls away, as opposed to the the one in empathy's bubble.

hi ..

been trying to debug that, but i think i'm stuck.

The notification class is inherited directly from TelepathyClient.Notification, the only override is its appendMessage method. 

I even tried using TelepathyClient.Notification class directly yet the bubble still scrolls the whole titlebar, body, and action input box instead of just the body. And that not the same as how TelepathyClient.Notification behaves with empathy source.

I think i'm kindof lost now .. anybody have clues? .. I think it might have been the CSS, but I dont know how to check what the class is.

Is theres anything like firebug where i can hover on a shell element and get its css class?

This extension is on extensions.gnome.org nowadays[0], so closing.

[0] https://extensions.gnome.org/extension/214/pidgin-conversation-integration/