Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 315851d8 authored by Fabián Kozynski's avatar Fabián Kozynski
Browse files

Fix race condition in TileSpecRepository 

Before this change, after a factory reset, the list of tiles will be
empty. While CurrentTilesInteractor was processing the first emit of the
flow (default tiles), AutoAdd will try to add tiles to whatever was in
Settings (empty list), causing only those tiles to be present.

This change introduces a local cache in TileSpecSettingsRepository so
changes are applied to its value (last known value).

Test: manual, check tiles after factory reset
Test: atest TileSpecSettingsRepositoryTest
CurrentTilesInteractorImplTest
Fixes 289003025

Change-Id: I05c25ffa9206c98533647f641184dcaa819925e7
parent 309528d9

packages/SystemUI/src/com/android/systemui/qs/TEST_MAPPING

+11 −0
Original line number Diff line number Diff line
@@ -10,6 +10,17 @@ 
          "exclude-annotation": "androidx.test.filters.FlakyTest"

        }

      ]

    },

    {

      "name": "QuickSettingsDeviceResetTests",

      "options": [

          {

            "exclude-annotation": "org.junit.Ignore"

          },

          {

            "exclude-annotation": "androidx.test.filters.FlakyTest"

          }

      ]

    }

  ]

}
 
 No newline at end of file

packages/SystemUI/src/com/android/systemui/qs/pipeline/data/repository/TileSpecRepository.kt

+18 −14
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import android.annotation.UserIdInt 
import android.content.res.Resources

import android.database.ContentObserver

import android.provider.Settings

import android.util.SparseArray

import com.android.systemui.R

import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCallbackFlow

import com.android.systemui.dagger.SysUISingleton
@@ -107,6 +108,7 @@ constructor( 
) : TileSpecRepository {



    private val mutex = Mutex()

    private val tileSpecsPerUser = SparseArray<List<TileSpec>>()



    private val retailModeTiles by lazy {

        resources
@@ -142,10 +144,12 @@ constructor( 
                awaitClose { secureSettings.unregisterContentObserver(observer) }

            }

            .onStart { emit(Unit) }

            .map { secureSettings.getStringForUser(SETTING, userId) ?: "" }

            .distinctUntilChanged()

            .map { loadTiles(userId) }

            .onEach { logger.logTilesChangedInSettings(it, userId) }

            .map { parseTileSpecs(it, userId) }

            .distinctUntilChanged()

            .map { parseTileSpecs(it, userId).also { storeTiles(userId, it) } }

            .distinctUntilChanged()

            .onEach { mutex.withLock { tileSpecsPerUser.put(userId, it) } }

            .flowOn(backgroundDispatcher)

    }
@@ -154,7 +158,7 @@ constructor( 
            if (tile == TileSpec.Invalid) {

                return

            }

            val tilesList = loadTiles(userId).toMutableList()

            val tilesList = tileSpecsPerUser.get(userId, emptyList()).toMutableList()

            if (tile !in tilesList) {

                if (position < 0 || position >= tilesList.size) {

                    tilesList.add(tile)
@@ -162,6 +166,7 @@ constructor( 
                    tilesList.add(position, tile)

                }

                storeTiles(userId, tilesList)

                tileSpecsPerUser.put(userId, tilesList)

            }

        }
@@ -170,9 +175,10 @@ constructor( 
            if (tiles.all { it == TileSpec.Invalid }) {

                return

            }

            val tilesList = loadTiles(userId).toMutableList()

            val tilesList = tileSpecsPerUser.get(userId, emptyList()).toMutableList()

            if (tilesList.removeAll(tiles)) {

                storeTiles(userId, tilesList.toList())

                tileSpecsPerUser.put(userId, tilesList)

            }

        }
@@ -181,15 +187,7 @@ constructor( 
            val filtered = tiles.filter { it != TileSpec.Invalid }

            if (filtered.isNotEmpty()) {

                storeTiles(userId, filtered)

            }

        }



    private suspend fun loadTiles(@UserIdInt forUser: Int): List<TileSpec> {

        return withContext(backgroundDispatcher) {

            (secureSettings.getStringForUser(SETTING, forUser) ?: "")

                .split(DELIMITER)

                .map(TileSpec::create)

                .filter { it !is TileSpec.Invalid }

                tileSpecsPerUser.put(userId, tiles)

            }

        }
@@ -214,6 +212,12 @@ constructor( 
        }

    }



    private suspend fun loadTiles(userId: Int): String {

        return withContext(backgroundDispatcher) {

            secureSettings.getStringForUser(SETTING, userId) ?: ""

        }

    }



    private fun parseTileSpecs(tilesFromSettings: String, user: Int): List<TileSpec> {

        val fromSettings =

            tilesFromSettings.split(DELIMITER).map(TileSpec::create).filter {

packages/SystemUI/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractor.kt

+5 −1
Original line number Diff line number Diff line
@@ -56,6 +56,8 @@ import kotlinx.coroutines.flow.asStateFlow 
import kotlinx.coroutines.flow.collectLatest

import kotlinx.coroutines.flow.combine

import kotlinx.coroutines.flow.distinctUntilChanged

import kotlinx.coroutines.flow.filter

import kotlinx.coroutines.flow.first

import kotlinx.coroutines.flow.flatMapLatest

import kotlinx.coroutines.flow.flowOn

import kotlinx.coroutines.flow.map
@@ -273,7 +275,9 @@ constructor( 
    }



    override fun addTile(spec: TileSpec, position: Int) {

        scope.launch {

        scope.launch(backgroundDispatcher) {

            // Block until the list is not empty

            currentTiles.filter { it.isNotEmpty() }.first()

            tileSpecRepository.addTile(userRepository.getSelectedUserInfo().id, spec, position)

        }

    }

packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/data/repository/TileSpecSettingsRepositoryTest.kt

+13 −1
Original line number Diff line number Diff line
@@ -32,6 +32,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi 
import kotlinx.coroutines.coroutineScope

import kotlinx.coroutines.test.StandardTestDispatcher

import kotlinx.coroutines.test.TestScope

import kotlinx.coroutines.test.runCurrent

import kotlinx.coroutines.test.runTest

import org.junit.Before

import org.junit.Test
@@ -372,12 +373,23 @@ class TileSpecSettingsRepositoryTest : SysuiTestCase() { 
            assertThat(loadTilesForUser(0)).isNull()

        }



    @Test

    fun emptyTilesReplacedByDefaultInSettings() =

        testScope.runTest {

            val tiles by collectLastValue(underTest.tilesSpecs(0))

            runCurrent()



            assertThat(loadTilesForUser(0))

                .isEqualTo(getDefaultTileSpecs().map { it.spec }.joinToString(","))

        }



    private fun getDefaultTileSpecs(): List<TileSpec> {

        return QSHost.getDefaultSpecs(context.resources).map(TileSpec::create)

    }



    private fun storeTilesForUser(specs: String, forUser: Int) {

    private fun TestScope.storeTilesForUser(specs: String, forUser: Int) {

        secureSettings.putStringForUser(SETTING, specs, forUser)

        runCurrent()

    }



    private fun loadTilesForUser(forUser: Int): String? {

packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractorImplTest.kt

+16 −0
Original line number Diff line number Diff line
@@ -648,6 +648,22 @@ class CurrentTilesInteractorImplTest : SysuiTestCase() { 
            assertThat(tiles!![1].spec).isEqualTo(CUSTOM_TILE_SPEC)

        }



    @Test

    fun tileAddedOnEmptyList_blocked() =

        testScope.runTest(USER_INFO_0) {

            val tiles by collectLastValue(underTest.currentTiles)

            val specs = listOf(TileSpec.create("a"), TileSpec.create("b"))

            val newTile = TileSpec.create("c")



            underTest.addTile(newTile)



            assertThat(tiles!!.isEmpty()).isTrue()



            tileSpecRepository.setTiles(USER_INFO_0.id, specs)



            assertThat(tiles!!.size).isEqualTo(3)

        }



    private fun QSTile.State.fillIn(state: Int, label: CharSequence, secondaryLabel: CharSequence) {

        this.state = state

        this.label = label