GNOME Bugzilla – Bug 653941
Be able to accept/decline subscription request
Last modified: 2011-08-29 14:36:47 UTC
As part of bug #653938 the Shell should be able to accept and decline subscription requests.
This is blocked by https://bugs.freedesktop.org/show_bug.cgi?id=26205
Created attachment 194480 [details] [review] Add IM subscription request support
Created attachment 194481 [details] [review] Monitor account/contact changes to catch incoming subscription requests
Created attachment 194482 [details] [review] Add IM subscription request support Based on initial work from Guillaume Desmottes
Created attachment 194490 [details] [review] Add IM subscription request support Based on initial work from Guillaume Desmottes
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.
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.
Created attachment 194589 [details] [review] Add IM subscription request support Based on initial work from Guillaume Desmottes
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.
Unfortunately this is blocked bug bug #645978: gjs can't emit signal contact-list-changed because it does not support GPtrArray in signals.
Created attachment 194809 [details] [review] telepathyClient: Add IM subscription request support Changed SubscriptionRequestSource to MultiNotificationSource so it can be reused for connection errors.
(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.
Created attachment 194947 [details] [review] telepathyClient: Add IM subscription request support Based on initial work from Guillaume Desmottes -- OK, added a C wrapper.
smart trick with the C wrapper. But you never call shell_tp_client_grab_contact_list_changed() do you?
(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.
ah right, didn't see that.
Xavier said on irc that the patch looks fine