GNOME Bugzilla – Bug 653740
Support room invitations
Last modified: 2011-07-07 09:19:38 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.
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.
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.
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.
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?
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.
Oh, I didn't see that you have already reviewed my patch. This new version is not related to your review comments.
(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.
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.
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.
Review of attachment 191027 [details] [review]: LGTM
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?
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.
(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.
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.
> > @@ +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.
(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
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.
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.
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. */
(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.
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.
Review of attachment 191393 [details] [review]: LGTM
Attachment 191393 [details] pushed as 998c5f1 - Approve room invitations