GNOME Bugzilla – Bug 653939
Be able to approve audio/video calls
Last modified: 2011-08-01 08:37:04 UTC
As part of bug #653938 the Shell should be an audio/video call approver.
Created attachment 191731 [details] [review] Turn RoomInviteSource to a more generic source All the approving source will be basically the same.
Created attachment 191732 [details] [review] Approve audio/video channels Support the old (StreamedMedia) and new (Call) API for now as the latter is still used.
Review of attachment 191731 [details] [review]: ::: js/ui/telepathyClient.js @@ +816,2 @@ + this._icon = icon; + this._setSummaryIcon(this.createNotificationIcon(icon)); createNotificationIcon doesn't take an argument?
Good catch, removed. Thanks JS for not detecting this error...
Created attachment 191735 [details] [review] Approve audio/video channels Support the old (StreamedMedia) and new (Call) API for now as the latter is still used.
Created attachment 191802 [details] [review] Turn RoomInviteSource to a more generic source All the approving source will be basically the same.
Created attachment 191804 [details] [review] Approve audio/video channels Support the old (StreamedMedia) and new (Call) API for now as the latter is still used.
Review of attachment 191802 [details] [review]: LGTM.
Review of attachment 191804 [details] [review]: telepathyClient.js is getting quite big. Do we want to try and split it out into two? telepathyChat.js handles all of the custom shell stuff (inline reply, etc.) and telepathyClient.js handles all of the underlying Telepathy stuff? ::: js/ui/notificationDaemon.js @@ +191,3 @@ let id; + // Filter out chat, presence, calls and invitation notifications from Empathy, since we Comment line length is getting a little long. @@ +195,3 @@ if (appName == 'Empathy' && (hints['category'] == 'im.received' || hints['category'] == 'x-empathy.im.room-invitation' || + hints['category'] == 'x-empathy.call.incoming' || Do you think it might be better to split this out into a separate blacklist variable: // app name => list of categories const NotificationBlacklist = { 'Empathy': [ 'im.received', 'x-empathy.im.room-invitation', ... ] }; Just thinking out loud though. ::: js/ui/telepathyClient.js @@ +236,3 @@ + this._approveTextChannel(account, conn, channel, dispatchOp, context); + else if (chanType == Tp.IFACE_CHANNEL_TYPE_STREAMED_MEDIA) + this._approveStreamedMediChannel(account, conn, channel, dispatchOp, context); "StreamedMedi"? @@ +261,3 @@ }, + _approveStreamedMediChannel: function(account, conn, channel, dispatchOp, context) { You've piqued my curiosity: what's a "Streamed Media" channel, and how does it differ from a call? @@ +264,3 @@ + let [targetHandle, targetHandleType] = channel.get_handle(); + + Shell.get_tp_contacts(conn, [targetHandle], Could there be any other target handle types that we need to watch out for? @@ +269,3 @@ + }, + + _approveCallChannel: function(account, conn, channel, dispatchOp, context) { These two are effectively the same method, just passing a different flag to the callback. @@ +277,3 @@ + }, + + _createAudioVideoSource: function(connection, contacts, failed, channel, context, dispatchOp, call) { "isCall" @@ +283,3 @@ + } + + let video = false; "isVideo" @@ +290,3 @@ + video = true; + } + else { Watch your style. No newline in between closing brace and else. @@ +941,3 @@ + +// Audio Video +function AudioVideoNotification(source, dispatchOp, channel, contact, video) { "isVideo" @@ +953,3 @@ + if (video) + /* translators: argument is a contact name like Alice for example. */ + title = _("Video Call from %s").format(contact.get_alias()); "Video call from %s" ::: src/shell-tp-client.c @@ +111,3 @@ NULL)); + /* Approve calls (old and new API). We let Empathy handling those. */ "old and new API"? "We let Empathy handle the call itself." @@ +119,3 @@ + NULL)); + + /* FIXME: use TP_IFACE_CHANNEL_TYPE_CALL once it's merged in tp-glib */ Can you put a link to FDO Bugzilla so we know "when it's merged"? @@ +123,3 @@ + tp_asv_new ( + TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, + "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT", Doesn't seem like we should be using "DRAFT".
Comment on attachment 191802 [details] [review] Turn RoomInviteSource to a more generic source Attachment 191802 [details] pushed as 90db743 - Turn RoomInviteSource to a more generic source
(In reply to comment #9) > Review of attachment 191804 [details] [review]: > > telepathyClient.js is getting quite big. Do we want to try and split it out > into two? > > telepathyChat.js handles all of the custom shell stuff (inline reply, etc.) and > telepathyClient.js handles all of the underlying Telepathy stuff? Humm We could have telepathyChat using the existing ShellTpClient and telepathyApprover using TpSimpleApprover and approving rooms, calls, file transfer, etc ? > ::: js/ui/notificationDaemon.js > @@ +191,3 @@ > let id; > > + // Filter out chat, presence, calls and invitation notifications from > Empathy, since we > > Comment line length is getting a little long. splitted. > @@ +195,3 @@ > if (appName == 'Empathy' && (hints['category'] == 'im.received' || > hints['category'] == 'x-empathy.im.room-invitation' || > + hints['category'] == 'x-empathy.call.incoming' || > > Do you think it might be better to split this out into a separate blacklist > variable: > > // app name => list of categories > const NotificationBlacklist = { > 'Empathy': [ > 'im.received', > 'x-empathy.im.room-invitation', > ... > ] > }; > > Just thinking out loud though. Don't think that would buy us anything tbh. > ::: js/ui/telepathyClient.js > @@ +236,3 @@ > + this._approveTextChannel(account, conn, channel, dispatchOp, > context); > + else if (chanType == Tp.IFACE_CHANNEL_TYPE_STREAMED_MEDIA) > + this._approveStreamedMediChannel(account, conn, channel, > dispatchOp, context); > > "StreamedMedi"? Media; fixed. > @@ +261,3 @@ > }, > > + _approveStreamedMediChannel: function(account, conn, channel, dispatchOp, > context) { > > You've piqued my curiosity: what's a "Streamed Media" channel, and how does it > differ from a call? StreamedMedia is the old API while Call is the new WIP one (that's why there is DRAFT in its name and I didn't use constants from tp-glib). > @@ +264,3 @@ > + let [targetHandle, targetHandleType] = channel.get_handle(); > + > + Shell.get_tp_contacts(conn, [targetHandle], > > Could there be any other target handle types that we need to watch out for? Atm no. We only ask to improve CONTACT channel so we'll only get those. This may change at some point when we'll have multi user calls but we're not there yet. > @@ +269,3 @@ > + }, > + > + _approveCallChannel: function(account, conn, channel, dispatchOp, context) > { > > These two are effectively the same method, just passing a different flag to the > callback. I now use the same function with an extra arg. > @@ +277,3 @@ > + }, > + > + _createAudioVideoSource: function(connection, contacts, failed, channel, > context, dispatchOp, call) { > > "isCall" changed. > @@ +283,3 @@ > + } > + > + let video = false; > > "isVideo" changed. > @@ +290,3 @@ > + video = true; > + } > + else { > > Watch your style. No newline in between closing brace and else. fixed. > @@ +941,3 @@ > + > +// Audio Video > +function AudioVideoNotification(source, dispatchOp, channel, contact, video) { > > "isVideo" changed. > @@ +953,3 @@ > + if (video) > + /* translators: argument is a contact name like Alice for > example. */ > + title = _("Video Call from %s").format(contact.get_alias()); > > "Video call from %s" changed. > ::: src/shell-tp-client.c > @@ +111,3 @@ > NULL)); > > + /* Approve calls (old and new API). We let Empathy handling those. */ > > "old and new API"? StreamedMedia and Call > "We let Empathy handle the call itself." changed. > @@ +119,3 @@ > + NULL)); > + > + /* FIXME: use TP_IFACE_CHANNEL_TYPE_CALL once it's merged in tp-glib */ > > Can you put a link to FDO Bugzilla so we know "when it's merged"? done. > @@ +123,3 @@ > + tp_asv_new ( > + TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, > + "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT", > > Doesn't seem like we should be using "DRAFT". It doesn't harm to do so and make things easier for people testing Telepathy Call support.
Created attachment 191942 [details] [review] Approve audio/video channels Support the old (StreamedMedia) and new (Call) API for now as the latter is still used.
Review of attachment 191942 [details] [review]: When I said "split out", I really meant splitting out only the telepathy chat interface (inline reply). Things like notifications/sources can still go in telepathyClient.js. Anyway, that's for another bug. ::: js/ui/telepathyClient.js @@ +278,3 @@ + + let props = channel.borrow_immutable_properties(); + if (isCall) { I'm not sure why 'isCall' is necessary. Why can't we do: if (props['org.freedesktop.Telepathy.Channel.Type.Call.DRAFT.InitialVideo'] || props[Tp.PROP_CHANNEL_TYPE_STREAMED_MEDIA_INITIAL_VIDEO]) isVideo = true; @@ +967,3 @@ + break; + case 'answer': + dispatchOp.handle_with_time_async('', global.get_current_time(), What happens when you approve? Empathy pops up with a video chat? What happens when you reject? The notification goes away? Should we pop up an entry for someone to type a reason? You should talk to the designers about this..
(In reply to comment #13) > Review of attachment 191942 [details] [review]: > > When I said "split out", I really meant splitting out only the telepathy chat > interface (inline reply). Things like notifications/sources can still go in > telepathyClient.js. Anyway, that's for another bug. So moving the ChatSource and ChatNotification objects to another file? But yeah, let's do that once this is merged. > ::: js/ui/telepathyClient.js > @@ +278,3 @@ > + > + let props = channel.borrow_immutable_properties(); > + if (isCall) { > > I'm not sure why 'isCall' is necessary. > > Why can't we do: > > if (props['org.freedesktop.Telepathy.Channel.Type.Call.DRAFT.InitialVideo'] > || > props[Tp.PROP_CHANNEL_TYPE_STREAMED_MEDIA_INITIAL_VIDEO]) > isVideo = true; I was assuming that js would have raised a key error exception as Python does but it seems it does't so I removed isChat. > @@ +967,3 @@ > + break; > + case 'answer': > + dispatchOp.handle_with_time_async('', > global.get_current_time(), > > What happens when you approve? Empathy pops up with a video chat? Yes. > What happens when you reject? The notification goes away? Should we pop up an > entry for someone to type a reason? The notification goes away and caller sees you rejected his call. > You should talk to the designers about this.. We did discuss this at the hackfest with Allan. He has some mockups and cool design about how to nicely integrate calls in the Shell. But that will be for later. For now my goal is to provide the same experience we have atm with Empathy's notification as we need to be able to handle incoming call if Empathy is not running (bug #653938).
Created attachment 192018 [details] [review] Approve audio/video channels Support the old (StreamedMedia) and new (Call) API for now as the latter is still used.
Also in http://cgit.collabora.com/git/user/cassidy/gnome-shell/log/?h=calls-653939
Review of attachment 192018 [details] [review]: One style fixup, otherwise LGTM. ::: js/ui/telepathyClient.js @@ +237,3 @@ + else if (chanType == Tp.IFACE_CHANNEL_TYPE_STREAMED_MEDIA) + this._approveCall(account, conn, channel, dispatchOp, context); + else if (chanType == 'org.freedesktop.Telepathy.Channel.Type.Call.DRAFT') else if (chanType == Tp.IFACE_CHANNEL_TYPE_STREAMED_MEDIA || chanType == 'org.freedesktop.Telepathy.Channel.Type.Call.DRAFT')
Comment on attachment 192018 [details] [review] Approve audio/video channels Committed as cecba0269fc2b3cbad8e4baeaf1e5b03a39fe37b after fixing coding style in Comment #17.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.