diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/AGroupedImageListQuestAnswerFragment.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/AGroupedImageListQuestAnswerFragment.kt index 652b850e9e..59f4197373 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/quests/AGroupedImageListQuestAnswerFragment.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/quests/AGroupedImageListQuestAnswerFragment.kt @@ -9,14 +9,11 @@ import android.view.View import androidx.core.view.postDelayed import androidx.preference.PreferenceManager -import javax.inject.Inject - import de.westnordost.streetcomplete.R import de.westnordost.streetcomplete.databinding.QuestGenericListBinding import de.westnordost.streetcomplete.view.image_select.GroupableDisplayItem import de.westnordost.streetcomplete.view.image_select.GroupedImageSelectAdapter import kotlin.math.max -import kotlin.math.min /** * Abstract class for quests with a grouped list of images and one to select. @@ -35,7 +32,7 @@ abstract class AGroupedImageListQuestAnswerFragment : AbstractQuestFormAnsw protected abstract val allItems: List> protected abstract val topItems: List> - @Inject internal lateinit var favs: LastPickedValuesStore + internal lateinit var favs: LastPickedValuesStore> private val selectedItem get() = imageSelector.selectedItem @@ -43,7 +40,14 @@ abstract class AGroupedImageListQuestAnswerFragment : AbstractQuestFormAnsw override fun onAttach(ctx: Context) { super.onAttach(ctx) - favs = LastPickedValuesStore(PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext)) + val validSuggestions = allItems.mapNotNull { it.items }.flatten() + val stringToItem = validSuggestions.associateBy { it.value.toString() } + favs = LastPickedValuesStore( + PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext), + key = javaClass.simpleName, + serialize = { item -> item.value.toString() }, + deserialize = { value -> stringToItem[value] } + ) } override fun onCreate(savedInstanceState: Bundle?) { @@ -92,8 +96,7 @@ abstract class AGroupedImageListQuestAnswerFragment : AbstractQuestFormAnsw } private fun getInitialItems(): List> { - val validSuggestions = allItems.mapNotNull { it.items }.flatten() - return favs.getWeighted(javaClass.simpleName, 6, 30, topItems, validSuggestions) + return favs.get().mostCommonWithin(6, 30).padWith(topItems).toList() } override fun onClickOk() { @@ -114,14 +117,14 @@ abstract class AGroupedImageListQuestAnswerFragment : AbstractQuestFormAnsw .setMessage(R.string.quest_generic_item_confirmation) .setNegativeButton(R.string.quest_generic_confirmation_no, null) .setPositiveButton(R.string.quest_generic_confirmation_yes) { _, _ -> - favs.add(javaClass.simpleName, itemValue, allowDuplicates = true) + favs.add(item) onClickOk(itemValue) } .show() } } else { - favs.add(javaClass.simpleName, itemValue, allowDuplicates = true) + favs.add(item) onClickOk(itemValue) } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/AImageListQuestAnswerFragment.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/AImageListQuestAnswerFragment.kt index 13f9f01b23..41b571475a 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/quests/AImageListQuestAnswerFragment.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/quests/AImageListQuestAnswerFragment.kt @@ -11,7 +11,6 @@ import de.westnordost.streetcomplete.R import de.westnordost.streetcomplete.databinding.QuestGenericListBinding import de.westnordost.streetcomplete.view.image_select.DisplayItem import de.westnordost.streetcomplete.view.image_select.ImageSelectAdapter -import java.util.* import kotlin.collections.ArrayList /** @@ -34,7 +33,7 @@ abstract class AImageListQuestAnswerFragment : AbstractQuestFormAnswerFragm protected lateinit var imageSelector: ImageSelectAdapter - private lateinit var favs: LastPickedValuesStore + private lateinit var favs: LastPickedValuesStore> protected open val itemsPerRow = 4 /** return -1 for any number. Default: 1 */ @@ -52,7 +51,13 @@ abstract class AImageListQuestAnswerFragment : AbstractQuestFormAnswerFragm override fun onAttach(ctx: Context) { super.onAttach(ctx) - favs = LastPickedValuesStore(PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext)) + val stringToItem = items.associateBy { it.value.toString() } + favs = LastPickedValuesStore( + PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext), + key = javaClass.simpleName, + serialize = { item -> item.value.toString() }, + deserialize = { value -> stringToItem[value] } + ) } override fun onViewCreated(view: View, savedInstanceState: Bundle?) { @@ -89,8 +94,8 @@ abstract class AImageListQuestAnswerFragment : AbstractQuestFormAnswerFragm override fun onClickOk() { val values = imageSelector.selectedItems if (values.isNotEmpty()) { - favs.add(javaClass.simpleName, values) - onClickOk(values) + favs.add(values) + onClickOk(values.map { it.value!! }) } } @@ -106,7 +111,7 @@ abstract class AImageListQuestAnswerFragment : AbstractQuestFormAnswerFragm private fun moveFavouritesToFront(originalList: List>): List> { return if (originalList.size > itemsPerRow && moveFavoritesToFront) { - favs.moveLastPickedDisplayItemsToFront(javaClass.simpleName, originalList, originalList) + favs.get().filterNotNull().padWith(originalList).toList() } else { originalList } diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/LastPickedValuesStore.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/LastPickedValuesStore.kt index 527629b829..08ebcb993a 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/quests/LastPickedValuesStore.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/quests/LastPickedValuesStore.kt @@ -3,63 +3,48 @@ package de.westnordost.streetcomplete.quests import android.content.SharedPreferences import androidx.core.content.edit -import javax.inject.Inject - import de.westnordost.streetcomplete.Prefs -import de.westnordost.streetcomplete.view.image_select.DisplayItem -import de.westnordost.streetcomplete.view.image_select.GroupableDisplayItem -import kotlin.math.min - -/** T must be a string or enum - something that distinctly converts toString. */ -class LastPickedValuesStore @Inject constructor(private val prefs: SharedPreferences) { - fun add(key: String, newValues: Iterable, max: Int = MAX_ENTRIES, allowDuplicates: Boolean = false) { - val values = newValues.asSequence().map { it.toString() } + get(key) - val lastValues = if (allowDuplicates) values else values.distinct() +class LastPickedValuesStore( + private val prefs: SharedPreferences, + private val key: String, + private val serialize: (T) -> String, + private val deserialize: (String) -> T? // null = unwanted value, see mostCommonWithin +) { + fun add(newValues: Iterable) { + val lastValues = newValues.asSequence().map(serialize) + getRaw() prefs.edit { - putString(getKey(key), lastValues.take(max).joinToString(",")) + putString(getKey(), lastValues.take(MAX_ENTRIES).joinToString(",")) } } - fun add(key: String, value: T, max: Int = MAX_ENTRIES, allowDuplicates: Boolean = false) { - add(key, listOf(value), max, allowDuplicates) - } + fun add(value: T) = add(listOf(value)) - fun get(key: String): Sequence = - prefs.getString(getKey(key), null)?.splitToSequence(",") ?: sequenceOf() + fun get(): Sequence = getRaw().map(deserialize) - private fun getKey(key: String) = Prefs.LAST_PICKED_PREFIX + key + private fun getRaw(): Sequence = + prefs.getString(getKey(), null)?.splitToSequence(",") ?: sequenceOf() + + private fun getKey() = Prefs.LAST_PICKED_PREFIX + key } private const val MAX_ENTRIES = 100 -/* Returns `count` unique items, sorted by how often they appear in the last `historyCount` answers. - * If fewer than `count` unique items are found, look farther back in the history. - * Only returns items in `itemPool` ("valid"), although other answers count towards `historyCount`. - * If there are not enough unique items in the whole history, add unique `defaultItems` as needed. - * Always include the most recent answer, if it is in `itemPool`, but still sorted normally. So, if - * it is not one of the `count` most frequent items, it will replace the last of those. - * - * impl: null represents items not in the item pool +/* In the first `historyCount` items, return the `count` most-common non-null items, in order. + * If the first item is not included (and is not null), it replaces the last of the common items. + * If fewer than `count` unique items are found, continue counting items until that many are found, + * or the end of the sequence is reached. */ -fun LastPickedValuesStore.getWeighted( - key: String, - count: Int, - historyCount: Int, - defaultItems: List>, - itemPool: List> -): List> { - val stringToItem = itemPool.associateBy { it.value.toString() } - val lastPickedItems = get(key).map { stringToItem.get(it) } - val counts = lastPickedItems.countUniqueNonNull(historyCount, count) - val topRecent = counts.keys.sortedByDescending { counts.get(it) } - val latest = lastPickedItems.take(1).filterNotNull() - val items = (latest + topRecent + defaultItems).distinct().take(count) - return items.sortedByDescending { counts.get(it) }.toList() +fun Sequence.mostCommonWithin(count: Int, historyCount: Int): Sequence { + val counts = this.countUniqueNonNull(historyCount, count) + val top = counts.keys.sortedByDescending { counts.get(it) } + val latest = this.take(1).filterNotNull() + val items = (latest + top).distinct().take(count) + return items.sortedByDescending { counts.get(it) } } // Counts at least the first `minItems`, keeps going until it finds at least `target` unique values -private fun Sequence.countUniqueNonNull(minItems: Int, target: Int): Map { +private fun Sequence.countUniqueNonNull(minItems: Int, target: Int): Map { val counts = mutableMapOf() val items = takeAtLeastWhile(minItems) { counts.size < target }.filterNotNull() return items.groupingBy { it }.eachCountTo(counts) @@ -69,12 +54,5 @@ private fun Sequence.countUniqueNonNull(minItems: Int, target: Int): Map private fun Sequence.takeAtLeastWhile(count: Int, predicate: (T) -> Boolean): Sequence = withIndex().takeWhile{ (i, t) -> i < count || predicate(t) }.map { it.value } -fun LastPickedValuesStore.moveLastPickedDisplayItemsToFront( - key: String, - defaultItems: List>, - itemPool: List> -): List> { - val stringToItem = itemPool.associateBy { it.value.toString() } - val lastPickedItems = get(key).mapNotNull { stringToItem.get(it) } - return (lastPickedItems + defaultItems).distinct().toList() -} +fun Sequence.padWith(defaults: List, count: Int = defaults.size) = + (this + defaults).distinct().take(count) diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/building_levels/AddBuildingLevelsForm.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/building_levels/AddBuildingLevelsForm.kt index 3c199e206e..0a6cff2b7a 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/quests/building_levels/AddBuildingLevelsForm.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/quests/building_levels/AddBuildingLevelsForm.kt @@ -1,21 +1,21 @@ package de.westnordost.streetcomplete.quests.building_levels +import android.content.Context import android.os.Bundle import android.view.LayoutInflater import androidx.appcompat.app.AlertDialog import android.view.View import android.view.ViewGroup +import androidx.preference.PreferenceManager import androidx.recyclerview.widget.RecyclerView -import javax.inject.Inject - -import de.westnordost.streetcomplete.Injector import de.westnordost.streetcomplete.R import de.westnordost.streetcomplete.databinding.QuestBuildingLevelsBinding import de.westnordost.streetcomplete.databinding.QuestBuildingLevelsLastPickedButtonBinding import de.westnordost.streetcomplete.quests.AbstractQuestFormAnswerFragment import de.westnordost.streetcomplete.quests.LastPickedValuesStore import de.westnordost.streetcomplete.quests.AnswerItem +import de.westnordost.streetcomplete.quests.mostCommonWithin import de.westnordost.streetcomplete.util.TextChangedWatcher class AddBuildingLevelsForm : AbstractQuestFormAnswerFragment() { @@ -31,15 +31,24 @@ class AddBuildingLevelsForm : AbstractQuestFormAnswerFragment { it.levels }.thenBy { it.roofLevels } - ).toList() + favs.get() + .mostCommonWithin(5, 30) + .sortedWith(compareBy { it.levels }.thenBy { it.roofLevels }) + .toList() } - @Inject internal lateinit var favs: LastPickedValuesStore - - init { - Injector.applicationComponent.inject(this) + internal lateinit var favs: LastPickedValuesStore + + override fun onAttach(ctx: Context) { + super.onAttach(ctx) + favs = LastPickedValuesStore( + PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext), + key = javaClass.simpleName, + serialize = { item -> listOfNotNull(item.levels, item.roofLevels).joinToString("#") }, + deserialize = { value -> + value.split("#").let { BuildingLevelsAnswer(it[0].toInt(), it.getOrNull(1)?.toInt()) } + } + ) } override fun onViewCreated(view: View, savedInstanceState: Bundle?) { @@ -64,7 +73,7 @@ class AddBuildingLevelsForm : AbstractQuestFormAnswerFragment(private val maxSelectableIndices: Int = -1) : val listeners: MutableList = CopyOnWriteArrayList() - val selectedItems get() = _selectedIndices.map { i -> items[i].value!! } + val selectedItems get() = _selectedIndices.map { i -> items[i] } interface OnItemSelectionListener { fun onIndexSelected(index: Int) diff --git a/app/src/test/java/de/westnordost/streetcomplete/quests/LastPickedValuesStoreTest.kt b/app/src/test/java/de/westnordost/streetcomplete/quests/LastPickedValuesStoreTest.kt index 827e166916..cde8b89cb7 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/quests/LastPickedValuesStoreTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/quests/LastPickedValuesStoreTest.kt @@ -1,74 +1,46 @@ package de.westnordost.streetcomplete.quests import de.westnordost.streetcomplete.quests.Letter.* -import de.westnordost.streetcomplete.testutils.mock -import de.westnordost.streetcomplete.testutils.on -import de.westnordost.streetcomplete.view.image_select.Item import org.junit.Assert.assertEquals -import org.junit.Before import org.junit.Test class LastPickedValuesStoreTest { - private lateinit var favs: LastPickedValuesStore - private val key: String = javaClass.simpleName - - private val allItems = Letter.values().toList().toItems() - private val defaultItems = listOf(X, Y, Z).toItems() - - @Before fun setUp() { - favs = mock() - } - - @Test fun `weighted sort returns the default items when there is no history`() { - on(favs.get(key)).thenReturn(sequenceOf()) - val returnedItems = favs.getWeighted(key, 4, 99, defaultItems, allItems) - assertEquals(defaultItems, returnedItems) + @Test fun `mostCommonWithin sorts by frequency first, then recency`() { + val items = sequenceOf(A, C, B, B, C, D) + assertEquals(items.mostCommonWithin(4, 99).toList(), listOf(C, B, A, D)) } - @Test fun `weighted sort considers frequency first, then recency, then defaults`() { - on(favs.get(key)).thenReturn(sequenceOf("A", "C", "B", "B", "C", "D")) - val returnedItems = favs.getWeighted(key, 6, 99, defaultItems, allItems) - val expectedItems = listOf(C, B, A, D, X, Y).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `mostCommonWithin includes the most recent item even if it is not the most common`() { + val items = sequenceOf(A, B, B, B, C, C) + assertEquals(items.mostCommonWithin(2, 99).toList(), listOf(B, A)) } - @Test fun `weighted sort returns most recent item even if it is not the most picked`() { - on(favs.get(key)).thenReturn(sequenceOf("A", "B", "B", "B", "C", "C")) - val returnedItems = favs.getWeighted(key, 2, 99, defaultItems, allItems) - val expectedItems = listOf(B, A).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `mostCommonWithin doesn't return duplicates`() { + val items = sequenceOf(A, B, A, B) + assertEquals(items.mostCommonWithin(4, 99).toList(), listOf(A, B)) } - @Test fun `weighted sort doesn't return duplicates`() { - on(favs.get(key)).thenReturn(sequenceOf("X", "Y", "X", "A")) - val returnedItems = favs.getWeighted(key, 4, 99, defaultItems, allItems) - val expectedItems = listOf(X, Y, A, Z).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `mostCommonWithin doesn't include the first item if it's null`() { + val items = sequenceOf(null, B, null, C) + assertEquals(items.mostCommonWithin(2, 99).toList(), listOf(B, C)) } - @Test fun `weighted sort only returns items in itemPool (the most recent is not exempt)`() { - on(favs.get(key)).thenReturn(sequenceOf("p", "B", "q", "C")) - val returnedItems = favs.getWeighted(key, 2, 99, defaultItems, allItems) - val expectedItems = listOf(B, C).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `mostCommonWithin includes nulls in the number of items to count`() { + val items = sequenceOf(A, null, null, B, /* stops here */ B, C, D) + assertEquals(items.mostCommonWithin(2, 4).toList(), listOf(A, B)) } - @Test fun `weighted sort still counts non-itemPool values in the history window`() { - on(favs.get(key)).thenReturn(sequenceOf("A", "p", "q", "B", /**/ "B", "C", "D")) - val returnedItems = favs.getWeighted(key, 2, 4, defaultItems, allItems) - val expectedItems = listOf(A, B).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `mostCommonWithin keeps counting until enough non-null items have been found`() { + val items = sequenceOf(A, null, null, B, B, C, /* stops here */ D) + assertEquals(items.mostCommonWithin(3, 4).toList(), listOf(B, A, C)) } - @Test fun `weighted sort extends the history window (only) as needed to find enough items`() { - on(favs.get(key)).thenReturn(sequenceOf("A", "p", "q", "B", "B", "C", /**/ "D")) - val returnedItems = favs.getWeighted(key, 3, 4, defaultItems, allItems) - val expectedItems = listOf(B, A, C).toItems() - assertEquals(expectedItems, returnedItems) + @Test fun `padWith doesn't include duplicates`() { + val items = sequenceOf(A, B).padWith(listOf(B, C, D, A)) + assertEquals(items.toList(), listOf(A, B, C, D)) } } -private enum class Letter { A, B, C, D, X, Y, Z } -private fun List.toItems(): List> = this.map(::Item) +private enum class Letter { A, B, C, D }