Merge "Fix race in DisplayRepository tests" into main (1a912c79) · Commits · e / os / android_frameworks_base

packages/SystemUI/src/com/android/systemui/display/data/repository/DisplayRepository.kt

+38 −58

Original line number	Diff line number	Diff line
		@@ -28,7 +28,6 @@ import android.util.Log
		import android.view.Display
		import com.android.app.tracing.FlowTracing.traceEach
		import com.android.app.tracing.traceSection
		import com.android.systemui.Flags
		import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCallbackFlow
		import com.android.systemui.dagger.SysUISingleton
		import com.android.systemui.dagger.qualifiers.Background
		@@ -51,7 +50,6 @@ import kotlinx.coroutines.flow.map
		import kotlinx.coroutines.flow.onEach
		import kotlinx.coroutines.flow.onStart
		import kotlinx.coroutines.flow.scan
		import kotlinx.coroutines.flow.shareIn
		import kotlinx.coroutines.flow.stateIn

		/** Provides a [Flow] of [Display] as returned by [DisplayManager]. */
		@@ -102,7 +100,7 @@ constructor(
		private val displayManager: DisplayManager,
		@Background backgroundHandler: Handler,
		@Background bgApplicationScope: CoroutineScope,
		@Background backgroundCoroutineDispatcher: CoroutineDispatcher
		@Background backgroundCoroutineDispatcher: CoroutineDispatcher,
		) : DisplayRepository {
		private val allDisplayEvents: Flow<DisplayEvent> =
		conflatedCallbackFlow {
		@@ -139,17 +137,17 @@ constructor(
		override val displayAdditionEvent: Flow<Display?> =
		allDisplayEvents.filterIsInstance<DisplayEvent.Added>().map { getDisplay(it.displayId) }

		// TODO: b/345472038 - Delete after the flag is ramped up.
		private val oldEnabledDisplays: Flow<Set<Display>> =
		allDisplayEvents
		.map { getDisplays() }
		.shareIn(bgApplicationScope, started = SharingStarted.WhileSubscribed(), replay = 1)
		// This is necessary because there might be multiple displays, and we could
		// have missed events for those added before this process or flow started.
		// Note it causes a binder call from the main thread (it's traced).
		private val initialDisplays: Set<Display> =
		traceSection("$TAG#initialDisplays") { displayManager.displays?.toSet() ?: emptySet() }
		private val initialDisplayIds = initialDisplays.map { display -> display.displayId }.toSet()

		/** Propagate to the listeners only enabled displays */
		private val enabledDisplayIds: Flow<Set<Int>> =
		if (Flags.enableEfficientDisplayRepository()) {
		allDisplayEvents
		.scan(initial = emptySet()) { previousIds: Set<Int>, event: DisplayEvent ->
		.scan(initial = initialDisplayIds) { previousIds: Set<Int>, event: DisplayEvent ->
		val id = event.displayId
		when (event) {
		is DisplayEvent.Removed -> previousIds - id
		@@ -158,31 +156,19 @@ constructor(
		}
		}
		.distinctUntilChanged()
		.stateIn(
		bgApplicationScope,
		SharingStarted.WhileSubscribed(),
		// This is necessary because there might be multiple displays, and we could
		// have missed events for those added before this process or flow started.
		// Note it causes a binder call from the main thread (it's traced).
		getDisplays().map { display -> display.displayId }.toSet(),
		)
		} else {
		oldEnabledDisplays.map { enabledDisplaysSet ->
		enabledDisplaysSet.map { it.displayId }.toSet()
		}
		}
		.stateIn(bgApplicationScope, SharingStarted.WhileSubscribed(), initialDisplayIds)
		.debugLog("enabledDisplayIds")

		private val defaultDisplay by lazy {
		getDisplay(Display.DEFAULT_DISPLAY) ?: error("Unable to get default display.")
		}

		/**
		* Represents displays that went though the [DisplayListener.onDisplayAdded] callback.
		*
		* Those are commonly the ones provided by [DisplayManager.getDisplays] by default.
		*/
		private val enabledDisplays: Flow<Set<Display>> =
		if (Flags.enableEfficientDisplayRepository()) {
		enabledDisplayIds
		.mapElementsLazily { displayId -> getDisplay(displayId) }
		.onEach {
		@@ -198,11 +184,8 @@ constructor(
		// performance concerns.
		// Ultimately, this is a trade-off between a one-time UI thread binder call and
		// the constant overhead of sharedFlows.
		initialValue = getDisplays()
		initialValue = initialDisplays,
		)
		} else {
		oldEnabledDisplays
		}

		/**
		* Represents displays that went though the [DisplayListener.onDisplayAdded] callback.
		@@ -211,10 +194,7 @@ constructor(
		*/
		override val displays: Flow<Set<Display>> = enabledDisplays

		private fun getDisplays(): Set<Display> =
		traceSection("$TAG#getDisplays()") { displayManager.displays?.toSet() ?: emptySet() }

		private val _ignoredDisplayIds = MutableStateFlow<Set<Int>>(emptySet())
		val _ignoredDisplayIds = MutableStateFlow<Set<Int>>(emptySet())
		private val ignoredDisplayIds: Flow<Set<Int>> = _ignoredDisplayIds.debugLog("ignoredDisplayIds")

		private fun getInitialConnectedDisplays(): Set<Int> =
		@@ -271,7 +251,7 @@ constructor(
		// the flow starts being collected. This is to ensure the call to get displays (an
		// IPC) happens in the background instead of when this object
		// is instantiated.
		initialValue = emptySet()
		initialValue = emptySet(),
		)

		private val connectedExternalDisplayIds: Flow<Set<Int>> =
		@@ -308,7 +288,7 @@ constructor(
		TAG,
		"combining enabled=$enabledDisplaysIds, " +
		"connectedExternalDisplayIds=$connectedExternalDisplayIds, " +
		"ignored=$ignoredDisplayIds"
		"ignored=$ignoredDisplayIds",
		)
		}
		connectedExternalDisplayIds - enabledDisplaysIds - ignoredDisplayIds
		@@ -382,7 +362,7 @@ constructor(
		val previousSet: Set<T>,
		// Caches T values from the previousSet that were already converted to V
		val valueMap: Map<T, V>,
		val resultSet: Set<V>
		val resultSet: Set<V>,
		)

		val emptyInitialState = State(emptySet<T>(), emptyMap(), emptySet<V>())

packages/SystemUI/tests/src/com/android/systemui/display/data/repository/DisplayRepositoryTest.kt

+18 −12

Original line number	Diff line number	Diff line
		@@ -63,21 +63,27 @@ class DisplayRepositoryTest : SysuiTestCase() {
		private val defaultDisplay =
		display(type = TYPE_INTERNAL, id = DEFAULT_DISPLAY, state = Display.STATE_ON)

		private lateinit var displayRepository: DisplayRepositoryImpl

		@Before
		fun setup() {
		setDisplays(listOf(defaultDisplay))
		setAllDisplaysIncludingDisabled(DEFAULT_DISPLAY)
		displayRepository =
		// This is Lazy as displays could be set before the instance is created, and we want to verify
		// that the initial state (soon after construction) contains the expected ones set in every
		// test.
		private val displayRepository: DisplayRepositoryImpl by lazy {
		DisplayRepositoryImpl(
		displayManager,
		testHandler,
		TestScope(UnconfinedTestDispatcher()),
		UnconfinedTestDispatcher()
		UnconfinedTestDispatcher(),
		)
		.also {
		verify(displayManager, never()).registerDisplayListener(any(), any())
		verify(displayManager, never()).getDisplays(any())
		// It needs to be called, just once, for the initial value.
		verify(displayManager).getDisplays()
		}
		}

		@Before
		fun setup() {
		setDisplays(listOf(defaultDisplay))
		setAllDisplaysIncludingDisabled(DEFAULT_DISPLAY)
		}

		@Test
		@@ -502,7 +508,7 @@ class DisplayRepositoryTest : SysuiTestCase() {
		.registerDisplayListener(
		connectedDisplayListener.capture(),
		eq(testHandler),
		eq(DisplayManager.EVENT_FLAG_DISPLAY_CONNECTION_CHANGED)
		eq(DisplayManager.EVENT_FLAG_DISPLAY_CONNECTION_CHANGED),
		)
		return flowValue
		}
		@@ -522,7 +528,7 @@ class DisplayRepositoryTest : SysuiTestCase() {
		DisplayManager.EVENT_FLAG_DISPLAY_ADDED or
		DisplayManager.EVENT_FLAG_DISPLAY_CHANGED or
		DisplayManager.EVENT_FLAG_DISPLAY_REMOVED
		)
		),
		)
		}