mirror of
https://github.com/ankidroid/Anki-Android.git
synced 2024-09-20 12:02:16 +02:00
fix: automated sync leads to "collection not open"
Lots of threading bugs here, this is sloppy code and needs to be cleaned up ASAP * Add a lock when closing the collection * Use 'safe' collection methods inside DeckAdapter to respect the lock * Add a mutex to stop 'duplicated' decks appearing Fixes 13429
This commit is contained in:
parent
f6b1382dea
commit
70151c563e
@ -261,11 +261,11 @@ open class DeckPicker :
|
||||
// LISTENERS
|
||||
// ----------------------------------------------------------------------------
|
||||
private val mDeckExpanderClickListener = View.OnClickListener { view: View ->
|
||||
toggleDeckExpand(view.tag as Long)
|
||||
launchCatchingTask { toggleDeckExpand(view.tag as Long) }
|
||||
}
|
||||
private val mDeckClickListener = View.OnClickListener { v: View -> onDeckClick(v, DeckSelectionType.DEFAULT) }
|
||||
private val mCountsClickListener = View.OnClickListener { v: View -> onDeckClick(v, DeckSelectionType.SHOW_STUDY_OPTIONS) }
|
||||
private fun onDeckClick(v: View, selectionType: DeckSelectionType) {
|
||||
private val mDeckClickListener = View.OnClickListener { v: View -> launchCatchingTask { onDeckClick(v, DeckSelectionType.DEFAULT) } }
|
||||
private val mCountsClickListener = View.OnClickListener { v: View -> launchCatchingTask { onDeckClick(v, DeckSelectionType.SHOW_STUDY_OPTIONS) } }
|
||||
private suspend fun onDeckClick(v: View, selectionType: DeckSelectionType) {
|
||||
val deckId = v.tag as Long
|
||||
Timber.i("DeckPicker:: Selected deck with id %d", deckId)
|
||||
var collectionIsOpen = false
|
||||
@ -1081,7 +1081,7 @@ open class DeckPicker :
|
||||
}
|
||||
KeyEvent.KEYCODE_SLASH, KeyEvent.KEYCODE_S -> {
|
||||
Timber.i("Study from keypress")
|
||||
handleDeckSelection(col.decks.selected(), DeckSelectionType.SKIP_STUDY_OPTIONS)
|
||||
launchCatchingTask { handleDeckSelection(col.decks.selected(), DeckSelectionType.SKIP_STUDY_OPTIONS) }
|
||||
}
|
||||
else -> {}
|
||||
}
|
||||
@ -1139,7 +1139,7 @@ open class DeckPicker :
|
||||
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
|
||||
@RustCleanup("make mDueTree a concrete DeckDueTreeNode")
|
||||
@Suppress("UNCHECKED_CAST")
|
||||
fun toggleDeckExpand(did: DeckId) {
|
||||
suspend fun toggleDeckExpand(did: DeckId) {
|
||||
if (!col.decks.children(did).isEmpty()) {
|
||||
// update DB
|
||||
col.decks.collapse(did)
|
||||
@ -1725,7 +1725,7 @@ open class DeckPicker :
|
||||
}
|
||||
}
|
||||
|
||||
private fun handleDeckSelection(did: DeckId, selectionType: DeckSelectionType) {
|
||||
private suspend fun handleDeckSelection(did: DeckId, selectionType: DeckSelectionType) {
|
||||
// Clear the undo history when selecting a new deck
|
||||
if (col.decks.selected() != did) {
|
||||
col.clearUndo()
|
||||
@ -1820,7 +1820,7 @@ open class DeckPicker :
|
||||
*
|
||||
* @param did The deck ID of the deck to select.
|
||||
*/
|
||||
private fun scrollDecklistToDeck(did: DeckId) {
|
||||
private suspend fun scrollDecklistToDeck(did: DeckId) {
|
||||
val position = mDeckListAdapter.findDeckPosition(did)
|
||||
mRecyclerViewLayoutManager.scrollToPositionWithOffset(position, recyclerView.height / 2)
|
||||
}
|
||||
@ -1883,7 +1883,7 @@ open class DeckPicker :
|
||||
}
|
||||
@Suppress("UNCHECKED_CAST")
|
||||
dueTree = result as List<TreeNode<AbstractDeckTreeNode>>?
|
||||
renderPage(collectionIsEmpty)
|
||||
launchCatchingTask { renderPage(collectionIsEmpty) }
|
||||
// Update the mini statistics bar as well
|
||||
launchCatchingTask {
|
||||
val reviewSummaryStatsSting = AnkiStatsTaskHandler.getReviewSummaryStatisticsString(this@DeckPicker)
|
||||
@ -1896,7 +1896,7 @@ open class DeckPicker :
|
||||
Timber.d("Startup - Deck List UI Completed")
|
||||
}
|
||||
|
||||
private fun renderPage(collectionIsEmpty: Boolean) {
|
||||
private suspend fun renderPage(collectionIsEmpty: Boolean) {
|
||||
if (dueTree == null) {
|
||||
// mDueTree may be set back to null when the activity restart.
|
||||
// We may need to recompute it.
|
||||
@ -1945,32 +1945,32 @@ open class DeckPicker :
|
||||
// We're done here
|
||||
return
|
||||
}
|
||||
mDeckListAdapter.buildDeckList(dueTree!!, col, currentFilter)
|
||||
mDeckListAdapter.buildDeckList(dueTree!!, currentFilter)
|
||||
|
||||
// Set the "x due in y minutes" subtitle
|
||||
try {
|
||||
val eta = mDeckListAdapter.eta
|
||||
val eta = mDeckListAdapter.eta()
|
||||
val due = mDeckListAdapter.due
|
||||
val res = resources
|
||||
if (col.cardCount() != -1) {
|
||||
val time: String = if (eta != -1 && eta != null) {
|
||||
Utils.timeQuantityTopDeckPicker(this, (eta * 60).toLong())
|
||||
|
||||
val time: String = if (eta != -1 && eta != null) {
|
||||
Utils.timeQuantityTopDeckPicker(this, (eta * 60).toLong())
|
||||
} else {
|
||||
"-"
|
||||
}
|
||||
if (due != null && supportActionBar != null) {
|
||||
val cardCount = withCol { cardCount() }
|
||||
val subTitle: String = if (due == 0) {
|
||||
res.getQuantityString(R.plurals.deckpicker_title_zero_due, cardCount, cardCount)
|
||||
} else {
|
||||
"-"
|
||||
}
|
||||
if (due != null && supportActionBar != null) {
|
||||
val subTitle: String = if (due == 0) {
|
||||
res.getQuantityString(R.plurals.deckpicker_title_zero_due, col.cardCount(), col.cardCount())
|
||||
} else {
|
||||
res.getQuantityString(R.plurals.deckpicker_title, due, due, time)
|
||||
}
|
||||
supportActionBar!!.subtitle = subTitle
|
||||
res.getQuantityString(R.plurals.deckpicker_title, due, due, time)
|
||||
}
|
||||
supportActionBar!!.subtitle = subTitle
|
||||
}
|
||||
} catch (e: RuntimeException) {
|
||||
Timber.e(e, "RuntimeException setting time remaining")
|
||||
}
|
||||
val current = col.decks.current().optLong("id")
|
||||
val current = withCol { decks.current().optLong("id") }
|
||||
if (mFocusedDeck != current) {
|
||||
scrollDecklistToDeck(current)
|
||||
mFocusedDeck = current
|
||||
|
@ -26,21 +26,25 @@ import android.widget.*
|
||||
import androidx.annotation.VisibleForTesting
|
||||
import androidx.core.content.ContextCompat
|
||||
import androidx.recyclerview.widget.RecyclerView
|
||||
import com.ichi2.anki.CollectionManager.withCol
|
||||
import com.ichi2.anki.R
|
||||
import com.ichi2.anki.servicelayer.DeckService.defaultDeckHasCards
|
||||
import com.ichi2.libanki.Collection
|
||||
import com.ichi2.libanki.DeckId
|
||||
import com.ichi2.libanki.sched.AbstractDeckTreeNode
|
||||
import com.ichi2.libanki.sched.Counts
|
||||
import com.ichi2.libanki.sched.TreeNode
|
||||
import com.ichi2.utils.KotlinCleanup
|
||||
import com.ichi2.utils.TypedFilter
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.sync.Mutex
|
||||
import kotlinx.coroutines.sync.withLock
|
||||
import net.ankiweb.rsdroid.BackendFactory
|
||||
import net.ankiweb.rsdroid.RustCleanup
|
||||
import timber.log.Timber
|
||||
import java.util.*
|
||||
|
||||
@KotlinCleanup("lots to do")
|
||||
@RustCleanup("synchronous col access")
|
||||
@RustCleanup("Lots of bad code: should not be using suspend functions inside an adapter")
|
||||
class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context) : RecyclerView.Adapter<DeckAdapter.ViewHolder>(), Filterable {
|
||||
private val mDeckList: MutableList<TreeNode<AbstractDeckTreeNode>>
|
||||
|
||||
@ -62,7 +66,6 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
private var mDeckExpanderClickListener: View.OnClickListener? = null
|
||||
private var mDeckLongClickListener: OnLongClickListener? = null
|
||||
private var mCountsClickListener: View.OnClickListener? = null
|
||||
private lateinit var mCol: Collection
|
||||
|
||||
// Totals accumulated as each deck is processed
|
||||
private var mNew = 0
|
||||
@ -120,26 +123,32 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
mPartiallyTransparentForBackground = isTransparent
|
||||
}
|
||||
|
||||
private val mutex = Mutex()
|
||||
|
||||
/**
|
||||
* Consume a list of [AbstractDeckTreeNode]s to render a new deck list.
|
||||
* @param filter The string to filter the deck by
|
||||
*/
|
||||
fun buildDeckList(nodes: List<TreeNode<AbstractDeckTreeNode>>, col: Collection, filter: CharSequence?) {
|
||||
mCol = col
|
||||
mDeckList.clear()
|
||||
mCurrentDeckList.clear()
|
||||
mRev = 0
|
||||
mLrn = mRev
|
||||
mNew = mLrn
|
||||
mNumbersComputed = true
|
||||
mHasSubdecks = false
|
||||
currentDeckId = mCol.decks.current().optLong("id")
|
||||
processNodes(nodes)
|
||||
// Filtering performs notifyDataSetChanged after the async work is complete
|
||||
getFilter().filter(filter)
|
||||
suspend fun buildDeckList(nodes: List<TreeNode<AbstractDeckTreeNode>>, filter: CharSequence?) {
|
||||
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 {
|
||||
mDeckList.clear()
|
||||
mCurrentDeckList.clear()
|
||||
mRev = 0
|
||||
mLrn = mRev
|
||||
mNew = mLrn
|
||||
mNumbersComputed = true
|
||||
mHasSubdecks = false
|
||||
currentDeckId = withCol { decks.current().optLong("id") }
|
||||
processNodes(nodes)
|
||||
// Filtering performs notifyDataSetChanged after the async work is complete
|
||||
getFilter().filter(filter)
|
||||
}
|
||||
}
|
||||
|
||||
fun getNodeByDid(did: DeckId): TreeNode<AbstractDeckTreeNode> {
|
||||
suspend fun getNodeByDid(did: DeckId): TreeNode<AbstractDeckTreeNode> {
|
||||
val pos = findDeckPosition(did)
|
||||
return deckList[pos]
|
||||
}
|
||||
@ -161,7 +170,7 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
deckLayout.setPadding(smallPadding, 0, rightPadding, 0)
|
||||
holder.deckExpander.visibility = View.VISIBLE
|
||||
// Create the correct expander for this deck
|
||||
setDeckExpander(holder.deckExpander, holder.indentView, treeNode)
|
||||
runBlocking { setDeckExpander(holder.deckExpander, holder.indentView, treeNode) }
|
||||
} else {
|
||||
holder.deckExpander.visibility = View.GONE
|
||||
val normalPadding = deckLayout.resources.getDimension(R.dimen.deck_picker_left_padding).toInt()
|
||||
@ -193,7 +202,7 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
val filtered = if (!BackendFactory.defaultLegacySchema) {
|
||||
node.filtered
|
||||
} else {
|
||||
mCol.decks.isDyn(node.did)
|
||||
runBlocking { withCol { decks.isDyn(node.did) } }
|
||||
}
|
||||
if (filtered) {
|
||||
holder.deckName.setTextColor(mDeckNameDynColor)
|
||||
@ -235,10 +244,11 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
return mCurrentDeckList.size
|
||||
}
|
||||
|
||||
private fun setDeckExpander(expander: ImageButton, indent: ImageButton, node: TreeNode<AbstractDeckTreeNode>) {
|
||||
@RustCleanup("non suspend")
|
||||
private suspend fun setDeckExpander(expander: ImageButton, indent: ImageButton, node: TreeNode<AbstractDeckTreeNode>) {
|
||||
val nodeValue = node.value
|
||||
val collapsed = if (BackendFactory.defaultLegacySchema) {
|
||||
mCol.decks.get(nodeValue.did).optBoolean("collapsed", false)
|
||||
withCol { decks.get(nodeValue.did).optBoolean("collapsed", false) }
|
||||
} else {
|
||||
node.value.collapsed
|
||||
}
|
||||
@ -261,19 +271,20 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
indent.minimumWidth = width
|
||||
}
|
||||
|
||||
private fun processNodes(nodes: List<TreeNode<AbstractDeckTreeNode>>) {
|
||||
private suspend fun processNodes(nodes: List<TreeNode<AbstractDeckTreeNode>>) {
|
||||
for (node in nodes) {
|
||||
var shouldRecurse = true
|
||||
if (BackendFactory.defaultLegacySchema) {
|
||||
// If the default deck is empty, hide it by not adding it to the deck list.
|
||||
// We don't hide it if it's the only deck or if it has sub-decks.
|
||||
if (node.value.did == 1L && nodes.size > 1 && !node.hasChildren()) {
|
||||
if (!defaultDeckHasCards(mCol)) {
|
||||
if (withCol { !defaultDeckHasCards(col) }) {
|
||||
continue
|
||||
}
|
||||
}
|
||||
// If any of this node's parents are collapsed, don't add it to the deck list
|
||||
for (parent in mCol.decks.parents(node.value.did)) {
|
||||
val parents = withCol { decks.parents(node.value.did) }
|
||||
for (parent in parents) {
|
||||
mHasSubdecks = true // If a deck has a parent it means it's a subdeck so set a flag
|
||||
if (parent.optBoolean("collapsed")) {
|
||||
return
|
||||
@ -311,14 +322,15 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
*
|
||||
* An invalid deck ID will return position 0.
|
||||
*/
|
||||
fun findDeckPosition(did: DeckId): Int {
|
||||
@RustCleanup("optimize")
|
||||
suspend fun findDeckPosition(did: DeckId): Int {
|
||||
for (i in mCurrentDeckList.indices) {
|
||||
if (mCurrentDeckList[i].value.did == did) {
|
||||
return i
|
||||
}
|
||||
}
|
||||
// If the deck is not in our list, we search again using the immediate parent
|
||||
val parents = mCol.decks.parents(did)
|
||||
val parents = withCol { decks.parents(did) }
|
||||
return if (parents.isEmpty()) {
|
||||
0
|
||||
} else {
|
||||
@ -326,12 +338,12 @@ class DeckAdapter(private val layoutInflater: LayoutInflater, context: Context)
|
||||
}
|
||||
}
|
||||
|
||||
val eta: Int?
|
||||
get() = if (mNumbersComputed) {
|
||||
mCol.sched.eta(Counts(mNew, mLrn, mRev))
|
||||
} else {
|
||||
null
|
||||
}
|
||||
suspend fun eta(): Int? = if (mNumbersComputed) {
|
||||
withCol { sched.eta(Counts(mNew, mLrn, mRev)) }
|
||||
} else {
|
||||
null
|
||||
}
|
||||
|
||||
val due: Int?
|
||||
get() = if (mNumbersComputed) {
|
||||
mNew + mLrn + mRev
|
||||
|
@ -24,6 +24,7 @@ import android.os.PowerManager
|
||||
import android.os.PowerManager.WakeLock
|
||||
import com.ichi2.anki.AnkiDroidApp
|
||||
import com.ichi2.anki.CollectionHelper
|
||||
import com.ichi2.anki.CollectionManager
|
||||
import com.ichi2.anki.CrashReportService
|
||||
import com.ichi2.anki.R
|
||||
import com.ichi2.anki.exception.MediaSyncException
|
||||
@ -455,7 +456,7 @@ class Connection : BaseAsyncTask<Connection.Payload, Any, Connection.Payload>()
|
||||
} finally {
|
||||
Timber.i("Sync Finished - Closing Collection")
|
||||
// don't bump mod time unless we explicitly save
|
||||
col?.close(false)
|
||||
CollectionManager.closeCollectionBlocking(false)
|
||||
CollectionHelper.instance.unlockCollection()
|
||||
}
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user