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 653941 - Be able to accept/decline subscription request
Be able to accept/decline subscription request
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Jasper St. Pierre (not reading bugmail)
gnome-shell-maint
Depends on: 645978 652753
Blocks: 654159
 
 
Reported: 2011-07-04 11:31 UTC by Guillaume Desmottes
Modified: 2011-08-29 14:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add IM subscription request support (8.95 KB, patch)
2011-08-23 13:24 UTC, Xavier Claessens
none Details | Review
Monitor account/contact changes to catch incoming subscription requests (5.21 KB, patch)
2011-08-23 13:25 UTC, Xavier Claessens
none Details | Review
Add IM subscription request support (10.64 KB, patch)
2011-08-23 13:31 UTC, Xavier Claessens
none Details | Review
Add IM subscription request support (10.72 KB, patch)
2011-08-23 15:24 UTC, Xavier Claessens
needs-work Details | Review
Add IM subscription request support (10.46 KB, patch)
2011-08-24 12:17 UTC, Xavier Claessens
accepted-commit_now Details | Review
telepathyClient: Add IM subscription request support (10.72 KB, patch)
2011-08-26 10:38 UTC, Xavier Claessens
none Details | Review
telepathyClient: Add IM subscription request support (15.00 KB, patch)
2011-08-28 00:11 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review

Description Guillaume Desmottes 2011-07-04 11:31:07 UTC
As part of bug #653938 the Shell should be able to accept and decline subscription requests.
Comment 1 Guillaume Desmottes 2011-07-04 11:34:08 UTC
This is blocked by https://bugs.freedesktop.org/show_bug.cgi?id=26205
Comment 2 Xavier Claessens 2011-08-23 13:24:54 UTC
Created attachment 194480 [details] [review]
Add IM subscription request support
Comment 3 Xavier Claessens 2011-08-23 13:25:21 UTC
Created attachment 194481 [details] [review]
Monitor account/contact changes to catch incoming subscription requests
Comment 4 Xavier Claessens 2011-08-23 13:31:55 UTC
Created attachment 194482 [details] [review]
Add IM subscription request support

Based on initial work from Guillaume Desmottes
Comment 5 Xavier Claessens 2011-08-23 15:24:19 UTC
Created attachment 194490 [details] [review]
Add IM subscription request support

Based on initial work from Guillaume Desmottes
Comment 6 Jasper St. Pierre (not reading bugmail) 2011-08-23 16:00:50 UTC
Review of attachment 194490 [details] [review]:

::: js/ui/telepathyClient.js
@@ +215,3 @@
+
+    _accountManagerPrepared: function(am, result)
+    {

Watch your braces.

@@ +230,3 @@
+
+        account.connect('notify::connection',
+            Lang.bind(this, this._connectionChanged));

I don't understand why you connect every time the validity changes instead of just once.

@@ +241,3 @@
+
+        conn.connect('notify::contact-list-state',
+            Lang.bind(this, this._contactListStateChanged));

s/validity/connection/

@@ +251,3 @@
+            return;
+
+        conn.connect('contact-list-changed', Lang.bind(this, this._contactListChanged));

s/validity/list state/

@@ +260,3 @@
+            let contact = added[j];
+            contact.connect('subscription-states-changed',
+                Lang.bind(this, this._subscriptionStateChanged))

Are we guaranteed that we've never seen this contact before? If so, it's fine. Otherwise, s/validity/contact list/.

@@ +298,3 @@
+
+        return this._subscriptionSource;
+    },

No comma.

@@ +808,3 @@
+        this._nbNotifications += 1;
+
+        // Display the source while there is at least one notification

I don't understand this. Why?

@@ +836,3 @@
+             * the contact's alias and the second one is the
+             * file name. The string will be something
+             * like: "Alice is sending you test.ogg"

This isn't the string.

@@ +846,3 @@
+
+        // Display avatar
+        let iconBox = new St.Bin({ style_class: 'avatar-box' });

Any reason you don't just fill in the "icon"/"body" instead of making your layout?

@@ +912,3 @@
+        // answered
+        if (publish != Tp.SubscriptionState.ASK)
+          this.destroy();

nit: 4 space indent always, regardless of the presence of braces.
Comment 7 Xavier Claessens 2011-08-24 12:16:03 UTC
Review of attachment 194490 [details] [review]:

::: js/ui/telepathyClient.js
@@ +215,3 @@
+
+    _accountManagerPrepared: function(am, result)
+    {

fixed.

@@ +230,3 @@
+
+        account.connect('notify::connection',
+            Lang.bind(this, this._connectionChanged));

When account becomes not valid, it is destroyed anyway. So account should becomes valid only once.

How would you do to connect that signal only once per account? Keep a list of valid accounts on this and connect the signal only if the account is not yet in that list?

@@ +241,3 @@
+
+        conn.connect('notify::contact-list-state',
+            Lang.bind(this, this._contactListStateChanged));

This signal happens when account goes online/offline. Each time that would be another connection object.

@@ +251,3 @@
+            return;
+
+        conn.connect('contact-list-changed', Lang.bind(this, this._contactListChanged));

state goes to SUCCESS only once

@@ +260,3 @@
+            let contact = added[j];
+            contact.connect('subscription-states-changed',
+                Lang.bind(this, this._subscriptionStateChanged))

yes, a same contact object is added only once. If you add a contact then remove it and add again, that would give you another contact object afaik.

@@ +298,3 @@
+
+        return this._subscriptionSource;
+    },

fixed

@@ +808,3 @@
+        this._nbNotifications += 1;
+
+        // Display the source while there is at least one notification

We can have more than one contact asking for authorization. In that case we stack the notifications into the same source, and we remove the source when all are replied.

@@ +836,3 @@
+             * the contact's alias and the second one is the
+             * file name. The string will be something
+             * like: "Alice is sending you test.ogg"

Oops wrong copy/paste in Guillaume's code it seems. Fixed.

@@ +846,3 @@
+
+        // Display avatar
+        let iconBox = new St.Bin({ style_class: 'avatar-box' });

What do you mean? The UI part was made by Guillaume, I don't know much (yet) about JS/gnome-shell :p

@@ +912,3 @@
+        // answered
+        if (publish != Tp.SubscriptionState.ASK)
+          this.destroy();

fixed.
Comment 8 Xavier Claessens 2011-08-24 12:17:14 UTC
Created attachment 194589 [details] [review]
Add IM subscription request support

Based on initial work from Guillaume Desmottes
Comment 9 Jasper St. Pierre (not reading bugmail) 2011-08-25 17:03:54 UTC
Review of attachment 194589 [details] [review]:

Looks fine. cassidy said that using the icon parameter "looked wrong". I can look into that soon, but for now, landing this is important.
Comment 10 Xavier Claessens 2011-08-26 10:32:10 UTC
Unfortunately this is blocked bug bug #645978: gjs can't emit signal contact-list-changed because it does not support GPtrArray in signals.
Comment 11 Xavier Claessens 2011-08-26 10:38:10 UTC
Created attachment 194809 [details] [review]
telepathyClient: Add IM subscription request support

Changed SubscriptionRequestSource to MultiNotificationSource so it can
be reused for connection errors.
Comment 12 Owen Taylor 2011-08-26 11:43:59 UTC
(In reply to comment #10)
> Unfortunately this is blocked bug bug #645978: gjs can't emit signal
> contact-list-changed because it does not support GPtrArray in signals.

Our policy is *not* to block on GJS limitations (or alternatively put, libraries doing things like having signals with GPtrArray arguments). Work around it with whatever C helper functions you need.
Comment 13 Jasper St. Pierre (not reading bugmail) 2011-08-28 00:11:43 UTC
Created attachment 194947 [details] [review]
telepathyClient: Add IM subscription request support

Based on initial work from Guillaume Desmottes



--

OK, added a C wrapper.
Comment 14 Xavier Claessens 2011-08-28 17:29:29 UTC
smart trick with the C wrapper. But you never call shell_tp_client_grab_contact_list_changed() do you?
Comment 15 Jasper St. Pierre (not reading bugmail) 2011-08-28 17:38:30 UTC
(In reply to comment #14)
> smart trick with the C wrapper. But you never call
> shell_tp_client_grab_contact_list_changed() do you?

I do, in _connectionChanged.
Comment 16 Xavier Claessens 2011-08-28 17:46:24 UTC
ah right, didn't see that.
Comment 17 Matthias Clasen 2011-08-29 14:30:48 UTC
Xavier said on irc that the patch looks fine