Skip to content

Commit

Permalink
fix some bugs in the new previewer + some refactors (ankidroid#15297)
Browse files Browse the repository at this point in the history 
* refactor: extract NoteEditorDestination in the new previewer
* refactor: move editCardResult handling to the view model
the goal is to ease testing

* refactor: extract WebViewClient creation in PreviewerFragment
onViewCreated was getting to long

* fix: do not open links inside the new previewer
* refactor: inline PreviewerFragment menu items properties
* refactor: add header comments in PreviewerViewModel
debatable, but I think this may help organize stuff

* hide answer if `backside only` is disabled
to match the desktop behavior

* refactor: extract getting card body class
* refactor: make showAnswer() independent of showQuestion()
Without a `bodyclass` argument, _showAnswer() only works if _showQuestion() was called before. Also, in the desktop code, _showQuestion() is always called before _showAnswer().

Given those facts and that I didn't notice that _showAnswer() could work independently

* fix: `_showQuestion() not defined` appearing in logs

currentIndex's collect block was being run before the webview was initialized
* make CoroutineScope.launchCatching use `EmptyCoroutineContext`
so it stays similar to `.launch {}` which uses `EmptyCoroutineContext` as well, which means that tasks are launched in the current thread if a coroutine context isn't provided

* add launchCatchingIO helper
* refactor: rename runCatchingTask
`Task` wasn't being useful in the name

* refactor: move Android dependencies of the new previewer to another file
* rename: backside -> backSide
  • Loading branch information
@BrayanDSO
BrayanDSO authored Feb 7, 2024
1 parent 9e7738f commit 08cd05c
Show file tree
Hide file tree
Showing 5 changed files with 262 additions and 221 deletions.
20 changes: 9 additions & 11 deletions AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import com.ichi2.utils.show
import com.ichi2.utils.title
import kotlinx.coroutines.CancellableContinuation
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
Expand All @@ -56,6 +55,8 @@ import net.ankiweb.rsdroid.exceptions.BackendInterruptedException
import net.ankiweb.rsdroid.exceptions.BackendNetworkException
import net.ankiweb.rsdroid.exceptions.BackendSyncException
import timber.log.Timber
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext
import kotlin.coroutines.resume
import kotlin.coroutines.suspendCoroutine

Expand All @@ -65,11 +66,11 @@ import kotlin.coroutines.suspendCoroutine
* Other errors should ideally be handled in the block.
*/
fun CoroutineScope.launchCatching(
dispatcher: CoroutineDispatcher = Dispatchers.Default,
context: CoroutineContext = EmptyCoroutineContext,
errorMessageHandler: suspend (String) -> Unit,
block: suspend () -> Unit
): Job {
return launch(dispatcher) {
return launch(context) {
try {
block()
} catch (cancellationException: CancellationException) {
Expand All @@ -90,12 +91,9 @@ interface OnErrorListener {
val onError: MutableSharedFlow<String>
}

fun <T> T.launchCatching(
dispatcher: CoroutineDispatcher = Dispatchers.Default,
block: suspend T.() -> Unit
): Job where T : ViewModel, T : OnErrorListener {
fun <T> T.launchCatchingIO(block: suspend T.() -> Unit): Job where T : ViewModel, T : OnErrorListener {
return viewModelScope.launchCatching(
dispatcher,
Dispatchers.IO,
{ onError.emit(it) },
{ block() }
)
Expand All @@ -111,7 +109,7 @@ fun <T> T.launchCatching(
* If not, add a comment explaining why, or refactor to have a method that returns
* a non-null localized message.
*/
suspend fun <T> FragmentActivity.runCatchingTask(
suspend fun <T> FragmentActivity.runCatching(
errorMessage: String? = null,
block: suspend () -> T?
): T? {
Expand Down Expand Up @@ -198,7 +196,7 @@ fun FragmentActivity.launchCatchingTask(
block: suspend CoroutineScope.() -> Unit
): Job {
return lifecycle.coroutineScope.launch {
runCatchingTask(errorMessage) { block() }
runCatching(errorMessage) { block() }
}
}

Expand All @@ -208,7 +206,7 @@ fun Fragment.launchCatchingTask(
block: suspend CoroutineScope.() -> Unit
): Job {
return lifecycle.coroutineScope.launch {
requireActivity().runCatchingTask(errorMessage) { block() }
requireActivity().runCatching(errorMessage) { block() }
}
}

Expand Down
6 changes: 3 additions & 3 deletions AnkiDroid/src/main/java/com/ichi2/anki/pages/CongratsPage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import com.ichi2.anki.R
import com.ichi2.anki.SingleFragmentActivity
import com.ichi2.anki.StudyOptionsActivity
import com.ichi2.anki.dialogs.customstudy.CustomStudyDialog
import com.ichi2.anki.launchCatching
import com.ichi2.anki.launchCatchingIO
import com.ichi2.libanki.DeckId
import com.ichi2.libanki.undoableOp
import kotlinx.coroutines.flow.MutableSharedFlow
Expand Down Expand Up @@ -166,7 +166,7 @@ class CongratsViewModel : ViewModel(), OnErrorListener {
val deckOptionsDestination = MutableSharedFlow<DeckOptionsDestination>()

fun onUnbury() {
launchCatching {
launchCatchingIO {
undoableOp {
sched.unburyDeck(decks.getCurrentId())
}
Expand All @@ -175,7 +175,7 @@ class CongratsViewModel : ViewModel(), OnErrorListener {
}

fun onDeckOptions() {
launchCatching {
launchCatchingIO {
val deckId = withCol { decks.getCurrentId() }
val isFiltered = withCol { decks.isDyn(deckId) }
deckOptionsDestination.emit(DeckOptionsDestination(deckId, isFiltered))
Expand Down
109 changes: 54 additions & 55 deletions AnkiDroid/src/main/java/com/ichi2/anki/previewer/PreviewerFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,36 +39,23 @@ import com.google.android.material.button.MaterialButton
import com.google.android.material.slider.Slider
import com.google.android.material.textview.MaterialTextView
import com.ichi2.anki.Flag
import com.ichi2.anki.NoteEditor
import com.ichi2.anki.R
import com.ichi2.anki.SingleFragmentActivity
import com.ichi2.anki.browser.PreviewerIdsFile
import com.ichi2.anki.getViewerAssetLoader
import com.ichi2.anki.pages.AnkiServer.Companion.LOCALHOST
import com.ichi2.anki.previewer.PreviewerViewModel.Companion.stdHtml
import com.ichi2.annotations.NeedsTest
import com.ichi2.compat.CompatHelper.Companion.getSerializableCompat
import com.ichi2.themes.Themes
import kotlinx.coroutines.flow.collectLatest
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import timber.log.Timber

class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickListener {
private lateinit var viewModel: PreviewerViewModel

private val menu: Menu
get() = requireView().findViewById<Toolbar>(R.id.toolbar).menu

private val backsideOnlyOption: MenuItem
get() = menu.findItem(R.id.action_back_side_only)

private val markOption: MenuItem
get() = menu.findItem(R.id.action_mark)

private val flagOption: MenuItem
get() = menu.findItem(R.id.action_flag)

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
val previewerIdsFile = requireNotNull(requireArguments().getSerializableCompat(IDS_FILE_EXTRA)) {
"$IDS_FILE_EXTRA is required"
Expand All @@ -80,23 +67,10 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
PreviewerViewModel.factory(previewerIdsFile, currentIndex)
)[PreviewerViewModel::class.java]

val assetLoader = requireContext().getViewerAssetLoader(LOCALHOST)
val webView = view.findViewById<WebView>(R.id.webview)
CookieManager.getInstance().setAcceptThirdPartyCookies(webView, true)
with(webView) {
webViewClient = object : WebViewClient() {
override fun shouldInterceptRequest(
view: WebView?,
request: WebResourceRequest
): WebResourceResponse? {
return assetLoader.shouldInterceptRequest(request.url)
}

override fun onPageFinished(view: WebView?, url: String?) {
super.onPageFinished(view, url)
viewModel.loadCurrentCard()
}
}
webViewClient = onCreateWebViewClient()
scrollBarStyle = View.SCROLLBARS_OUTSIDE_OVERLAY
with(settings) {
javaScriptEnabled = true
Expand Down Expand Up @@ -135,13 +109,6 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
webView.evaluateJavascript(eval, null)
}
.launchIn(lifecycleScope)
lifecycleScope.launch {
viewModel.backsideOnly
.flowWithLifecycle(lifecycle)
.collectLatest { isBacksideOnly ->
setBacksideOnlyButtonIcon(isBacksideOnly)
}
}

val cardsCount = viewModel.cardsCount()
lifecycleScope.launch {
Expand All @@ -154,24 +121,38 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
getString(R.string.preview_progress_bar_text, displayIndex, cardsCount)
}
}
/* ************************************* Menu items ************************************* */
val menu = view.findViewById<Toolbar>(R.id.toolbar).menu

lifecycleScope.launch {
viewModel.backSideOnly
.flowWithLifecycle(lifecycle)
.collectLatest { isBackSideOnly ->
setBackSideOnlyButtonIcon(menu, isBackSideOnly)
}
}

lifecycleScope.launch {
viewModel.isMarked
.flowWithLifecycle(lifecycle)
.collectLatest { isMarked ->
if (isMarked) {
markOption.setIcon(R.drawable.ic_star)
markOption.setTitle(R.string.menu_unmark_note)
} else {
markOption.setIcon(R.drawable.ic_star_border_white)
markOption.setTitle(R.string.menu_mark_note)
with(menu.findItem(R.id.action_mark)) {
if (isMarked) {
setIcon(R.drawable.ic_star)
setTitle(R.string.menu_unmark_note)
} else {
setIcon(R.drawable.ic_star_border_white)
setTitle(R.string.menu_mark_note)
}
}
}
}

lifecycleScope.launch {
viewModel.flagCode
.flowWithLifecycle(lifecycle)
.collectLatest { flagCode ->
flagOption.setIcon(Flag.fromCode(flagCode).drawableRes)
menu.findItem(R.id.action_flag).setIcon(Flag.fromCode(flagCode).drawableRes)
}
}

Expand Down Expand Up @@ -222,11 +203,36 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
super.onViewCreated(view, savedInstanceState)
}

private fun onCreateWebViewClient(): WebViewClient {
val assetLoader = requireContext().getViewerAssetLoader(LOCALHOST)
return object : WebViewClient() {
override fun shouldInterceptRequest(
view: WebView?,
request: WebResourceRequest
): WebResourceResponse? {
return assetLoader.shouldInterceptRequest(request.url)
}

override fun onPageFinished(view: WebView?, url: String?) {
viewModel.onPageFinished()
}

override fun shouldOverrideUrlLoading(view: WebView, request: WebResourceRequest): Boolean {
try {
startActivity(Intent(Intent.ACTION_VIEW, request.url))
} catch (_: Exception) {
Timber.w("Could not open url")
}
return true
}
}
}

override fun onMenuItemClick(item: MenuItem): Boolean {
when (item.itemId) {
R.id.action_edit -> editCard()
R.id.action_mark -> viewModel.toggleMark()
R.id.action_back_side_only -> viewModel.toggleBacksideOnly()
R.id.action_back_side_only -> viewModel.toggleBackSideOnly()
R.id.action_flag_zero -> viewModel.setFlag(Flag.NONE)
R.id.action_flag_one -> viewModel.setFlag(Flag.RED)
R.id.action_flag_two -> viewModel.setFlag(Flag.ORANGE)
Expand All @@ -239,9 +245,9 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
return true
}

private fun setBacksideOnlyButtonIcon(isBacksideOnly: Boolean) {
backsideOnlyOption.apply {
if (isBacksideOnly) {
private fun setBackSideOnlyButtonIcon(menu: Menu, isBackSideOnly: Boolean) {
menu.findItem(R.id.action_back_side_only).apply {
if (isBackSideOnly) {
setIcon(R.drawable.ic_card_answer)
setTitle(R.string.card_side_answer)
} else {
Expand All @@ -252,18 +258,11 @@ class PreviewerFragment : Fragment(R.layout.previewer), Toolbar.OnMenuItemClickL
}

private val editCardLauncher = registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result ->
if (result.data?.getBooleanExtra(NoteEditor.RELOAD_REQUIRED_EXTRA_KEY, false) == true ||
result.data?.getBooleanExtra(NoteEditor.NOTE_CHANGED_EXTRA_KEY, false) == true
) {
viewModel.loadCurrentCard(reload = true)
}
viewModel.handleEditCardResult(result)
}

private fun editCard() {
val intent = Intent(requireContext(), NoteEditor::class.java).apply {
putExtra(NoteEditor.EXTRA_CALLER, NoteEditor.CALLER_PREVIEWER_EDIT)
putExtra(NoteEditor.EXTRA_EDIT_FROM_CARD_ID, viewModel.cardId())
}
val intent = viewModel.getNoteEditorDestination().toIntent(requireContext())
editCardLauncher.launch(intent)
}

Expand Down
Loading

0 comments on commit 08cd05c

Please sign in to comment.