From 54c8426f8c266553fb53d82f2c4cbdf53ce3532b Mon Sep 17 00:00:00 2001 From: shaiguelman <42308480+shaiguelman@users.noreply.github.com> Date: Thu, 21 Jul 2022 06:36:37 -0500 Subject: [PATCH] Fix crash during deck filtering (#11880) Fixes a `ConcurrentModificationException`: the wrong list was modified * Create copy of list when filtering * Optimize containsFilterString * Add error catching * Remove whitespace * Add comment and better naming * Rename usage of childrenCopy variable I forgot to rename * Change to more efficient solution * Remove unecessary comment * Add unit tests for DeckFilter * Remove unused imports * Add copywrite * Remove whitespace * Format fixes * Add last name to dictionary * Flipped actual and expected in assert statement * Removed try/catch block in publishResults() * Remove unused import * Added null check * Revert "Optimize containsFilterString" This reverts commit 5b617b03f694a40db9082c0bd3b73dea6aa4df7e. * Reverting to using !! operator on results * Rename test method --- .idea/dictionaries/usernames.xml | 1 + .../com/ichi2/anki/widgets/DeckAdapter.kt | 6 +- .../main/java/com/ichi2/utils/TypedFilter.kt | 3 +- .../com/ichi2/anki/DeckAdapterFilterTest.kt | 92 +++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 AnkiDroid/src/test/java/com/ichi2/anki/DeckAdapterFilterTest.kt diff --git a/.idea/dictionaries/usernames.xml b/.idea/dictionaries/usernames.xml index d349d8f32497..61d5e900f64b 100644 --- a/.idea/dictionaries/usernames.xml +++ b/.idea/dictionaries/usernames.xml @@ -20,6 +20,7 @@ Freidle Goel Gruber + Guelman Houssam Jadhav Jolta diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt b/AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt index 8c80ff06832c..c051c270fc96 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt @@ -25,6 +25,7 @@ import android.view.View import android.view.View.OnLongClickListener import android.view.ViewGroup import android.widget.* +import androidx.annotation.VisibleForTesting import androidx.core.content.ContextCompat import androidx.recyclerview.widget.RecyclerView import com.ichi2.anki.R @@ -343,7 +344,8 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context) return DeckFilter() } - private inner class DeckFilter : TypedFilter>(mDeckList) { + @VisibleForTesting + inner class DeckFilter(deckList: List> = mDeckList) : TypedFilter>(deckList) { override fun filterResults(constraint: CharSequence, items: List>): List> { val filterPattern = constraint.toString().lowercase(Locale.getDefault()).trim { it <= ' ' } return items.mapNotNull { t: TreeNode -> filterDeckInternal(filterPattern, t) } @@ -375,7 +377,7 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context) // we have a root, and a list of trees with the counts already calculated. return TreeNode(root.value).apply { - children.addAll(ret) + this.children.addAll(ret) } } diff --git a/AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt b/AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt index 124b5b4e4289..ac5adf22d74b 100644 --- a/AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt +++ b/AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt @@ -49,8 +49,7 @@ abstract class TypedFilter(private val getCurrentItems: (() -> List)) : Fi @Suppress("UNCHECKED_CAST") override fun publishResults(constraint: CharSequence?, results: FilterResults?) { - // this is only ever called from performFiltering so we can guarantee the value is non-null - // and can be cast to List + // this is only ever called from performFiltering so we can guarantee the value can be cast to List val list = results!!.values as List publishResults(constraint, list) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/DeckAdapterFilterTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/DeckAdapterFilterTest.kt new file mode 100644 index 000000000000..2cda57d2bb92 --- /dev/null +++ b/AnkiDroid/src/test/java/com/ichi2/anki/DeckAdapterFilterTest.kt @@ -0,0 +1,92 @@ +/**************************************************************************************** + * Copyright (c) 2022 Shai Guelman * + * * + * This program is free software; you can redistribute it and/or modify it under * + * the terms of the GNU General Public License as published by the Free Software * + * Foundation; either version 3 of the License, or (at your option) any later * + * version. * + * * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY * + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A * + * PARTICULAR PURPOSE. See the GNU General Public License for more details. * + * * + * You should have received a copy of the GNU General Public License along with * + * this program. If not, see . * + ****************************************************************************************/ +package com.ichi2.anki + +import com.ichi2.anki.widgets.DeckAdapter +import com.ichi2.libanki.sched.AbstractDeckTreeNode +import com.ichi2.libanki.sched.DeckTreeNode +import com.ichi2.libanki.sched.TreeNode +import org.junit.Assert +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.MockitoAnnotations + +class DeckAdapterFilterTest { + + @Mock + private lateinit var adapter: DeckAdapter + + private lateinit var filter: DeckAdapter.DeckFilter + + @Before + fun setUp() { + MockitoAnnotations.openMocks(this) + filter = adapter.DeckFilter(deckList) + } + + @Test + fun verifyFilterResultsReturnsCorrectList() { + val pattern = "Math" + + val actual = filter.filterResults(pattern, deckList) + val expected = deckList.getByDids(0, 4, 5, 6, 8) + + Assert.assertEquals(expected, actual) + } + + @Test + fun verifyFilterResultsReturnsEmptyForNoMatches() { + val deckList = deckList + val pattern = "geometry" + + val actual = filter.filterResults(pattern, deckList) + + Assert.assertTrue(actual.isEmpty()) + } + + private val deckList: MutableList> + get() { + val deckList: MutableList> = mutableListOf( + TreeNode(DeckTreeNode("Chanson", 0)), + TreeNode(DeckTreeNode("Chanson::A Vers", 1)), + TreeNode(DeckTreeNode("Chanson::A Vers::1", 2)), + TreeNode(DeckTreeNode("Chanson::A Vers::Other", 3)), + TreeNode(DeckTreeNode("Chanson::Math HW", 4)), + TreeNode(DeckTreeNode("Chanson::Math HW::Theory", 5)), + TreeNode(DeckTreeNode("Chanson::Important", 6)), + TreeNode(DeckTreeNode("Chanson::Important::Stuff", 7)), + TreeNode(DeckTreeNode("Chanson::Important::Math", 8)), + TreeNode(DeckTreeNode("Chanson::Important::Stuff::Other Stuff", 9)), + ) + + deckList.getByDid(0).children.addAll(deckList.getByDids(1, 4, 6)) + deckList.getByDid(1).children.addAll(deckList.getByDids(2, 3)) + deckList.getByDid(4).children.addAll(deckList.getByDids(5)) + deckList.getByDid(6).children.addAll(deckList.getByDids(7, 8)) + deckList.getByDid(7).children.addAll(deckList.getByDids(9)) + + return deckList + } + + private fun List>.getByDid(did: Long): TreeNode { + return this.first { it.value.did == did } + } + + private fun List>.getByDids(vararg dids: Long): List> { + return this.filter { it.value.did in dids } + } +}