Skip to content

Commit

Permalink
improvement(set-due-date): simplify for FSRS
Browse files Browse the repository at this point in the history
When FSRS is enabled, there is no reason
for due date to be inconsistent with interval

So, hide 'set interval to same value' and use `true`
Property name: `updateIntervalToMatchDueDate`

Confirmed by Jarrett and Expertium paraphrased:

> Expertium: when FSRS is enabled, is there any reason for keeping `ivl`
  not consistent with due date? Because I'm pretty sure there isn't
> Jarrett: Yeah, there isn’t.

https://discord.com/channels/368267295601983490/1282005522513530952/1324758732122624020

The current backend implementation is not ideal:
* `1!` => interval is set to days from today, rather than days from last review
* `1` => interval does not match due date

So I suspect this will be improved upstream in the future

Fixes 17707
  • Loading branch information
david-allison committed Jan 4, 2025
1 parent 1a9b387 commit b6370a9
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 14 deletions.
9 changes: 5 additions & 4 deletions AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,11 @@ open class Reviewer :
}
}

private fun showDueDateDialog() {
val dialog = SetDueDateDialog.newInstance(listOf(currentCardId!!))
showDialogFragment(dialog)
}
private fun showDueDateDialog() =
launchCatchingTask {
val dialog = SetDueDateDialog.newInstance(listOf(currentCardId!!))
showDialogFragment(dialog)
}

private fun showResetCardDialog() {
Timber.i("showResetCardDialog() Reset progress button pressed")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import android.widget.TextView
import androidx.annotation.CheckResult
import androidx.core.content.ContextCompat
import androidx.core.os.bundleOf
import androidx.core.view.isVisible
import androidx.core.widget.doOnTextChanged
import androidx.fragment.app.DialogFragment
import androidx.fragment.app.Fragment
Expand All @@ -48,6 +49,7 @@ import com.ichi2.anki.R
import com.ichi2.anki.launchCatchingTask
import com.ichi2.anki.requireAnkiActivity
import com.ichi2.anki.scheduling.SetDueDateViewModel.Tab
import com.ichi2.anki.servicelayer.getFSRSStatus
import com.ichi2.anki.showThemedToast
import com.ichi2.anki.snackbar.showSnackbar
import com.ichi2.anki.ui.internationalization.toSentenceCase
Expand All @@ -59,6 +61,7 @@ import com.ichi2.utils.create
import com.ichi2.utils.negativeButton
import com.ichi2.utils.neutralButton
import com.ichi2.utils.positiveButton
import com.ichi2.utils.requireBoolean
import com.ichi2.utils.title
import kotlinx.coroutines.launch
import timber.log.Timber
Expand All @@ -84,10 +87,15 @@ class SetDueDateDialog : DialogFragment() {
// used to determine if a rotation has taken place
private var initialRotation: Int = 0

val cardIds: LongArray
get() = requireNotNull(requireArguments().getLongArray(ARG_CARD_IDS)) { ARG_CARD_IDS }

val fsrsEnabled: Boolean
get() = requireArguments().requireBoolean(ARG_FSRS)

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
val cardIds = requireNotNull(requireArguments().getLongArray(ARG_CARD_IDS)) { ARG_CARD_IDS }
viewModel.init(cardIds)
viewModel.init(cardIds, fsrsEnabled)
Timber.d("Set due date dialog: %d card(s)", cardIds.size)
this.initialRotation = getScreenRotation()
}
Expand Down Expand Up @@ -151,9 +159,12 @@ class SetDueDateDialog : DialogFragment() {
)

// setup 'set interval to same value' checkbox
findViewById<MaterialCheckBox>(R.id.change_interval)!!.apply {
isChecked = viewModel.updateIntervalToMatchDueDate
setOnCheckedChangeListener { _, isChecked ->
findViewById<MaterialCheckBox>(R.id.change_interval)!!.also { cb ->
// `.also` is used as .isVisible is an extension, so Kotlin prefers
// incorrectly setting Fragment.isVisible
cb.isVisible = viewModel.canSetUpdateIntervalToMatchDueDate
cb.isChecked = viewModel.updateIntervalToMatchDueDate
cb.setOnCheckedChangeListener { _, isChecked ->
viewModel.updateIntervalToMatchDueDate = isChecked
}
}
Expand Down Expand Up @@ -184,12 +195,17 @@ class SetDueDateDialog : DialogFragment() {

companion object {
const val ARG_CARD_IDS = "ARGS_CARD_IDS"
const val ARG_FSRS = "ARGS_FSRS"
const val MAX_WIDTH_DP = 450

@CheckResult
fun newInstance(cardIds: List<CardId>) =
suspend fun newInstance(cardIds: List<CardId>) =
SetDueDateDialog().apply {
arguments = bundleOf(ARG_CARD_IDS to cardIds.toLongArray())
arguments =
bundleOf(
ARG_CARD_IDS to cardIds.toLongArray(),
ARG_FSRS to (getFSRSStatus() ?: false.also { Timber.w("FSRS Status error") }),
)
Timber.i("Showing 'set due date' dialog for %d cards", cardIds.size)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ class SetDueDateViewModel : ViewModel() {
/** The cards to change the due date of */
lateinit var cardIds: List<CardId>

/** Whether the Free Spaced Repetition Scheduler is enabled */
// initialized in init()
private var fsrsEnabled: Boolean = false
set(value) {
field = value
Timber.d("fsrsEnabled : %b", value)
if (value) {
Timber.d("updateIntervalToMatchDueDate forced to true: FSRS is enabled")
this.updateIntervalToMatchDueDate = true
}
}

/** Whether the user can set [updateIntervalToMatchDueDate] */
val canSetUpdateIntervalToMatchDueDate
// this only makes sense in SM-2, where the due date does not directly impact the next
// interval calculation. In FSRS, the current date is taken into account
// so ivl should match due date for simplicity
get() = !fsrsEnabled

/**
* The number of cards which will be affected
*
Expand Down Expand Up @@ -75,15 +94,30 @@ class SetDueDateViewModel : ViewModel() {
refreshIsValid()
}

/** If `true`, the interval of the card is updated to match the calculated due date */
/**
* If `true`, the interval of the card is updated to match the calculated due date
*
* This is only supported when using SM-2. In FSRS, there's no reason for ivl != due date,
* as the date when a card is seen affects the scheduling.
*
* @throws UnsupportedOperationException if unset when FSRS is enabled
*/
var updateIntervalToMatchDueDate: Boolean = false
get() = if (fsrsEnabled) true else field
set(value) {
Timber.d("updateIntervalToMatchDueDate: %b", value)
if (fsrsEnabled && !value) {
throw UnsupportedOperationException("due date must match interval if using FSRS")
}
field = value
}

fun init(cardIds: LongArray) {
fun init(
cardIds: LongArray,
fsrsEnabled: Boolean,
) {
this.cardIds = cardIds.toList()
this.fsrsEnabled = fsrsEnabled
}

fun setNextDateRangeStart(value: Int?) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import org.junit.runner.RunWith

@Ignore("selectTab(1) does not attach Ids")
@NeedsTest("get the tests working")
@NeedsTest("set interval to same value visibility with FSRS")
@RunWith(AndroidJUnit4::class)
class SetDueDateDialogTest : RobolectricTest() {
@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ import com.ichi2.anki.scheduling.SetDueDateViewModel.Tab
import com.ichi2.libanki.CardId
import com.ichi2.libanki.sched.SetDueDateDays
import com.ichi2.testutils.JvmTest
import com.ichi2.testutils.common.assertThrows
import org.hamcrest.MatcherAssert.assertThat
import org.hamcrest.Matchers.equalTo
import org.junit.Test
import org.junit.runner.RunWith
import kotlin.test.assertFalse
import kotlin.test.assertTrue

@RunWith(AndroidJUnit4::class)
class SetDueDateViewModelTest : JvmTest() {
Expand Down Expand Up @@ -111,12 +114,58 @@ class SetDueDateViewModelTest : JvmTest() {
assertThat(calculateDaysParameter(), eq("0-1!"))
}

@Test
fun `if FSRS is enabled updateIntervalToMatchDueDate may not be set to false`() {
runViewModelTest(fsrsEnabled = true) {
// the value can be set to true
updateIntervalToMatchDueDate = true

assertFalse(
canSetUpdateIntervalToMatchDueDate,
"canSetUpdateIntervalToMatchDueDate when FSRS enabled",
)

val ex =
assertThrows<UnsupportedOperationException>("due date mismatch & FSRS enabled") {
updateIntervalToMatchDueDate = false
}
assertThat(ex.message, equalTo("due date must match interval if using FSRS"))
}

runViewModelTest(fsrsEnabled = false) {
assertTrue(
canSetUpdateIntervalToMatchDueDate,
"canSetUpdateIntervalToMatchDueDate and no FSRS",
)
}
}

@Test
fun `updateIntervalToMatchDueDate default value, given scheduler`() {
runViewModelTest(fsrsEnabled = true) {
assertThat(
"updateIntervalToMatchDueDate default if FSRS",
updateIntervalToMatchDueDate,
equalTo(true),
)
}

runViewModelTest(fsrsEnabled = false) {
assertThat(
"updateIntervalToMatchDueDate default if no FSRS",
updateIntervalToMatchDueDate,
equalTo(false),
)
}
}

private fun runViewModelTest(
cardIds: List<CardId> = listOf(1, 2, 3),
fsrsEnabled: Boolean = false,
testBody: suspend SetDueDateViewModel.() -> Unit,
) = runTest {
val viewModel = SetDueDateViewModel()
viewModel.init(cardIds.toLongArray())
viewModel.init(cardIds.toLongArray(), fsrsEnabled = fsrsEnabled)
testBody(viewModel)
}
}
Expand Down

0 comments on commit b6370a9

Please sign in to comment.