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

Fix crash during deck filtering #11880

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
1 change: 1 addition & 0 deletions .idea/dictionaries/usernames.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<w>Freidle</w>
<w>Goel</w>
<w>Gruber</w>
<w>Guelman</w>
<w>Houssam</w>
<w>Jadhav</w>
<w>Jolta</w>
Expand Down
8 changes: 5 additions & 3 deletions AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -343,7 +344,8 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
return DeckFilter()
}

private inner class DeckFilter : TypedFilter<TreeNode<AbstractDeckTreeNode>>(mDeckList) {
@VisibleForTesting
inner class DeckFilter(deckList: List<TreeNode<AbstractDeckTreeNode>> = mDeckList) : TypedFilter<TreeNode<AbstractDeckTreeNode>>(deckList) {
override fun filterResults(constraint: CharSequence, items: List<TreeNode<AbstractDeckTreeNode>>): List<TreeNode<AbstractDeckTreeNode>> {
val filterPattern = constraint.toString().lowercase(Locale.getDefault()).trim { it <= ' ' }
return items.mapNotNull { t: TreeNode<AbstractDeckTreeNode> -> filterDeckInternal(filterPattern, t) }
Expand Down Expand Up @@ -375,13 +377,13 @@ 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)
shaiguelman marked this conversation as resolved.
Show resolved Hide resolved
}
}

private fun containsFilterString(filterPattern: String, root: AbstractDeckTreeNode): Boolean {
val deckName = root.fullDeckName
return deckName.lowercase(Locale.getDefault()).contains(filterPattern) || deckName.lowercase(Locale.ROOT).contains(filterPattern)
return deckName.contains(filterPattern, ignoreCase = true)
shaiguelman marked this conversation as resolved.
Show resolved Hide resolved
david-allison marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
9 changes: 8 additions & 1 deletion AnkiDroid/src/main/java/com/ichi2/utils/TypedFilter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.ichi2.utils

import android.widget.Filter
import timber.log.Timber

/** Implementation of [Filter] which is strongly typed */
abstract class TypedFilter<T>(private val getCurrentItems: (() -> List<T>)) : Filter() {
Expand Down Expand Up @@ -51,7 +52,13 @@ abstract class TypedFilter<T>(private val getCurrentItems: (() -> List<T>)) : Fi
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<T>
shaiguelman marked this conversation as resolved.
Show resolved Hide resolved
val list = results!!.values as List<T>
val list = try {
Copy link
Member

Choose a reason for hiding this comment

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

I think an explicit call to if results == null would be better. Try are more costly than conditional in general, and anyway clearer.

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've since reverted this code since David said it was better to have it crash

Copy link
Contributor Author

@shaiguelman shaiguelman Jul 19, 2022

Choose a reason for hiding this comment

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

I changed it to use a nullable to avoid an if check

Copy link
Member

@david-allison david-allison Jul 19, 2022

Choose a reason for hiding this comment

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

I think I'd prefer a crash here (AKA no change), as it's an assertion which doesn't hold, but I'll leave the final decision to you. Implementer's choice, I'm happy either way =)

I think the answer to the question of 'what do we do if we get a null' is hard to pin down: do we want all results, or no results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that's a good question. I did no results since that was simpler to implement but it's not necessarily the best UX choice. It would require a sizeable refactor though to return all results.

I think since we're getting a little outside the scope of the PR I'll revert it to original for now and leave it for a non-urgent issue.

results!!.values as List<T>
} catch (npe: NullPointerException) {
// allow publishResults to fail gracefully while reporting error
Timber.e(npe)
listOf()
}
david-allison marked this conversation as resolved.
Show resolved Hide resolved
publishResults(constraint, list)
}

Expand Down
92 changes: 92 additions & 0 deletions AnkiDroid/src/test/java/com/ichi2/anki/DeckAdapterFilterTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/****************************************************************************************
* Copyright (c) 2022 Shai Guelman <[email protected]> *
* *
* 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 <http://www.gnu.org/licenses/>. *
****************************************************************************************/
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 {
david-allison marked this conversation as resolved.
Show resolved Hide resolved

@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(actual, expected)
}

@Test
fun verifyFilterResultsReturnsNullWhenForNoMatches() {
val deckList = deckList
val pattern = "geometry"

val actual = filter.filterResults(pattern, deckList)

Assert.assertTrue(actual.isEmpty())
}

private val deckList: MutableList<TreeNode<AbstractDeckTreeNode>>
get() {
val deckList: MutableList<TreeNode<AbstractDeckTreeNode>> = mutableListOf(
TreeNode(DeckTreeNode("Chanson", 0)),
shaiguelman marked this conversation as resolved.
Show resolved Hide resolved
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<TreeNode<AbstractDeckTreeNode>>.getByDid(did: Long): TreeNode<AbstractDeckTreeNode> {
return this.first { it.value.did == did }
}

private fun List<TreeNode<AbstractDeckTreeNode>>.getByDids(vararg dids: Long): List<TreeNode<AbstractDeckTreeNode>> {
return this.filter { it.value.did in dids }
}
}