Merge Incoming, Outgoing, Call activities into a single one by kanat · Pull Request #72 · GetStream/stream-video-android

kanat · 2022-11-11T00:03:39Z

Closes: #38

# Conflicts: # app/src/main/kotlin/io/getstream/video/android/app/router/StreamRouterImpl.kt # app/src/main/kotlin/io/getstream/video/android/app/ui/call/CallActivity.kt # app/src/main/kotlin/io/getstream/video/android/app/ui/home/HomeActivity.kt # dogfooding/src/main/java/io/getstream/video/android/dogfooding/CallActivity.kt # dogfooding/src/main/java/io/getstream/video/android/dogfooding/DeeplinkingActivity.kt # dogfooding/src/main/java/io/getstream/video/android/dogfooding/HomeActivity.kt # stream-video-android/src/main/kotlin/io/getstream/video/android/router/StreamRouter.kt # stream-video-android/src/main/kotlin/io/getstream/video/android/viewmodel/CallViewModel.kt # stream-video-android/src/main/kotlin/io/getstream/video/android/viewmodel/IncomingCallViewModel.kt # stream-video-android/src/main/kotlin/io/getstream/video/android/viewmodel/OutgoingCallViewModel.kt

filbabic · 2022-11-11T06:31:35Z

+
    override fun onDestroy() {
-        streamVideo.removeSocketListener(socketListener)
+        logger.d { "[onDestroy] no args" }


Do we need this then if we're not doing anything in onDestroy?

Yeah, left after debugging, thanks for catching this

filbabic · 2022-11-11T06:31:39Z

    }

+    override fun finish() {
+        logger.d { "[finish] no args" }


filbabic · 2022-11-11T06:34:43Z

+import io.getstream.video.android.viewmodel.CallViewModelFactory
+import io.getstream.video.android.viewmodel.PermissionManagerImpl
+
+public abstract class AbstractComposeCallActivity : AppCompatActivity(), StreamCallActivity {


Will this be the recommended way of building our Call screen for customers?

We'll have to outline this in our tutorial if so

Also - it mgith be better to rename StreamCallActivity to something else, as this seems like it implements two different set of activities (not possible in Android).

Maybe: StreamVideoProvider

Yeah, kinda lazy way for lazy people 😁

StreamVideoProvider - looks good 👌

filbabic · 2022-11-11T06:39:44Z

+        if (isInitialized) return
+        callViewModel.connectToCall(
+            CallSettings(
+                audioOn = false,


Maybe this should be a function that returns the call settings and users can override it to provide a different default:

protected open fun getDefaultCallSettings(): CallSettings { return CallSettings(false, true, false) }

filbabic · 2022-11-11T06:41:56Z

 }
+
+private fun StreamCallState.asTitle() = when (this) {
+    is StreamCallState.Drop -> "Drop"


Is it possible to use the class name here maybe? Or just expose a function in StreamCallState called: formatAsTitle that each sealed class member implements

filbabic · 2022-11-11T06:42:34Z

+    onAcceptCall: () -> Unit,
+    onCancelCall: () -> Unit = {},
+    onMicToggleChanged: (Boolean) -> Unit = {},
+    onVideoToggleChanged: (Boolean) -> Unit,


I would say that either all have default or none. Having a mix is confusing

Prefer all having default empty here

filbabic · 2022-11-11T06:43:15Z

     */
    public val callState: StateFlow<StreamCallState>

+    public fun launch(block: suspend CoroutineScope.() -> Unit)


Why do we need this here?

I was debugging the call flow and left it by mistake 🙈

filbabic · 2022-11-11T06:44:26Z

     */
    override val callState: StateFlow<StreamCallState> = engine.callState

+    override fun launch(block: suspend CoroutineScope.() -> Unit) {


Yeah this feels a bit weird to me - if we're going for Java interop or something, not sure if they can pass in a suspending lambda

will be removed

filbabic · 2022-11-11T06:54:09Z

+    override suspend fun acceptCall(cid: String): Result<JoinedCall> {
+        logger.d { "[acceptCall] cid: $cid" }
+        val (type, id) = cid.split(":").apply {
+            if (size != 2) return Failure(VideoError(message = "invalid cid: $cid"))


Maybe we can pull these two functions out as utilities:

public fun isValidCid(cid: String): Boolean { // can also be an extension return cid.split(":").size == 2 } public fun toIdAndType(cid: String): Pair<String, String> { return (maybe valid check here) cid.split(":").take(2) }

filbabic · 2022-11-11T06:57:18Z

+            logger.i { "[onSfuEvent] event: $event" }
+        }
+        when (event) {
+            is AudioLevelChangedEvent -> { }


Does it make sense to only handle the ones we care about and then just use:

else -> {}

As a default? I know that if we add more events it might be missed in the else branch, but this way it's just a bit ugly because we're effectively doing that but in a much longer way

Since Engine is still something as WIP and we're supposed to support other events, so I just left the whole when options to see how many are left to support.
But we can replace it with else, I have no issues with that

I'm fine with leaving it as-is until we reach a more "final" state

filbabic · 2022-11-11T06:59:48Z

+        StreamLog.d(TAG) { "/start/ activity: ${input.className}" }
+        startActivity(
+            newIntent(input.className).apply {
+                addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)


Do we always need these flags? I imagine so because we're coming here from services

It's the default behavior defined in DefaultCallAndroidInputLauncher, cause Activity can be launched from the appContext when PN arrives, for instance.

This can be easily overridden by providing a custom implementation of CallAndroidInputLauncher interface

filbabic · 2022-11-11T07:01:12Z

+    public fun onUserLoggedIn()

-    public fun onOutgoingCall()
+    public fun onUserLoggedOut()


Why does the router only know about login and logout now?

If that's the only thing it does, should be just remove the router altogether?

filbabic · 2022-11-11T07:03:01Z

+    private val _callId: MutableStateFlow<String> = MutableStateFlow(value = "")
+    public val callId: StateFlow<String> = _callId
+
+    private val _participants: MutableStateFlow<List<CallUser>> = MutableStateFlow(emptyList())


We already have participantList, and now we also have CallUser - what is the difference/use case for these?

I left it as it is for now, just merged what we had in all 3 flows (outgoing, incoming, in-call).

That's what I was thinking about as well, cause we have duplicate data in the following models:

CallUser

CallMember

CallParticipant

Maybe we could leave just CallUser & CallParticipant, what do you think?

Yeah that sounds good, the CallParticipant is when a user is in a call and has possible tracks published, whereas CallMember can be outside-of-the-call, but still a member

filbabic · 2022-11-11T07:03:37Z

+    private val _participants: MutableStateFlow<List<CallUser>> = MutableStateFlow(emptyList())
+    public val participants: StateFlow<List<CallUser>> = _participants
+
+    private val _isMicrophoneEnabled = MutableStateFlow(false)


These should be moved up to permissions and videoEnabled and such

filbabic · 2022-11-11T07:05:17Z

+                    is State.Outgoing -> {
+                        _callType.value = CallType.fromType(state.callGuid.type)
+                        _callId.value = state.callGuid.id
+                        _participants.value = state.users.values


We should probably call this something else then, not participants - as if we're in Incoming/Outgoing calls, we're not participanting yet, maybe members or invitees or idk

filbabic · 2022-11-11T07:05:55Z

-        initializeCall(callSettings = callSettings)
+        viewModelScope.launch {
+            logger.d { "[connectToCall] state: ${streamCallState.value}" }
+            withTimeout(30_000) {


Pull out as a constant

filbabic · 2022-11-11T07:06:26Z

+                val state = streamCallState.first { it is State.InCall } as State.InCall
+                logger.v { "[connectToCall] received: ${streamCallState.value}" }
+                client = streamVideo.createCallClient(
+                    state.callUrl.removeSuffix("/twirp"),


ideally we'd not hardcode this but the BE should provide us with a sanitized URL, however we can maybe pull out as a util

Not sure about this URL massaging, cause it was there before, so I just left it as it is =]

Yeah I added this bit since the BE returns a URL with an extra path suffix, and retrofit hates that

We can pull this out to a utils function maybe

filbabic · 2022-11-11T07:08:03Z

+        this._isCameraEnabled.value = videoEnabled
+    }
+
+    private fun State.Outgoing.toMetadata(): CallMetadata =


Can be pulled out to a utility function rather than be placed here

filbabic

Left some general comments on things I wasn't sure about.

There's one larger overarching thing:

We need KDocs absolutely everywhere we can: values/properties, private, internal nad public functions.

The codebase is getting larger and more complex, so it would be great to understand what's going on. Especially for new people.

We can use this branch for baseline and then branch off of it to work on other stuff, but I'd recommend writing KDocs as you go for everything you can (even if it seems repetitive), so that we have a nice and clean codebase that's well documented.

kanat added 4 commits November 10, 2022 07:20

[38] WIP - merge activities

758cc5f

fix merge conflicts

6eb27bd

[38] merge activities

73cd50d

kanat linked an issue Nov 11, 2022 that may be closed by this pull request

Merge Incoming, Outgoing, Call activities into a single one #38

Closed

kanat marked this pull request as ready for review November 11, 2022 00:04

kanat changed the title ~~Task/merge activities~~ Merge Incoming, Outgoing, Call activities into a single one Nov 11, 2022

filbabic reviewed Nov 11, 2022

View reviewed changes

Comment thread ...compose/src/main/kotlin/io/getstream/video/android/compose/ui/AbstractComposeCallActivity.kt

filbabic reviewed Nov 11, 2022

View reviewed changes

kanat added 2 commits November 11, 2022 16:37

[38] fix PR comments

eebf3e2

[38] fix api dump

656455d

kanat merged commit fb211ae into main Nov 14, 2022

Uh oh!

Conversation

kanat commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kanat Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filbabic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kanat commented Nov 11, 2022 •

edited

Loading

kanat Nov 11, 2022 •

edited

Loading