Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ribActionEvents stream #599

Merged
merged 20 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8587e79
Drafting monitoring option for RIBs
FranAguilera Jun 14, 2023
66eac33
Expose stream for RIB component duration info
FranAguilera Jun 15, 2023
5ff4748
Drafting more ideas
FranAguilera Jun 16, 2023
26fe506
Merge branch 'main' into add_rib_monitoring
FranAguilera Jun 20, 2023
3ab3955
Drafting more ideas
FranAguilera Jun 16, 2023
18ced24
Merge branch 'add_rib_monitoring' of https://github.com/FranAguilera/…
FranAguilera Jun 20, 2023
80c5846
spotless apply
FranAguilera Jun 20, 2023
38ae582
Rename callRibActionAndEmitEvents to triggerRibActionAndEmitEvents
FranAguilera Jun 21, 2023
546ac78
Adding back getInstance for RibEvents and mark it as deprecated
FranAguilera Jun 21, 2023
062b002
Removing again getInstance of RibEvents
FranAguilera Jun 21, 2023
747b831
Change actionBlock declaration order
FranAguilera Jun 21, 2023
5a42204
Update JUnit tests
FranAguilera Jun 21, 2023
7009b81
Apply PR feedback
FranAguilera Jun 21, 2023
2edee91
First proposal after PR feedback
FranAguilera Jun 22, 2023
7f251b1
Rename startCapturingRibActionInfo
FranAguilera Jun 22, 2023
8201984
PR feedback: Rename RibComponent and add switch method for RibEvents
FranAguilera Jun 22, 2023
2a5db8e
Remove unnecessary comments for test method
FranAguilera Jun 22, 2023
3e7983a
Rename RibEventEmitter to RibActionEmitter
FranAguilera Jun 22, 2023
ee3aff2
Open areRibActionEmissionsAllowed get visibility
FranAguilera Jun 22, 2023
59014ba
Remove disableRibActionEmissions and use internal setter
FranAguilera Jun 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ object ApplicationLevelWorkerLogger {

@OptIn(DelicateCoroutinesApi::class)
fun start() {
RibEvents.allowRibActionEmissions()

GlobalScope.launch {
RibEvents.ribActionEvents
.filter { it.ribComponentType == RibComponentType.DEPRECATED_WORKER }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import kotlinx.coroutines.rx2.asObservable
* @param <P> the type of [Presenter].
* @param <R> the type of [Router].
*/
public abstract class Interactor<P : Any, R : Router<*>>() : InteractorType {
public abstract class Interactor<P : Any, R : Router<*>>() : InteractorType, RibComponent {
@Inject public lateinit var injectedPresenter: P
internal var actualPresenter: P? = null
private val _lifecycleFlow = MutableSharedFlow<InteractorEvent>(1, 0, BufferOverflow.DROP_OLDEST)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import org.checkerframework.checker.guieffect.qual.UIEffect
* practice this caused confusion: if both a presenter and interactor can perform complex rx logic
* it becomes unclear where you should write your bussiness logic.
*/
public abstract class Presenter : ScopeProvider {
public abstract class Presenter : ScopeProvider, RibComponent {
private val _lifecycleFlow = MutableSharedFlow<PresenterEvent>(1, 0, BufferOverflow.DROP_OLDEST)
public open val lifecycleFlow: SharedFlow<PresenterEvent>
get() = _lifecycleFlow
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
*/
package com.uber.rib.core

public interface RibComponent

public enum class RibComponentType {
Copy link
Member

@tyvsmith tyvsmith Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coroutine Worker? Why reserve for next release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to tweak a bit more RibCoroutine worker in order to provide accurate monitoring as mentioned by @psteiger earlier

Given there are barely any users internally of RibCoroutineWorker, I'm not concerned to leave these changes to be implemented on a separate PR

ROUTER,
PRESENTER,
INTERACTOR,
DEPRECATED_WORKER,

/** RIB_COROUTINE_WORKER -> To be added on next releases */
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ import kotlinx.coroutines.supervisorScope
import kotlinx.coroutines.withContext

/** A manager or helper class bound to a [CoroutineScope] by using a binder like [bind]. */
public fun interface RibCoroutineWorker {
public fun interface RibCoroutineWorker : RibComponent {

/** Called when worker is started. Children coroutines can be launched in [scope]. */
public suspend fun onStart(scope: CoroutineScope)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,24 @@ public object RibEvents {
@JvmStatic
public val ribActionEvents: Observable<RibActionInfo> = mutableRibDurationEvents.asObservable()

private var areRibActionEmissionsAllowed = false

/**
* To be called before start observing/collecting on [ribActionEvents] (usually at your earliest
* application point)
*/
@JvmStatic
public fun allowRibActionEmissions() {
this.areRibActionEmissionsAllowed = true
}

/**
* @param eventType [RibEventType]
* @param child [Router]
* @param parent [Router] and null for the root ribs that are directly attached to
* RibActivity/Fragment
*/
@JvmStatic
public fun emitRouterEvent(eventType: RibEventType, child: Router<*>, parent: Router<*>?) {
psteiger marked this conversation as resolved.
Show resolved Hide resolved
FranAguilera marked this conversation as resolved.
Show resolved Hide resolved
mutableRouterEvents.tryEmit(RibRouterEvent(eventType, child, parent))
}
Expand All @@ -48,39 +60,48 @@ public object RibEvents {
* for each RIB component.
*
* @param ribAction The related RIB action type. e.g. didBecomeActive, willLoad, etc
* @param ribCallerClassType Related RIB component class type
* @param ribComponent Related RIB component
* @param ribComponentType The RIB component type (e.g. Interactor, Router, Presenter, Worker)
* @param ribEventType RIB event type (e.g. ATTACH/DETACH)
*/
internal inline fun <T : Any> triggerRibActionAndEmitEvents(
ribCallerClassType: T,
internal inline fun triggerRibActionAndEmitEvents(
ribComponent: RibComponent,
ribComponentType: RibComponentType,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to specify a property on the new RibComponent to be overriden, unfortunately given that Worker and RibCoroutineWorker are interfaces we cannot restrict visibility of those

ribEventType: RibEventType,
ribAction: () -> Unit,
) {
val ribClassName = ribCallerClassType.asQualifiedName()
ribClassName?.emitRibEventAction(ribComponentType, ribEventType, RibActionState.STARTED)
emitRibEventActionIfNeeded(ribComponent, ribComponentType, ribEventType, RibActionState.STARTED)
ribAction()
ribClassName?.emitRibEventAction(ribComponentType, ribEventType, RibActionState.COMPLETED)
emitRibEventActionIfNeeded(
ribComponent,
ribComponentType,
ribEventType,
RibActionState.COMPLETED,
)
}

private fun Any.asQualifiedName(): String? = javaClass.kotlin.qualifiedName

/**
* Emits emission of ATTACHED/DETACHED events for each RIB component.
*
* @param ribComponentType The RIB component type (e.g. Interactor, Router, Presenter)
* @param ribEventType RIB event type (e.g. ATTACH/DETACH)
* @param ribActionState: RibActionType,
*/
private fun String.emitRibEventAction(
private fun emitRibEventActionIfNeeded(
ribComponent: RibComponent,
ribComponentType: RibComponentType,
ribEventType: RibEventType,
ribActionState: RibActionState,
) {
if (!areRibActionEmissionsAllowed) {
// Unless specified explicitly via [RibEvents.allowRibActionEmissions] there is no need
// to create unnecessary objects if no one is observing/collecting RibAction events
return
}

val ribActionInfo =
RibActionInfo(
this,
ribComponent.javaClass.name,
ribComponentType,
ribEventType,
ribActionState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ protected constructor(
public open val interactor: I,
private val ribRefWatcher: RibRefWatcher,
private val mainThread: Thread,
) {
) : RibComponent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably too generic for a name for this set of callbacks. How about something more explicit around emitting callbacks around lifecycles, like RibEventEmitter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I like this better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to go with RibActionEmitter to avoid confusion with existing RibEventType

private val children: MutableList<Router<*>> = CopyOnWriteArrayList()
private val interactorGeneric: Interactor<*, *>
get() = interactor as Interactor<*, *>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import kotlinx.coroutines.CoroutineDispatcher
""",
replaceWith = ReplaceWith("RibCoroutineWorker"),
)
public interface Worker {
public interface Worker : RibComponent {

/**
* When overriden, will specify on which [CoroutineContext] the [Worker] will be bound via
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class InteractorAndRouterTest {
}
interactor = TestInteractor(childInteractor)
router = TestRouter(interactor, component)
RibEvents.allowRibActionEmissions()
}

@Test
Expand All @@ -67,7 +68,7 @@ class InteractorAndRouterTest {
RibEventType.ATTACHED,
RibComponentType.INTERACTOR,
RibActionState.COMPLETED,
"com.uber.rib.core.InteractorAndRouterTest.TestInteractor",
"com.uber.rib.core.InteractorAndRouterTest${'$'}TestInteractor",
)
verify(childInteractor).dispatchAttach(null)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.advanceUntilIdle
import kotlinx.coroutines.test.runTest
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
Expand Down Expand Up @@ -59,6 +60,11 @@ class WorkerBinderTest(private val adaptFromRibCoroutineWorker: Boolean) {
private val interactor = FakeInteractor<Presenter, Router<*>>()
private val ribActionInfoObserver = TestObserver<RibActionInfo>()

@Before
fun setUp() {
RibEvents.allowRibActionEmissions()
}

@Test
fun bind_whenInteractorAttached_shouldStartWorker() {
val lifecycle = BehaviorRelay.createDefault(InteractorEvent.ACTIVE)
Expand Down