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 653740 - Support room invitations
Support room invitations
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 653938
 
 
Reported: 2011-06-30 12:38 UTC by Guillaume Desmottes
Modified: 2011-07-07 09:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rename Source and Notification to make clear they are about chats (3.53 KB, patch)
2011-06-30 12:39 UTC, Guillaume Desmottes
reviewed Details | Review
Approve room invitations (8.62 KB, patch)
2011-06-30 12:39 UTC, Guillaume Desmottes
reviewed Details | Review
Approve room invitations (9.06 KB, patch)
2011-06-30 13:31 UTC, Guillaume Desmottes
none Details | Review
rename Source and Notification to make clear they are about chats (3.53 KB, patch)
2011-06-30 13:56 UTC, Guillaume Desmottes
committed Details | Review
Approve room invitations (9.08 KB, patch)
2011-06-30 13:56 UTC, Guillaume Desmottes
needs-work Details | Review
Approve room invitations (9.88 KB, patch)
2011-07-04 10:19 UTC, Guillaume Desmottes
none Details | Review
Approve room invitations (9.89 KB, patch)
2011-07-04 11:10 UTC, Guillaume Desmottes
none Details | Review
Approve room invitations (10.84 KB, patch)
2011-07-05 11:21 UTC, Guillaume Desmottes
reviewed Details | Review
Approve room invitations (9.89 KB, patch)
2011-07-06 11:16 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2011-06-30 12:38:45 UTC
The Shell should act as a room invitation approver so user will be able to accept/decline invitations even if Empathy is not running.
Comment 1 Guillaume Desmottes 2011-06-30 12:39:28 UTC
Created attachment 191008 [details] [review]
rename Source and Notification to make clear they are about chats

We are going to support more Telepathy events hence more Source and
Notification types.
Comment 2 Guillaume Desmottes 2011-06-30 12:39:31 UTC
Created attachment 191009 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-06-30 13:03:13 UTC
Review of attachment 191008 [details] [review]:

::: js/ui/telepathyClient.js
@@ +71,3 @@
     _init : function() {
+        // channel path -> ChatSource
+        this._Chatsources = {};

this._sources was just fine. If you really want to rename, use this._chatSources.
Comment 4 Jasper St. Pierre (not reading bugmail) 2011-06-30 13:29:58 UTC
Review of attachment 191009 [details] [review]:

::: js/ui/telepathyClient.js
@@ +196,3 @@
+        Shell.get_tp_contacts(conn, [actor],
+                contactFeatures,
+                Lang.bind(this,  function (connection, contacts, failed) {

style nit: No space before function signature

  function(connection, contacts, failed)

@@ +206,3 @@
+                    Main.messageTray.add(source);
+
+                    let notif = new RoomInviteNotification(source, dispatchOp, channel, contacts[0]);

Only one contact again?

@@ +216,3 @@
     _approveChannels: function(approver, account, conn, channels,
                                dispatchOp, context) {
+        let channel = channels[0];

Shouldn't we support more than one channel in here?

@@ +222,3 @@
+            // Approve private text channels right away as we are going to handle it
+            dispatchOp.claim_with_async(this._tpClient,
+                                        Lang.bind (this, function(dispatchOp, result) {

style nit: No space before function call

  Lang.bind(this, function(dispatchOp, result) {

@@ +232,3 @@
+            context.accept();
+        }
+        else {

style nit: else goes on the same line

  } else {

@@ +730,3 @@
+
+function RoomInviteSource() {
+    this._init.apply(this, arguments);

Why bother with passing arguments? You never use it in the _init function anyway.

@@ +737,3 @@
+
+    _init: function() {
+        MessageTray.Source.prototype._init.call(this, _("Invitation"));

"Invitation" is kind of a bad name for a source.

Do we want to group all invitations into one source, sort of like how Cosimo is doing for the hotplug stuff?

http://blogs.gnome.org/cosimoc/2011/06/23/hotplug-hotness/

@@ +778,3 @@
+            switch (action) {
+            case 'decline':
+                dispatchOp.leave_channels_async(0, "", function(src, result) {

The zero seems suspect. As far as I could tell, you want:

  Tp.ChannelGroupChangeReason.NONE

instead.

::: src/shell-tp-client.c
@@ +103,3 @@
 
+  /* approve room invitations */
+  tp_base_client_take_approver_filter (TP_BASE_CLIENT (self), tp_asv_new (

Why isn't the observer/handler taking this filter as well?
Comment 5 Guillaume Desmottes 2011-06-30 13:31:27 UTC
Created attachment 191025 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 6 Guillaume Desmottes 2011-06-30 13:32:51 UTC
Oh, I didn't see that you have already reviewed my patch. This new version is not related to your review comments.
Comment 7 Guillaume Desmottes 2011-06-30 13:56:02 UTC
(In reply to comment #3)
> Review of attachment 191008 [details] [review]:
> 
> ::: js/ui/telepathyClient.js
> @@ +71,3 @@
>      _init : function() {
> +        // channel path -> ChatSource
> +        this._Chatsources = {};
> 
> this._sources was just fine. If you really want to rename, use
> this._chatSources.

fixed.

(In reply to comment #4)
> Review of attachment 191009 [details] [review]:
> 
> ::: js/ui/telepathyClient.js
> @@ +196,3 @@
> +        Shell.get_tp_contacts(conn, [actor],
> +                contactFeatures,
> +                Lang.bind(this,  function (connection, contacts, failed) {
> 
> style nit: No space before function signature
> 
>   function(connection, contacts, failed)

fixed.

> @@ +206,3 @@
> +                    Main.messageTray.add(source);
> +
> +                    let notif = new RoomInviteNotification(source, dispatchOp,
> channel, contacts[0]);
> 
> Only one contact again?

Yeah that's the inviter.

> @@ +216,3 @@
>      _approveChannels: function(approver, account, conn, channels,
>                                 dispatchOp, context) {
> +        let channel = channels[0];
> 
> Shouldn't we support more than one channel in here?

Mutli channels dispatching is pretty theoretical atm. It's not really
supported as MC always dispatch channel one by one (there is no API to request
more than one channel anyway).

> @@ +222,3 @@
> +            // Approve private text channels right away as we are going to
> handle it
> +            dispatchOp.claim_with_async(this._tpClient,
> +                                        Lang.bind (this, function(dispatchOp,
> result) {
> 
> style nit: No space before function call

>   Lang.bind(this, function(dispatchOp, result) {

fixed.

> @@ +232,3 @@
> +            context.accept();
> +        }
> +        else {
> 
> style nit: else goes on the same line
> 
>   } else {

changed.

> @@ +730,3 @@
> +
> +function RoomInviteSource() {
> +    this._init.apply(this, arguments);
> 
> Why bother with passing arguments? You never use it in the _init function
> anyway.

That's from a copy/paste of some code; fixed.

> @@ +737,3 @@
> +
> +    _init: function() {
> +        MessageTray.Source.prototype._init.call(this, _("Invitation"));
> 
> "Invitation" is kind of a bad name for a source.
> 
> Do we want to group all invitations into one source, sort of like how Cosimo is
> doing for the hotplug stuff?
> 
> http://blogs.gnome.org/cosimoc/2011/06/23/hotplug-hotness/

I don't know tbh; I'm open to design suggestions. But it's pretty rare to be
invited to more than one room at the same time so I wouldn't worry too much.

> @@ +778,3 @@
> +            switch (action) {
> +            case 'decline':
> +                dispatchOp.leave_channels_async(0, "", function(src, result) {
> 
> The zero seems suspect. As far as I could tell, you want:
> 
>   Tp.ChannelGroupChangeReason.NONE
> 
> instead.

Oh good catch! I wrote 0 for my test and forgot to change later. Fixed.

> ::: src/shell-tp-client.c
> @@ +103,3 @@
> 
> +  /* approve room invitations */
> +  tp_base_client_take_approver_filter (TP_BASE_CLIENT (self), tp_asv_new (
> 
> Why isn't the observer/handler taking this filter as well?

Because we just act as an Approver. All we want to do, for now at least, is to
display room invitations and allow user to accept/reject them; which is
exactly what Approvers are for.
Comment 8 Guillaume Desmottes 2011-06-30 13:56:16 UTC
Created attachment 191027 [details] [review]
rename Source and Notification to make clear they are about chats

We are going to support more Telepathy events hence more Source and
Notification types.
Comment 9 Guillaume Desmottes 2011-06-30 13:56:19 UTC
Created attachment 191028 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-06-30 14:32:18 UTC
Review of attachment 191027 [details] [review]:

LGTM
Comment 11 Guillaume Desmottes 2011-07-01 06:15:23 UTC
Comment on attachment 191027 [details] [review]
rename Source and Notification to make clear they are about chats

I merged the first patch. What about the second?
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-07-01 06:30:43 UTC
Review of attachment 191028 [details] [review]:

::: js/ui/telepathyClient.js
@@ +180,3 @@
 
+    _displayRoomInvitation: function(conn, channel, dispatchOp, context) {
+        // We only improve room to which we have been invited

... what?

@@ +183,3 @@
+        let selfHandle = channel.group_get_self_handle();
+        if (selfHandle == 0) {
+            Shell.decline_dispatch_op(context, "Not invited to the room");

We don't need to pass the dispatch op?

Also, should this text be translated? If so, you need to add some gettext around it. If not, lose the double quotes.

@@ +187,3 @@
+        }
+
+        let [invited, actor, reason, msg] = channel.group_get_local_pending_info(selfHandle);

"actor" seems like poor word choice here. Try "inviter"?

@@ +196,3 @@
+        Shell.get_tp_contacts(conn, [actor],
+                contactFeatures,
+                Lang.bind(this, function(connection, contacts, failed) {

This seems like it would be better in a separate function. To keep "channel" around like it was in the closure, use:

  Lang.bind(this, this._createRoomInviteSource, channel)

and the signature is:

  _createRoomInviteSource(connection, contacts, failed, channel);

@@ +227,3 @@
+                    this._handlingChannels(account, conn, channels);
+                } catch (err) {
+                    global.logError('Failed to Claim channel: ' + err);

Have we always been using global.logError in these situations? Nobody ever checks the looking glass...

@@ +744,3 @@
+        // destroy the source if the channel dispatch operation is invalidated
+        // as we can't approve any more.
+        this._invalidId = dispatchOp.connect('invalidated', Lang.bind(this, function(domain, code, msg) {

Watch your line lengths here.

@@ +762,3 @@
+        // system-users for now as Empathy does.
+        return new St.Icon({ icon_name: 'system-users',
+                             icon_type: St.IconType.SYMBOLIC,

We don't use SYMBOLIC for anything else in the message tray...

@@ +777,3 @@
+        MessageTray.Notification.prototype._init.call(this,
+                                                      source,
+                                                      _("Invitation to %s").format(channel.get_identifier()),

We should probably add translator comments here.

@@ +782,3 @@
+        this.setResident(true);
+
+        this.addBody(_("%s is inviting you to join %s").format(inviter.get_alias(), channel.get_identifier()));

... and here.

@@ +790,3 @@
+            switch (action) {
+            case 'decline':
+                dispatchOp.leave_channels_async(Tp.ChannelGroupChangeReason.NONE, "", function(src, result) {

Use single quotes for non-translatable text.

And watch your line lengths.

@@ +794,3 @@
+                break;
+            case 'accept':
+                dispatchOp.handle_with_time_async("", global.get_current_time(), function(src, result) {

I don't like that we could be doing a round-trip to the X server for the timestamp here.

::: src/shell-tp-client.c
@@ +103,3 @@
 
+  /* approve room invitations */
+  tp_base_client_take_approver_filter (TP_BASE_CLIENT (self), tp_asv_new (

I'd add a comment explaining why you only add this filter to the approver.
Comment 13 Guillaume Desmottes 2011-07-04 10:19:25 UTC
(In reply to comment #12)
> Review of attachment 191028 [details] [review]:
> 
> ::: js/ui/telepathyClient.js
> @@ +180,3 @@
> 
> +    _displayRoomInvitation: function(conn, channel, dispatchOp, context) {
> +        // We only improve room to which we have been invited
> 
> ... what?

"approve"; fixed.

> @@ +183,3 @@
> +        let selfHandle = channel.group_get_self_handle();
> +        if (selfHandle == 0) {
> +            Shell.decline_dispatch_op(context, "Not invited to the room");
> 
> We don't need to pass the dispatch op?

No, it's included in the context.

> Also, should this text be translated? If so, you need to add some gettext
> around it. If not, lose the double quotes.

No that's just a GError message used in debug messages; I fixed the quoting.

> @@ +187,3 @@
> +        }
> +
> +        let [invited, actor, reason, msg] =
> channel.group_get_local_pending_info(selfHandle);
> 
> "actor" seems like poor word choice here. Try "inviter"?

That's the TP name, but ok "inviter" is clearer; fixed.

> @@ +196,3 @@
> +        Shell.get_tp_contacts(conn, [actor],
> +                contactFeatures,
> +                Lang.bind(this, function(connection, contacts, failed) {
> 
> This seems like it would be better in a separate function. To keep "channel"
> around like it was in the closure, use:
> 
>   Lang.bind(this, this._createRoomInviteSource, channel)
> 
> and the signature is:
> 
>   _createRoomInviteSource(connection, contacts, failed, channel);

done.

> @@ +227,3 @@
> +                    this._handlingChannels(account, conn, channels);
> +                } catch (err) {
> +                    global.logError('Failed to Claim channel: ' + err);
> 
> Have we always been using global.logError in these situations? Nobody ever
> checks the looking glass...

I changed to a throw.

> @@ +744,3 @@
> +        // destroy the source if the channel dispatch operation is invalidated
> +        // as we can't approve any more.
> +        this._invalidId = dispatchOp.connect('invalidated', Lang.bind(this,
> function(domain, code, msg) {
> 
> Watch your line lengths here.

fixed.

> @@ +762,3 @@
> +        // system-users for now as Empathy does.
> +        return new St.Icon({ icon_name: 'system-users',
> +                             icon_type: St.IconType.SYMBOLIC,
> 
> We don't use SYMBOLIC for anything else in the message tray...

what should I use then?

> @@ +777,3 @@
> +        MessageTray.Notification.prototype._init.call(this,
> +                                                      source,
> +                                                      _("Invitation to
> %s").format(channel.get_identifier()),
> 
> We should probably add translator comments here.

done.

> @@ +782,3 @@
> +        this.setResident(true);
> +
> +        this.addBody(_("%s is inviting you to join
> %s").format(inviter.get_alias(), channel.get_identifier()));
> 
> ... and here.

done.

> @@ +790,3 @@
> +            switch (action) {
> +            case 'decline':
> +               
> dispatchOp.leave_channels_async(Tp.ChannelGroupChangeReason.NONE, "",
> function(src, result) {
> 
> Use single quotes for non-translatable text.

changed.

> And watch your line lengths.

fixed.

> @@ +794,3 @@
> +                break;
> +            case 'accept':
> +                dispatchOp.handle_with_time_async("",
> global.get_current_time(), function(src, result) {
> 
> I don't like that we could be doing a round-trip to the X server for the
> timestamp here.

You are the one who told me to use this function...
https://bugzilla.gnome.org/show_bug.cgi?id=643594#c17

> ::: src/shell-tp-client.c
> @@ +103,3 @@
> 
> +  /* approve room invitations */
> +  tp_base_client_take_approver_filter (TP_BASE_CLIENT (self), tp_asv_new (
> 
> I'd add a comment explaining why you only add this filter to the approver.

done.
Comment 14 Guillaume Desmottes 2011-07-04 10:19:37 UTC
Created attachment 191221 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 15 Jasper St. Pierre (not reading bugmail) 2011-07-04 10:42:02 UTC
> > @@ +762,3 @@
> > +        // system-users for now as Empathy does.
> > +        return new St.Icon({ icon_name: 'system-users',
> > +                             icon_type: St.IconType.SYMBOLIC,
> > 
> > We don't use SYMBOLIC for anything else in the message tray...
> 
> what should I use then?
> 

why not FULLCOLOR?
> 
> > @@ +794,3 @@
> > +                break;
> > +            case 'accept':
> > +                dispatchOp.handle_with_time_async("",
> > global.get_current_time(), function(src, result) {
> > 
> > I don't like that we could be doing a round-trip to the X server for the
> > timestamp here.
> 
> You are the one who told me to use this function...
> https://bugzilla.gnome.org/show_bug.cgi?id=643594#c17

Hi. I'm Jasper, nice to meet you. That was Dan. :)

And I was thinking that we could get a timestamp from the message tp-glib sends us, but that was stupid. Ignore that.
Comment 16 Guillaume Desmottes 2011-07-04 11:10:31 UTC
(In reply to comment #15)
> > > @@ +762,3 @@
> > > +        // system-users for now as Empathy does.
> > > +        return new St.Icon({ icon_name: 'system-users',
> > > +                             icon_type: St.IconType.SYMBOLIC,
> > > 
> > > We don't use SYMBOLIC for anything else in the message tray...
> > 
> > what should I use then?
> > 
> 
> why not FULLCOLOR?

changed.

> > > @@ +794,3 @@
> > > +                break;
> > > +            case 'accept':
> > > +                dispatchOp.handle_with_time_async("",
> > > global.get_current_time(), function(src, result) {
> > > 
> > > I don't like that we could be doing a round-trip to the X server for the
> > > timestamp here.
> > 
> > You are the one who told me to use this function...
> > https://bugzilla.gnome.org/show_bug.cgi?id=643594#c17
> 
> Hi. I'm Jasper, nice to meet you. That was Dan. :)

Rah sorry, I keep thinking you are the only one commenting on my patch for
some reason. :p
Comment 17 Guillaume Desmottes 2011-07-04 11:10:39 UTC
Created attachment 191224 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 18 Guillaume Desmottes 2011-07-05 11:21:53 UTC
Created attachment 191296 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 19 Jasper St. Pierre (not reading bugmail) 2011-07-05 17:13:17 UTC
Review of attachment 191296 [details] [review]:

::: js/ui/telepathyClient.js
@@ +118,3 @@
             let [targetHandle, targetHandleType] = channel.get_handle();
 
+            // Only observe contact text channels

Whoops, I had no idea we already used these commenting styles in some places.

Leave these alone. Sorry.

@@ +180,3 @@
 
+    _displayRoomInvitation: function(conn, channel, dispatchOp, context) {
+        // We can only approve the room if we have been invited to it

Still a bit Engrish-y.

"We can only approve the rooms"

@@ +203,3 @@
+    _createRoomInviteSource: function(connection, contacts, failed, channel, context, dispatchOp) {
+        if (contacts.length < 1) {
+            Shell.decline_dispatch_op(context, 'Failed to get inviter');

Is this something we want to translate? Where would it be shown?

@@ +788,3 @@
+
+        /* translators: first argument is the name of a contact and the second
+         * one the name of a room. "Alice is inviting to join room@jabber.org

"Alice is inviting *you*"

@@ +803,3 @@
+                break;
+            case 'accept':
+                dispatchOp.handle_with_time_async("", global.get_current_time(),

Single quotes.

::: src/shell-tp-client.c
@@ +102,3 @@
   tp_base_client_add_approver_filter (TP_BASE_CLIENT (self), filter);
 
+  /* Approve room invitations. We don't handle or observe room channels so

/* We don't actually support room channels in our client, 
 * so just register us as an approver. */
Comment 20 Guillaume Desmottes 2011-07-06 11:15:32 UTC
(In reply to comment #19)
> Review of attachment 191296 [details] [review]:
> 
> ::: js/ui/telepathyClient.js
> @@ +118,3 @@
>              let [targetHandle, targetHandleType] = channel.get_handle();
> 
> +            // Only observe contact text channels
> 
> Whoops, I had no idea we already used these commenting styles in some places.
> 
> Leave these alone. Sorry.

sigh...

changed.

> @@ +180,3 @@
> 
> +    _displayRoomInvitation: function(conn, channel, dispatchOp, context) {
> +        // We can only approve the room if we have been invited to it
> 
> Still a bit Engrish-y.
> 
> "We can only approve the rooms"

changed.

> @@ +203,3 @@
> +    _createRoomInviteSource: function(connection, contacts, failed, channel,
> context, dispatchOp) {
> +        if (contacts.length < 1) {
> +            Shell.decline_dispatch_op(context, 'Failed to get inviter');
> 
> Is this something we want to translate? Where would it be shown?

As said in Comment 13, that's just the message of a GError which may
eventuallly be displayed in some debug logs; no need to translate.

> @@ +788,3 @@
> +
> +        /* translators: first argument is the name of a contact and the second
> +         * one the name of a room. "Alice is inviting to join room@jabber.org
> 
> "Alice is inviting *you*"

fixed.

> @@ +803,3 @@
> +                break;
> +            case 'accept':
> +                dispatchOp.handle_with_time_async("",
> global.get_current_time(),
> 
> Single quotes.

changed.

> ::: src/shell-tp-client.c
> @@ +102,3 @@
>    tp_base_client_add_approver_filter (TP_BASE_CLIENT (self), filter);
> 
> +  /* Approve room invitations. We don't handle or observe room channels so
> 
> /* We don't actually support room channels in our client, 
>  * so just register us as an approver. */

Well we do support them as we approve them. We are just an Approver and not a
Handler.
Comment 21 Guillaume Desmottes 2011-07-06 11:16:10 UTC
Created attachment 191393 [details] [review]
Approve room invitations

We use to rely on Empathy for this but as we plan to allow users to be
connected on IM without having Empathy running the Shell should do it now.
Comment 22 Jasper St. Pierre (not reading bugmail) 2011-07-06 16:38:54 UTC
Review of attachment 191393 [details] [review]:

LGTM
Comment 23 Guillaume Desmottes 2011-07-07 09:19:32 UTC
Attachment 191393 [details] pushed as 998c5f1 - Approve room invitations