Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

[BUG] AnkiDroid API no longer lists media on cards

Open piotrbrzezina opened this issue 1 year ago • 15 comments

Hi, I'm trying to write an app that will launch upon unlocking the phone, and its only task will be to present one card, then close after receiving a response. Unfortunately, I'm struggling with one problem, namely how to display images and play audio files because I don't have access to them due to the new Android permissions. I'm wondering if there's a way around this?

I was thinking about whether it would be possible to add another method to the AnkiDroid API that, return an image/audio binary data if it had access to such a file. Do you think this is doable in Android?

piotrbrzezina avatar Sep 13 '24 05:09 piotrbrzezina

Hello! 👋 Thanks for logging this issue. Please remember we are all volunteers here, so some patience may be required before we can get to the issue. Also remember that the fastest way to get resolution on an issue is to propose a change directly, https://github.com/ankidroid/Anki-Android/wiki/Contributing

welcome[bot] avatar Sep 13 '24 05:09 welcome[bot]

Hey there - that seems feasible. We already have a ContentResolver - it allows you to fetch a card IIRC, it could also allow you to fetch an array (or whatever ContentResolver calls it - IIRC it is kind of a database-ish design so maybe a "result set" or whatever) of all media related to that card

I imagine you'd want file name, media object size then the data stream or similar

Anything that did something like that seems reasonable to me

mikehardy avatar Sep 13 '24 18:09 mikehardy

Thanks @mikehardy for the reply, but unfortunately, I'm not sure if I understood it correctly (I'm not an Android developer, I've only learned a bit about it). Are you saying that I can already retrieve image data using the AnkiDroid API, or did you mean that a new method needs to be added to the API, which would return file data for a specific card?

piotrbrzezina avatar Sep 14 '24 04:09 piotrbrzezina

@piotrbrzezina I should have looked more closely before, and I'm sorry I was ambiguous.

It appears that media files are on the ReviewInfo object, which sounds like it might be just what you need: https://github.com/ankidroid/Anki-Android/blob/83ed107806b2d9259bdb9a08414aad672a567e08/api/src/main/java/com/ichi2/anki/FlashCardsContract.kt#L688

mikehardy avatar Sep 15 '24 21:09 mikehardy

Thanks for the information. I was trying to do what you proposed, but in my case, it is not working. In that JSON, I only get an empty array '[]'. Could you check if I'm doing it correctly? This is the code I used to get data from the JSON array.

          val deckSelector = "limit=?,deckID=?"
          val deckArguments = arrayOfNulls<String>(2)
          deckArguments[0] = "" + 1
          deckArguments[1] = "" + deckId
          
          val card = HashMap<String, String>()
          val cardCursor = context.contentResolver.query(
                  FlashCardsContract.ReviewInfo.CONTENT_URI,
                  null,
                  deckSelector,
                  deckArguments,
                  null
          )
          
          if (cardCursor?.moveToFirst() == true) {
              val noteIdIndex = cardCursor.getColumnIndex(FlashCardsContract.ReviewInfo.NOTE_ID)
              val cardIdIndex = cardCursor.getColumnIndex(FlashCardsContract.ReviewInfo.CARD_ORD)
              val buttonCountIndex = cardCursor.getColumnIndex(FlashCardsContract.ReviewInfo.BUTTON_COUNT)
              val nextReviewIndex = cardCursor.getColumnIndex(FlashCardsContract.ReviewInfo.NEXT_REVIEW_TIMES)
              val mediaFilesIndex = cardCursor.getColumnIndex(FlashCardsContract.ReviewInfo.MEDIA_FILES)
          
              if (noteIdIndex > -1 && cardIdIndex > -1) {
                  val noteId = cardCursor.getString(noteIdIndex)
                  val specificCardId = cardCursor.getString(cardIdIndex)
                  val buttonCount = cardCursor.getString(buttonCountIndex)
                  val reviewIndex = cardCursor.getString(nextReviewIndex)
                  val mediaFiles = cardCursor.getString(mediaFilesIndex)

Maybe the issue is that the deck I'm trying to work with is not following some guidelines?

piotrbrzezina avatar Sep 28 '24 06:09 piotrbrzezina

Sorry - I honestly don't know, this may be a case of needing to get AnkiDroid building locally and adding debug code into the part of AnkiDroid that populates (or should populate) the media files, and perhaps doing the same with the api that gets built into your app. I apologize but I won't have time to pursue this myself, so getting a locally test/debug environment for the other side of the API seems like fastest path forward

mikehardy avatar Sep 28 '24 22:09 mikehardy

Relevant:

https://github.com/ankidroid/Anki-Android/blob/2910ce55a8eb47991d1e750f20ada8b059e98e73/AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt#L1077

Also not going to have time to dig deep here, apologies!

https://github.com/ankidroid/Anki-Android/blob/c63e93a09f86e455ea8c1e9ba13e6ae572550957/AnkiDroid/src/main/java/com/ichi2/libanki/Media.kt#L60-L72

david-allison avatar Oct 03 '24 02:10 david-allison

My presumption is that filesInStr was broken in 7a65160e0e23e20080091a30abdd4c05fc610e06

I suspect it previously returned images and audio/video, and now only returns audio/video.

A unit test will confirm

david-allison avatar Oct 03 '24 02:10 david-allison

Good first issue: Produce a unit test which confirms this hypothesis

It should fail on main, and pass before 7a65160e0e23e20080091a30abdd4c05fc610e06

Assuming the test does portray a problem:

Either:

  • @Ignore the test, and add a PR (preferred for less experienced developers)
  • Fix the issue and check in the fix, along with a passing test

There are many sample tests to follow:

https://github.com/ankidroid/Anki-Android/blob/06e8ac2ef3e0d35fb3efc59aabb38c796b276b04/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt#L975-L1028

david-allison avatar Oct 03 '24 02:10 david-allison

Oh my, this:

My presumption is that filesInStr was broken in https://github.com/ankidroid/Anki-Android/commit/7a65160e0e23e20080091a30abdd4c05fc610e06

...is correct. It was definitely broken then. And as near as I can tell there is a lot of code and infrastructure around parsing out AV tags (audio, or video) but nothing about images in the upstream anki (where this is all implemented).

A unit test will certainly confirm this

I think the most correct path is likely to lobby for + propose a PR into upstream anki that also implements something like extract_img_tags here next to extract_av_tags https://github.com/ankitects/anki/blob/9ced6be03ec25afef66452a655c88a16bb5eb607/rslib/src/card_rendering/mod.rs#L19 - including fresh parsing of those nodes and the protobuf request/response etc

I think the most expedient path is likely to be re-implementing something like the logic of Media.filesInStr before dropping legacy backend, but focused solely on card contents text parse for images, and adding those to the list already returned by the AV tag backend method: https://github.com/ankidroid/Anki-Android/blame/4d39b5b9dd932d86826dd9dacc5ec2d94603046a/AnkiDroid/src/main/java/com/ichi2/libanki/Media.kt#L218

mikehardy avatar Oct 03 '24 13:10 mikehardy

I'd prefer to reimplement our old code, and leave a lower priority TODO to make it faster by implementing it in the backend

Just for speed of the bugfix

david-allison avatar Oct 04 '24 11:10 david-allison

I agree with that, especially since the old code objectively worked - not saying it's an awesome path or anything just saying that pulling the old code that found images from prior commits on git blame has a high chance of "just working" since the code actually did work...

mikehardy avatar Oct 04 '24 16:10 mikehardy

Started on a Unit Test. Audio is also not working, as we pass in [anki:play tags to the function

david-allison avatar Oct 06 '24 20:10 david-allison

Please assign me

CODE-RED-101 avatar Oct 07 '24 00:10 CODE-RED-101

@CODE-RED-101

Brief notes:

  • Add two unit tests in ContentProviderTest:
    • One with no media
    • One with audio and video: Use: """<img src="img.jpg"> [sound:test.mp3]""" as a test case

Each audio and video file should be listed once.

Note that this is different from past behaviour, where media files were listed multiple times.

Then fix the bug. Use the code from before the linked commit as a base, but change the call to col.media.filesInStr(currentCard)

This is because .question() and .answer() now produce rendered media tags as [anki:play..., and you cannot obtain the media names from this output. Instead use card.renderOutput, which provides a collection of media files to be used.

david-allison avatar Oct 07 '24 02:10 david-allison

@david-allison no update for more than 2 months. Can I work on this?

ujjol1234 avatar Jan 04 '25 12:01 ujjol1234

Sure

david-allison avatar Jan 04 '25 12:01 david-allison

Hi @david-allison, just to confirm I have to write Unit tests for the filesInStr function right? To check weather it can return images or not?

ujjol1234 avatar Jan 05 '25 13:01 ujjol1234

https://github.com/ankidroid/Anki-Android/issues/17062#issuecomment-2395749164 should be sufficient. I don't believe filesInStr is what you want to work with. Use the public API surface to be sure the value is correct

david-allison avatar Jan 05 '25 13:01 david-allison

Hi @david-allison, I was unwell last week so apologies for my delayed response. I am currently working on the unit test to verify the addition of media files, but I am facing an issue accessing the ReviewInfo from the query to retrieve the media_files. Could you please guide me on where I might be going wrong? Here's my code so far:

    @Test
    fun testQueryAddsMediaFilesCorrectly() {
        val frontContent = """<img src="img.jpg"> [sound:test.mp3]"""
        val backContent = """[sound:back.mp3]"""
        val note = addNoteUsingBasicModel(frontContent,backContent)
        val ord = 0

        val noteUri = Uri.withAppendedPath(FlashCardsContract.Note.CONTENT_URI,note.id.toString())

        val cardsUri = Uri.withAppendedPath(noteUri, "cards")
        val specificCardUri = Uri.withAppendedPath(cardsUri, ord.toString())

        val projection = arrayOf(
            FlashCardsContract.ReviewInfo.MEDIA_FILES
        )


        contentResolver.query(
            specificCardUri,
            projection,
            null,
            null,
            null
        )?. let { cursor ->
            if (!cursor.moveToFirst()) {
                fail("failed")
            }


            assertNotNull("Cursor should not be null", cursor)
        }

    }

When I try to log the cursor content I am getting this: note_id: 1736883696550, ord: 0, card_name: Card 1, deck_id: 1, question: <img src="img.jpg"> [sound:test.mp3], answer: <img src="img.jpg"> [sound:test.mp3]

ujjol1234 avatar Jan 14 '25 21:01 ujjol1234

@ujjol1234 MEDIA_FILES is in ReviewInfo, not Card

david-allison avatar Jan 14 '25 21:01 david-allison

@david-allison you are absolutely right. It was my mistake. I have corrected the code and my unit test seems to be working fine now. Here's the code for the test:

    @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]


        card.queue = QueueType.New
        card.due = col.sched.today
        col.updateCard(card)

        contentResolver.query(
            FlashCardsContract.ReviewInfo.CONTENT_URI,
            null,
            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

            for (ele in mediaFilesArray.split(",")){
                if(imageFileName == ele){
                    imageFilePresent =true
                }
                if(audioFileName == ele){
                    audioFilePresent = true
                }
            }
            if(imageFilePresent and  audioFilePresent){allMediaFilesPresent = true}

            assertTrue("All media files should be present in the media_files array",allMediaFilesPresent)

        }

    }

Now I am trying to fix the bug...

ujjol1234 avatar Jan 15 '25 21:01 ujjol1234

Quick refactor of the above:

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 1a8fb3e3b1b0347cfc297e24ae040a9d901f8cd8)
+++ b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ContentProviderTest.kt	(date 1736990564130)
@@ -52,6 +52,7 @@
 import com.ichi2.utils.emptyStringArray
 import net.ankiweb.rsdroid.exceptions.BackendNotFoundException
 import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.contains
 import org.hamcrest.Matchers.containsString
 import org.hamcrest.Matchers.equalTo
 import org.hamcrest.Matchers.greaterThan
@@ -329,6 +330,24 @@
         col.notetypes.rem(noteType)
     }
 
+    @Test
+    fun testMediaFilesAddedCorrectlyInReviewInfo() {
+        val imageFileName = "img.jpg"
+        val audioFileName = "test.mp3"
+        val note = addNoteUsingBasicModel(
+            """Hello <img src="$imageFileName"> [sound:$audioFileName]""",
+            "backContent"
+        )
+         note.firstCard(col).update {
+            queue = QueueType.New
+            due = col.sched.today
+        }
+
+        val media = contentResolver.queryAnkiMediaFiles()
+        assertThat("image found", media, contains(imageFileName))
+        assertThat("audio found", media, contains(audioFileName))
+    }
+
     /**
      * Check that inserting and removing a note into default deck works as expected
      */
@@ -1436,3 +1455,20 @@
         unburyDeck(did)
     }
 }
+
+
+fun ContentResolver.queryAnkiMediaFiles(): List<String> {
+    this.query(
+        FlashCardsContract.ReviewInfo.CONTENT_URI,
+        null,
+        null,
+        null,
+        null
+    ).use { cursor ->
+        assertNotNull(cursor)
+        assertTrue("cursor has elements", cursor.moveToFirst())
+        return cursor
+            .getString(cursor.getColumnIndex(FlashCardsContract.ReviewInfo.MEDIA_FILES))
+            .split(",")
+    }
+}
\ No newline at end of file
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 1a8fb3e3b1b0347cfc297e24ae040a9d901f8cd8)
+++ b/AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt	(date 1736901346504)
@@ -157,6 +157,11 @@
         col.updateCard(this, true)
     }
 
+    fun Card.update(block: Card.() -> Unit) {
+        block(this)
+        col.updateCard(this)
+    }
+
     @DuplicatedCode("This is copied from RobolectricTest. This will be refactored into a shared library later")
     internal fun addNoteUsingBasicModel(
         front: String = "Front",

david-allison avatar Jan 16 '25 01:01 david-allison

Hi @david-allison, I hope you are doing well. I am trying to fix the bug now and I had a quick question, should I work on changing the current implementation of fileInStr function to this?

fun filesInStr(mid: Long?, string: String, includeRemote: Boolean = false): List<String> {
    val l: MutableList<String> = ArrayList()
    val model = col.models.get(mid!!)
    var strings: MutableList<String?> = ArrayList()

    if (model!!.isCloze && string.contains("{{c")) {
        // if the field has clozes in it, we'll need to expand the possibilities so we can render LaTeX
        strings = _expandClozes(string)
    } else {
        strings.add(string)
    }

    for (s in strings) {
        @Suppress("NAME_SHADOWING")
        var s = s

        // handle LaTeX
        @KotlinCleanup("change to .map { }")
        val svg = model.optBoolean("latexsvg", false)
        s = LaTeX.mungeQA(s!!, col, svg)

        // extract filenames
        var m: Matcher
        for (p in REGEXPS) {
            // NOTE: python uses the named group 'fname'. Java doesn't have named groups, so we have to determine
            // the index based on which pattern we are using
            val fnameIdx = if (p == fSoundRegexps) 2 else if (p == 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
}

or should I try something else?

ujjol1234 avatar Jan 21 '25 17:01 ujjol1234

Hi @ujjol1234

With apologies, I'm busy this week, could you try our Discord, or ping me next Monday

david-allison avatar Jan 21 '25 17:01 david-allison

Sure. Thank you for letting me know. I completely understand! I'll reach out on Discord in the meantime, and if needed, I'll ping you again next Monday.

ujjol1234 avatar Jan 21 '25 18:01 ujjol1234

Prior direction was:

Then fix the bug. Use the code from before the linked commit as a base, but change the call to col.media.filesInStr(currentCard)

This is because .question() and .answer() now produce rendered media tags as [anki:play..., and you cannot obtain the media names from this output. Instead use card.renderOutput, which provides a collection of media files to be used.

Prior filesInStr was what you proposed, however the signature needs to change:

fun filesInStr(mid: Long?, string: String, includeRemote: Boolean = false): List<String> {

should become more like

fun filesInStr(currentCard: Card, includeRemote: Boolean = false): List<String> {

and then I assume you want to call currentCard.renderOutput

https://github.com/ankidroid/Anki-Android/blob/a9b95293f35b57857c96ef47232fe168034383e7/AnkiDroid/src/main/java/com/ichi2/libanki/Card.kt#L187-L196

It looks like it returns a type that has arrays with the AV tags that filesInStr previous logic should be able to work through?

https://github.com/ankidroid/Anki-Android/blob/a9b95293f35b57857c96ef47232fe168034383e7/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt#L234-L239

mikehardy avatar Jan 21 '25 18:01 mikehardy

Thank you for the help @mikehardy. I’ll try this approach and let you know how it goes.

ujjol1234 avatar Jan 21 '25 18:01 ujjol1234

@mikehardy Thanks for the guidance earlier! I followed the steps you provided and I have fixed the filesInStr function.

ujjol1234 avatar Jan 22 '25 19:01 ujjol1234

Hello 👋, this issue has been opened for more than 3 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

github-actions[bot] avatar Apr 22 '25 20:04 github-actions[bot]