Fixed bug which was causing AnkiDroid API to not list media on cards. #17859
Conversation
|
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
| currentCard: Card, | ||
| includeRemote: Boolean = false, | ||
| ): List<String> { | ||
| val l: MutableList<String> = ArrayList() |
There was a problem hiding this comment.
This doesn't look like upstream code:
There was a problem hiding this comment.
@david-allison I have reimplemented the old code for the fileinStr from here:
This logic was able to extract the image file names but it could not extract the AV tags. My guess is that this is because the
Card.renderOutput returns AVTags and it is in the form SoundOrVideoTag(filename). To address this, I added some logic in the function to extract the AVTags as well, and it is now working as expected! Could you please review the changes and let me know if you think this implementation is correct? Any feedback or suggestions for improvement would be greatly appreciated.
There was a problem hiding this comment.
We should follow upstream as close as possible in libanki, unless there's a strong reason not to
There was a problem hiding this comment.
ok I will try again with the upstream code you provided
There was a problem hiding this comment.
Hi @david-allison,
I have attempted to implement the upstream code and although the logic successfully includes image files in the returned media array, the audio files are missing. From my observation, the logic I initially added for extracting audio files ensures they are included in the media array.
Here's the code I used:
fun filesInStr(
currentCard: Card,
includeRemote: Boolean = false,
): List<String> {
val l: MutableList<String> = ArrayList()
val model = currentCard.noteType(col)
val renderOutput = currentCard.renderOutput(col)
val string = renderOutput.questionText + renderOutput.answerText
var strings: MutableList<String?> = ArrayList()
if (model!!.isCloze && string.contains("{{c")) {
// Expand clozes if necessary
strings = expandClozes(string)
} else {
strings.add(string)
}
for (s in strings) {
var s = s
// Handle LaTeX
@KotlinCleanup("change to .map { }")
val svg = model.optBoolean("latexsvg", false)
s = LaTeX.mungeQA(s!!, col, svg)
// Extract filenames from the strings using regex patterns
var m: Matcher
for (p in REGEXPS) {
val fnameIdx =
when (p) {
fSoundRegexps -> 2
fImgAudioRegExpU -> 2
else -> 3
}
m = p.matcher(s)
while (m.find()) {
val fname = m.group(fnameIdx)!!
val isLocal = !fRemotePattern.matcher(fname.lowercase(Locale.getDefault())).find()
if (isLocal || includeRemote) {
l.add(fname)
}
}
}
}
return l
}Could you please advise on how you would like to proceed? Should I enhance the upstream logic to account for audio files, or is there another direction you'd suggest?
There was a problem hiding this comment.
That doesn't look like the upstream code, which will cause problems.
From a brief glance, the upstream code looks like it handles sound files:
- Implement the upstream code, as-is. This should get a subset of sounds or images
- Write a couple of ignored & failing tests for the functionality which it doesn't cover.
- We can discuss the exact issues with the failing tests, and see if it's an issue upstream, or an issue with our conversion
There was a problem hiding this comment.
@david-allison I have implemented the upstream python code that you provided:
fun filesInStr(
currentCard: Card,
includeRemote: Boolean = false,
): List<String> {
val files = mutableListOf<String>()
var string = currentCard.renderOutput(col).questionText + currentCard.renderOutput(col).answerText
string = LaTeX.mungeQA(string,col,true)
for (reg in REGEXPS){
val matcher = reg.matcher(string)
while (matcher.find()) {
val fname = matcher.group("fname")?:continue
val isLocal = !Pattern.compile("(https?|ftp)://", Pattern.CASE_INSENSITIVE)
.matcher(fname).find()
if (isLocal || includeRemote) {
files.add(fname)
}
}
}
return files
}The media_files array is now returning an empty result. This might be due to the current LaTeX tags not aligning with those expected by mungeQA(). Kindly let me know how you’d like to proceed.
There was a problem hiding this comment.
Hey @david-allison, just checking in—any updates on my last comment? Thanks!
There was a problem hiding this comment.
Could you push the code, or provide a patch from Copy patch to clipboard so it can be discussed?
Please follow up in 24 hours if it's not looked at. Still far too busy, but my lack of communication wasn't intentional
lukstbit
added
the
Needs Author Reply
| @@ -1344,6 +1345,57 @@ class ContentProviderTest : InstrumentedTest() { | |||
| } ?: fail("query returned null") | |||
| } | |||
|
|
|||
| @Test | |||
| fun testMediaFilesAddedCorrectlyInReviewInfo() { | |||
There was a problem hiding this comment.
Had time to refactor this a little.
Double tap shift to apply patches from the clipboard
Subject: [PATCH]
---
Index: AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt
--- a/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt (revision 47b50bb72275d02c5f3459f6c6c9907f6d4242f0)
+++ b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt (date 1738013022332)
@@ -21,6 +21,7 @@
import android.content.ContentResolver
import android.content.ContentUris
import android.content.ContentValues
+import android.database.Cursor
import android.database.CursorWindow
import android.net.Uri
import anki.notetypes.StockNotetype
@@ -53,10 +54,12 @@
import kotlinx.serialization.json.Json
import net.ankiweb.rsdroid.exceptions.BackendNotFoundException
import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.allOf
import org.hamcrest.Matchers.containsString
import org.hamcrest.Matchers.equalTo
import org.hamcrest.Matchers.greaterThan
import org.hamcrest.Matchers.greaterThanOrEqualTo
+import org.hamcrest.Matchers.hasItem
import org.json.JSONObject
import org.junit.After
import org.junit.Assert.assertEquals
@@ -69,6 +72,7 @@
import org.junit.Test
import timber.log.Timber
import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
import kotlin.test.junit.JUnitAsserter.assertNotNull
/**
@@ -1347,17 +1351,27 @@
@Test
fun testMediaFilesAddedCorrectlyInReviewInfo() {
- val frontContent = """<img src="img.jpg"> [sound:test.mp3]"""
val imageFileName = "img.jpg"
val audioFileName = "test.mp3"
- val note = addNoteUsingBasicModel("Hello $frontContent", "backContent")
- val ord = 0
- val card = note.cards(col)[ord]
+ addNoteUsingBasicModel("""Hello <img src="$imageFileName"> [sound:$audioFileName]""")
+ .firstCard(col).update {
+ queue = QueueType.New
+ due = col.sched.today
+ }
- card.queue = QueueType.New
- card.due = col.sched.today
- col.updateCard(card)
+ queryReviewInfo { cursor ->
+ val media = cursor
+ .getString(cursor.getColumnIndex(FlashCardsContract.ReviewInfo.MEDIA_FILES))
+ .let { Json.decodeFromString<List<String>>(it) }
+ assertThat("media files returned", media, allOf(
+ hasItem(imageFileName),
+ hasItem(audioFileName),
+ ))
+ }
+ }
+
+ private fun queryReviewInfo(block: (Cursor) -> Unit) {
contentResolver
.query(
FlashCardsContract.ReviewInfo.CONTENT_URI,
@@ -1365,34 +1379,9 @@
null,
null,
null,
- )?. let { cursor ->
- if (!cursor.moveToFirst()) {
- fail("failed")
- }
-
- assertNotNull("Cursor should not be null", cursor)
-
- val mediaFilesArray = cursor.getString(cursor.getColumnIndex(FlashCardsContract.ReviewInfo.MEDIA_FILES))
-
- var imageFilePresent = false
- var audioFilePresent = false
- var allMediaFilesPresent = false
-
- val media: List<String> = Json.decodeFromString(mediaFilesArray)
-
- for (ele in media) {
- if (ele == imageFileName) {
- imageFilePresent = true
- }
- if (ele == audioFileName) {
- audioFilePresent = true
- }
- }
- if (imageFilePresent and audioFilePresent) {
- allMediaFilesPresent = true
- }
-
- assertTrue("All media files should be present in the media_files array", allMediaFilesPresent)
+ )?.use { cursor ->
+ assertTrue("has rows") { cursor.moveToFirst() }
+ block(cursor)
}
}
Index: AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt
--- a/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt (revision 47b50bb72275d02c5f3459f6c6c9907f6d4242f0)
+++ b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt (date 1738013022339)
@@ -149,6 +149,14 @@
}
}
+
+ @DuplicatedCode("This should be refactored into a shared library later")
+ fun Card.update(block: Card.() -> Unit): Card {
+ block(this)
+ col.updateCard(this)
+ return this
+ }
+
@DuplicatedCode("This is copied from RobolectricTest. This will be refactored into a shared library later")
protected fun Card.moveToReviewQueue() {
this.queue = QueueType.RevThere was a problem hiding this comment.
Hey @david-allison,
I hope you're doing well! I just wanted to check in again on this PR—it’s been two weeks since my last update, and I haven’t received a reply yet. I wanted to make sure I haven’t missed anything or if there’s anything I should adjust to move this forward.
Please let me know if there are any changes you'd like me to make or if there’s anything else I can do. Thanks for your time, and I really appreciate your feedback!
There was a problem hiding this comment.
Hi, I haven't seen any movement on this comment in a while. It still feels to be pending
Purpose / Description
Fixed the bug which was causing AnkiDroid API to not list media on cards. Added a unit test to confirm that media files are listed correctly in the ReviewInfo.
Fixes
Approach
Reimplemented the
filesinStrfunction which was causing the bug.How Has This Been Tested?
A unit test
testMediaFilesAddedCorrectlyInReviewInfo()has been added in theContentProviderTest.ktto confirm that theMEDIA_FILESattribute ofReviewInfogets populated correctly.Checklist
Please, go through these checks before submitting the PR.