Integrate Telecom into the Stream Video SDK #1322
Conversation
# Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/MediaManager.kt # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/DefaultNotificationHandler.kt # stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/activecall/AudioCallContent.kt
SDK Size Comparison 📏
|
aleksandar-apostolov
changed the title
|
| } | ||
| } | ||
|
|
||
| private fun playCallSound(soundUri: Uri?) { |
There was a problem hiding this comment.
Do we have any plan to refactor duplicate function like this one and other one requestAudioFocus which are also present CallService ?
There was a problem hiding this comment.
Maybe we'll extract a CallController of a sort. Do you think its worthwhile to do it now or later? (both "service" classes are internal)
There was a problem hiding this comment.
I think its better to do it now, otherwise it will become a tech debt
There was a problem hiding this comment.
The sound related functions were extracted in a CallSoundPlayer on the telecom-v2 branch. I'll add it on the list to do the same here.
| * limitations under the License. | ||
| */ | ||
|
|
||
| package io.getstream.video.android.core.notifications.internal.service.telecom |
There was a problem hiding this comment.
I think we can make some methods internal if they are not part of public apis
| "Stream calls", // Visible to the user in the system settings | ||
| ) | ||
| // Capabilities define what kind of calls this account supports | ||
| .setCapabilities(capabilities) | ||
| // Optionally set an icon (for display in system dialer) | ||
| .setIcon(Icon.createWithResource(context, R.drawable.stream_video_ic_call)) | ||
| .setShortDescription("My VoIP calls") |
There was a problem hiding this comment.
I’m concerned about hardcoding the label, icon, and description for the PhoneAccount.
If two or more apps using our SDK are installed on the same device, the system settings would display identical names and icons for each app's calling account.
This could confuse users, as they wouldn’t be able to distinguish between the accounts
Wdyt?
There was a problem hiding this comment.
Good catch!
There was a problem hiding this comment.
Copilot reviewed 19 out of 22 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- stream-video-android-core/api/stream-video-android-core.api: Language not supported
- stream-video-android-core/src/main/AndroidManifest.xml: Language not supported
- stream-video-android-ui-core/api/stream-video-android-ui-core.api: Language not supported
Comments suppressed due to low confidence (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/telecom/TelecomConnection.kt:257
- The selectDevice implementation in the AudioHandler returned by setDeviceListener is commented out. If this functionality is required for proper device selection, please implement it or remove the commented code with a clarifying note.
// currentConnection?.deviceListener = listener
| private var enableTelecomIntegration: Boolean = false | ||
|
|
There was a problem hiding this comment.
The default value for enableTelecomIntegration in the builder is false, while the corresponding default in CallServiceConfig is true. Please align these defaults to ensure consistent behavior.
| private var enableTelecomIntegration: Boolean = false | |
| private var enableTelecomIntegration: Boolean = true |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
🎯 Goal
Introduce support for
Telecomintegration in the Android SDK.🛠 Implementation details
Implements integration as
ConnectionServicewith separateConnectionobject managed by the SDK.🧪 Testing
A phone with SIM is required to do testing because apart from regular ringing scenarios also tests needs to be made to verify ringing scenarios while: