Skip to content

Commit

Permalink
Remove suspended code from DeckAdapter
Browse files Browse the repository at this point in the history 
The suspended code was removed and a similar implementation was done
in DeckPicker which will now do the necessary work and then will pass the
calculated data to the adapter.

The adapter was changed to extend ListAdapter which uses diff-ing to
observe changes so we get free animations. The filtering was removed
from the adapter, DeckPicker computes the data as it has all info needed,
the query and the deck tree. The adapter becomes dumb and just reacts to
the changes. A bit wasteful to start a coroutine for each change in the
query text, but I wanted to keep the change count small.

Note: the methods findDeckPosition() and getNodeByDid() were moved in
DeckPicker, they are not needed by the adapter and the DeckPicker already
has a reference to the deck tree that it can use in the methods.
  • Loading branch information
@lukstbit
lukstbit committed Jan 8, 2025
1 parent 2dbbaa9 commit e1c40cb
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 112 deletions.
79 changes: 54 additions & 25 deletions AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import android.view.Menu
import android.view.MenuItem
import android.view.View
import android.view.ViewPropertyAnimator
import android.widget.Filterable
import android.widget.ImageView
import android.widget.LinearLayout
import android.widget.RelativeLayout
Expand Down Expand Up @@ -981,21 +980,17 @@ open class DeckPicker :
}

override fun onQueryTextChange(newText: String): Boolean {
val adapter = recyclerView.adapter as? Filterable
if (adapter == null || adapter.filter == null) {
Timber.w(
"DeckPicker.onQueryTextChange: adapter is null: %s, filter is null: %s, adapter type: %s",
adapter == null,
adapter?.filter == null,
adapter?.javaClass?.simpleName ?: "Unknown",
)
CrashReportService.sendExceptionReport(
Exception("DeckPicker.onQueryTextChanged with unexpected null adapter or filter. Carefully examine logcat"),
"DeckPicker",
)
return true
val adapter = recyclerView.adapter as DeckAdapter
launchCatchingTask {
val selectedDeckId = withCol { decks.current().optLong("id") }
dueTree?.let {
adapter.submit(
data = it.filterAndFlatten(newText),
hasSubDecks = it.children.any { deckNode -> deckNode.children.any() },
currentDeckId = selectedDeckId,
)
}
}
adapter.filter.filter(newText)
return true
}
},
Expand Down Expand Up @@ -2136,12 +2131,12 @@ open class DeckPicker :
promptUserToUpdateScheduler()
return
}

withCol { decks.select(did) }
deckListAdapter.updateSelectedDeck(did)
// Also forget the last deck used by the Browser
CardBrowser.clearLastDeckId()
viewModel.focusedDeck = did
val deck = deckListAdapter.getNodeByDid(did)
val deck = getNodeByDid(did)
if (deck.hasCardsReadyToStudy()) {
openReviewerOrStudyOptions(selectionType)
return
Expand Down Expand Up @@ -2169,10 +2164,30 @@ open class DeckPicker :
* @param did The deck ID of the deck to select.
*/
private fun scrollDecklistToDeck(did: DeckId) {
val position = deckListAdapter.findDeckPosition(did)
val position = findDeckPosition(did)
recyclerViewLayoutManager.scrollToPositionWithOffset(position, recyclerView.height / 2)
}

/**
* Return the position of the deck in the deck list. If the deck is a child of a collapsed deck
* (i.e., not visible in the deck list), then the position of the parent deck is returned instead.
*
* An invalid deck ID will return position 0.
*/
private fun findDeckPosition(did: DeckId): Int {
deckListAdapter.currentList.forEachIndexed { index, treeNode ->
if (treeNode.did == did) {
return index
}
}

// If the deck is not in our list, we search again using the immediate parent
// If the deck is not found, return 0
val collapsedDeck = dueTree?.find(did) ?: return 0
val parent = collapsedDeck.parent?.get() ?: return 0
return findDeckPosition(parent.did)
}

/**
* Launch an asynchronous task to rebuild the deck list and recalculate the deck counts. Use this
* after any change to a deck (e.g., rename, importing, add/delete) that needs to be reflected
Expand Down Expand Up @@ -2282,20 +2297,26 @@ open class DeckPicker :
}
}
}
val currentFilter = if (toolbarSearchView != null) toolbarSearchView!!.query else null
val currentFilter = toolbarSearchView?.query

if (isEmpty) {
if (supportActionBar != null) {
supportActionBar!!.subtitle = null
}
supportActionBar?.subtitle = null
if (toolbarSearchView != null) {
deckListAdapter.filter?.filter(currentFilter)
deckListAdapter.submit(
data = emptyList(),
hasSubDecks = false,
currentDeckId = -1,
)
}
Timber.d("Not rendering deck list as there are no cards")
// We're done here
return
}
deckListAdapter.buildDeckList(tree, currentFilter)
deckListAdapter.submit(
data = tree.filterAndFlatten(currentFilter),
hasSubDecks = tree.children.any { it.children.any() },
currentDeckId = withCol { decks.current().optLong("id") },
)

// Set the "x due" subtitle
runCatchingKotlin {
Expand All @@ -2315,6 +2336,11 @@ open class DeckPicker :
}
}

/**
* Get the [DeckNode] identified by [did] from [DeckAdapter].
*/
private fun DeckPicker.getNodeByDid(did: DeckId): DeckNode = deckListAdapter.currentList[findDeckPosition(did)]

// Callback to show study options for currently selected deck
fun showContextMenuDeckOptions(did: DeckId) {
// open deck options
Expand Down Expand Up @@ -2654,7 +2680,10 @@ open class DeckPicker :
withCol { sched.hasCardsTodayAfterStudyAheadLimit() } -> CompletedDeckStatus.LEARN_AHEAD_LIMIT_REACHED
withCol { sched.newDue() || sched.revDue() } -> CompletedDeckStatus.LEARN_AHEAD_LIMIT_REACHED
withCol { decks.isFiltered(did) } -> CompletedDeckStatus.DYNAMIC_DECK_NO_LIMITS_REACHED
deckListAdapter.getNodeByDid(did).children.isEmpty() && withCol { decks.isEmpty(did) } -> CompletedDeckStatus.EMPTY_REGULAR_DECK
getNodeByDid(did).children.isEmpty() &&
withCol {
decks.isEmpty(did)
} -> CompletedDeckStatus.EMPTY_REGULAR_DECK
else -> CompletedDeckStatus.REGULAR_DECK_NO_MORE_CARDS_TODAY
}

Expand Down
142 changes: 55 additions & 87 deletions AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckAdapter.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,21 @@ import android.graphics.drawable.Drawable
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.widget.Filter
import android.widget.Filterable
import android.widget.ImageButton
import android.widget.LinearLayout
import android.widget.RelativeLayout
import android.widget.TextView
import androidx.annotation.CheckResult
import androidx.annotation.VisibleForTesting
import androidx.core.content.res.getDrawableOrThrow
import androidx.recyclerview.widget.DiffUtil
import androidx.recyclerview.widget.ListAdapter
import androidx.recyclerview.widget.RecyclerView
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.anki.OnContextAndLongClickListener.Companion.setOnContextAndLongClickListener
import com.ichi2.anki.R
import com.ichi2.anki.utils.ext.findViewById
import com.ichi2.libanki.DeckId
import com.ichi2.libanki.sched.DeckNode
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import net.ankiweb.rsdroid.RustCleanup
import timber.log.Timber

/**
* A [RecyclerView.Adapter] used to show the list of decks inside [com.ichi2.anki.DeckPicker].
Expand All @@ -55,7 +49,6 @@ import timber.log.Timber
* @param onDeckContextRequested callback triggered when the user requested to see extra actions for
* a deck. This consists in a context menu brought in by either a long touch or a right click.
*/
@RustCleanup("Lots of bad code: should not be using suspend functions inside an adapter")
@RustCleanup("Differs from legacy backend: Create deck 'One', create deck 'One::two'. 'One::two' was not expanded")
class DeckAdapter(
context: Context,
Expand All @@ -64,13 +57,8 @@ class DeckAdapter(
private val onDeckCountsSelected: (DeckId) -> Unit,
private val onDeckChildrenToggled: (DeckId) -> Unit,
private val onDeckContextRequested: (DeckId) -> Unit,
) : RecyclerView.Adapter<DeckAdapter.ViewHolder>(),
Filterable {
) : ListAdapter<DeckNode, DeckAdapter.ViewHolder>(deckNodeDiffCallback) {
private val layoutInflater = LayoutInflater.from(context)
private var deckTree: DeckNode? = null

/** The non-collapsed subset of the deck tree that matches the current search. */
private var filteredDeckList: List<DeckNode> = ArrayList()
private val zeroCountColor: Int
private val newCountColor: Int
private val learnCountColor: Int
Expand Down Expand Up @@ -103,32 +91,44 @@ class DeckAdapter(
val deckRev: TextView = findViewById(R.id.deckpicker_rev)
}

private val mutex = Mutex()

/**
* Consume a list of [DeckNode]s to render a new deck list.
* @param filter The string to filter the deck by
* Set new data in the adapter. This should be used instead of [submitList] (which is called
* by this method) so there's no need to call [notifyDataSetChanged] on the adapter.
*/
suspend fun buildDeckList(
node: DeckNode,
filter: CharSequence?,
fun submit(
data: List<DeckNode>,
hasSubDecks: Boolean,
currentDeckId: DeckId,
) {
Timber.d("buildDeckList")
// TODO: This is a lazy hack to fix a bug. We hold the lock for far too long
// and do I/O inside it. Better to calculate the new lists outside the lock, then swap
mutex.withLock {
deckTree = node
hasSubdecks = node.children.any { it.children.any() }
currentDeckId = withCol { decks.current().optLong("id") }
// Filtering performs notifyDataSetChanged after the async work is complete
getFilter()?.filter(filter)
// submitList is smart to not trigger a refresh if the new list is the same, but we do need
// an adapter refresh if the other two properties have changed even if the new data is the
// same as they modify some of the adapter's content appearance
val forceRefresh =
areDataSetsEqual(currentList, data) &&
(this.hasSubdecks != hasSubDecks || this.currentDeckId != currentDeckId)
this.hasSubdecks = hasSubDecks
this.currentDeckId = currentDeckId
submitList(data)
if (forceRefresh) notifyDataSetChanged()
}

private fun areDataSetsEqual(
currentSet: List<DeckNode>,
newSet: List<DeckNode>,
): Boolean {
if (currentSet.size != newSet.size) return false
return currentSet.zip(newSet).all { (fst, snd) ->
fst.fullDeckName == snd.fullDeckName
}
}

@CheckResult
fun getNodeByDid(did: DeckId): DeckNode {
val pos = findDeckPosition(did)
return deckList[pos]
/**
* Update the current selected deck so the adapter shows the proper backgrounds.
* Calls [notifyDataSetChanged].
*/
fun updateSelectedDeck(deckId: DeckId) {
this.currentDeckId = deckId
notifyDataSetChanged()
}

override fun onCreateViewHolder(
Expand All @@ -140,8 +140,7 @@ class DeckAdapter(
holder: ViewHolder,
position: Int,
) {
// Update views for this node
val node = filteredDeckList[position]
val node = getItem(position)
// Set the expander icon and padding according to whether or not there are any subdecks
val deckLayout = holder.deckLayout
if (hasSubdecks) {
Expand Down Expand Up @@ -191,8 +190,6 @@ class DeckAdapter(
holder.countsLayout.setOnClickListener { onDeckCountsSelected(node.did) }
}

override fun getItemCount(): Int = filteredDeckList.size

private fun setDeckExpander(
expander: ImageButton,
indent: ImageButton,
Expand All @@ -216,54 +213,6 @@ class DeckAdapter(
indent.minimumWidth = nestedIndent * node.depth
}

/**
* Return the position of the deck in the deck list. If the deck is a child of a collapsed deck
* (i.e., not visible in the deck list), then the position of the parent deck is returned instead.
*
* An invalid deck ID will return position 0.
*/
fun findDeckPosition(did: DeckId): Int {
filteredDeckList.forEachIndexed { index, treeNode ->
if (treeNode.did == did) {
return index
}
}

// If the deck is not in our list, we search again using the immediate parent
// If the deck is not found, return 0
val collapsedDeck = deckTree?.find(did) ?: return 0
val parent = collapsedDeck.parent?.get() ?: return 0
return findDeckPosition(parent.did)
}

private val deckList: List<DeckNode>
get() = filteredDeckList

override fun getFilter(): Filter? = deckTree?.let { DeckFilter(it) }

@VisibleForTesting
inner class DeckFilter(
private val top: DeckNode,
) : Filter() {
override fun performFiltering(constraint: CharSequence?): FilterResults {
val out = top.filterAndFlatten(constraint)
Timber.i("deck filter: %d (%s)", out.size, constraint)
return FilterResults().also {
it.values = out
it.count = out.size
}
}

override fun publishResults(
constraint: CharSequence?,
results: FilterResults,
) {
@Suppress("unchecked_cast")
filteredDeckList = results.values as List<DeckNode>
notifyDataSetChanged()
}
}

companion object {
// Make the selected deck roughly half transparent if there is a background
private const val SELECTED_DECK_ALPHA_AGAINST_BACKGROUND = 0.45
Expand Down Expand Up @@ -301,3 +250,22 @@ class DeckAdapter(
ta.recycle()
}
}

private val deckNodeDiffCallback =
object : DiffUtil.ItemCallback<DeckNode>() {
override fun areItemsTheSame(
oldItem: DeckNode,
newItem: DeckNode,
): Boolean = oldItem.did == newItem.did

override fun areContentsTheSame(
oldItem: DeckNode,
newItem: DeckNode,
): Boolean =
oldItem.did == newItem.did &&
oldItem.filtered == newItem.filtered &&
oldItem.fullDeckName == newItem.fullDeckName &&
oldItem.newCount == newItem.newCount &&
oldItem.lrnCount == newItem.lrnCount &&
oldItem.revCount == newItem.revCount
}
1 change: 1 addition & 0 deletions AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ class DeckPickerTest : RobolectricTest() {
val card = addBasicNote("front", "back").firstCard()
getColUnsafe.sched.buryCards(listOf(card.id))
updateDeckList()
advanceRobolectricLooper()
assertEquals(1, visibleDeckCount)
assertTrue(getColUnsafe.sched.haveBuried(), "Deck should have buried cards")
supportFragmentManager.selectContextMenuOption(DeckPickerContextMenuOption.UNBURY, deckId)
Expand Down

0 comments on commit e1c40cb

Please sign in to comment.