JS function to setTags and getTags #17795

Haz3-jolt · 2025-01-11T13:35:04Z

Purpose / Description

Adds two JS API functions [set/replace]NoteTags and getTagsFromNote which act similar to PUT and GET from a REST API

Fixes

Fixes FR: JS API function to remove a specific tag #17716

Approach

Adds two methods to allow the caller to get Tags and update/set tags.

How Has This Been Tested?

Added two unit tests both verified and passing.

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

Haz3-jolt · 2025-01-11T13:37:13Z

@david-allison Do you think we should deprecate addTagToNote? Also [set/replace]NoteTags triggers lint failure

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt

mikehardy

Do you think we should deprecate addTagToNote

Yes - haven't done our deprecation strategy in a while, not sure exactly how. Is it documented anywhere? May not be, but we should be able to mark something as deprecated and have it emit a warning in console or similar

Haz3-jolt · 2025-01-11T16:06:25Z

Do you think we should deprecate addTagToNote

Yes - haven't done our deprecation strategy in a while, not sure exactly how. Is it documented anywhere? May not be, but we should be able to mark something as deprecated and have it emit a warning in console or similar

From some searching, the most I can find is the deprecated decorator on Kotlin side, I dunno how we would approach on js. Is there official documentation for JS api? if not we can just have a comment?

Haz3-jolt · 2025-01-11T16:09:38Z

Thanks for bringing this up. Here's my perspective...

Hey! Can you you elaborate further?

mikehardy · 2025-01-11T16:19:40Z

@Mizokuiam please stop. You are wasting everyone's time.

Haz3-jolt · 2025-01-11T16:22:20Z

@Mizokuiam please stop. You are wasting everyone's time.

Spammer? :(

mikehardy · 2025-01-11T16:33:15Z

@Haz3-jolt I assume so. Negative value contributions, banned at organization level by me now.

AnkiDroid/src/main/assets/scripts/js-api.js

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt

Haz3-jolt · 2025-01-11T18:41:33Z

@mikehardy How is this for a deprecation warning? Since people mainly interact on Js side

Subject: [PATCH] JS function to setTags and getTags
---
Index: AnkiDroid/src/main/assets/scripts/js-api.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/assets/scripts/js-api.js b/AnkiDroid/src/main/assets/scripts/js-api.js
--- a/AnkiDroid/src/main/assets/scripts/js-api.js	(revision 6d3d270e9067fbad4797f25b0f3060adc60e41cd)
+++ b/AnkiDroid/src/main/assets/scripts/js-api.js	(date 1736620228061)
@@ -121,6 +121,7 @@
 Object.keys(jsApiList).forEach(method => {
     if (method === "ankiAddTagToNote") {
         AnkiDroidJS.prototype[method] = async function (noteId, tag) {
+            console.warn("ankiAddTagToNote is deprecated. Use ankiSetNoteTags instead.");
             const endpoint = jsApiList[method];
             const data = JSON.stringify({ noteId, tag });
             return await this.handleRequest(endpoint, data);

mikehardy · 2025-01-11T18:50:20Z

@Haz3-jolt I believe that deprecation in javascript console is the correctly location for it

Haz3-jolt · 2025-01-11T18:54:03Z

@Haz3-jolt I believe that deprecation in javascript console is the correctly location for it

Amend the current commit or separate commit?

mikehardy · 2025-01-11T18:56:28Z

amend I think, the deprecation of the old is part of adding the new to me, it's all one piece

Haz3-jolt · 2025-01-11T18:57:17Z

amend I think, the deprecation of the old is part of adding the new to me, it's all one piece

Cool!

david-allison · 2025-01-11T21:24:23Z

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt

+                    getColUnsafe.getNote(noteId).apply {
+                        setTagsFromStr(getColUnsafe, tags.stringIterable().joinToString(","))
+                    }
+                getColUnsafe.updateNote(note)


This is one for an issue, not for this PR.

updateNote is undoable, but we don't handle it here.

We also have tons of getColUnsafe uses here which should be fixed.

Oh ok ok.

Btw why is getColUnsafe meant to be avoided?

We should be accessing the Collection using a mutex (to avoid concurrent access).

This was called getCol() in Java, and we had various problems with the collection being closed while operations were occurring. withCol { } ensures that calls are serialized correctly.

@david-allison what do you think of

Subject: [PATCH] JS function to setTags and getTags --- Index: AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt --- a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt (revision 3300a42402970f5eceaee01262442a0becd14508) +++ b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt (date 1736620370730) @@ -380,19 +380,22 @@ val jsonObject = JSONObject(apiParams) val noteId = jsonObject.getLong("noteId") val tags = jsonObject.getJSONArray("tags") - val note = - getColUnsafe.getNote(noteId).apply { - setTagsFromStr(getColUnsafe, tags.stringIterable().joinToString(",")) + withCol { + val note = getNote(noteId).apply { + setTagsFromStr(this@withCol, tags.stringIterable().joinToString(",")) } - getColUnsafe.updateNote(note) + updateNote(note) + } convertToByteArray(apiContract, true) } "getNoteTags" -> { val jsonObject = JSONObject(apiParams) val noteId = jsonObject.getLong("noteId") - val note = getColUnsafe.getNote(noteId) - convertToByteArray(apiContract, JSONArray(note.tags).toString()) + val noteTags = withCol { + getNote(noteId).tags + } + convertToByteArray(apiContract, JSONArray(noteTags).toString()) } "sttSetLanguage" -> convertToByteArray(apiContract, speechRecognizer.setLanguage(apiParams))

Ah, didn't realize it was already a suspend fun, looks great!

Haz3-jolt · 2025-01-14T17:06:44Z

@david-allison did a quick patch test for unit testing the two api calls

Subject: [PATCH] JS function to setTags and getTags
---
Index: AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt	(revision 1373d9a6adbe2071d8fe8416b441523044ef4bda)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt	(date 1736873379876)
@@ -417,6 +417,24 @@
             assertEquals(CardType.New, cardAfterReset.type, "Card type after reset")
         }
 
+    @Test
+    fun ankiGetNoteTagsTest() =
+        runTest {
+            val n = addBasicNote("Front", "Back")
+            n.addTag("tag1")
+            n.addTag("tag2")
+            n.addTag("tag3")
+
+            val reviewer: Reviewer = startReviewer()
+            waitForAsyncTasksToComplete()
+
+            val jsapi = reviewer.jsApi
+
+            // test get tags for note
+            println(n.tags)
+            println(getDataFromRequest("getNoteTags", jsapi, jsonObjectOf("noteId" to n.id)))
+        }
+
     companion object {
         fun jsApiContract(data: String = ""): ByteArray =
             JSONObject()
@@ -450,5 +468,21 @@
             jsAPI
                 .handleJsApiRequest(methodName, jsApiContract(apiData), false)
                 .decodeToString()
+
+        suspend fun getDataFromRequest(
+            methodName: String,
+            jsAPI: AnkiDroidJsAPI,
+            apiData: JSONObject,
+        ): String =
+            jsAPI
+                .handleJsApiRequest(methodName, jsApiContract(apiData.toString()), false)
+                .decodeToString()
     }
 }
+
+
+private fun jsonObjectOf(vararg pairs: Pair<String, Any>): JSONObject = JSONObject().apply {
+    for ((key, value) in pairs) {
+        put(key, value)
+    }
+}
\ No newline at end of file

But in output I noticed a discrepancy:
[tag1, tag2, tag3]
{"success":true,"value":"[]"}

Perhaps an array is not actually being returned here? could this be an issue with getDataFromRequest or handleJsApiRequest

Haz3-jolt · 2025-01-17T21:20:55Z

Subject: [PATCH] JS function to setTags and getTags
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt	(revision bc86eea4642d2dde773f8f7f32db7481dd0b4565)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt	(date 1737148207863)
@@ -397,6 +397,11 @@
                     withCol {
                         getNote(noteId).tags
                     }
+                //Temp logging
+                println("NotedID = $noteId")
+                println("NoteTags = $noteTags")
+                println(JSONArray(noteTags).toString())
+
                 convertToByteArray(apiContract, JSONArray(noteTags).toString())
             }
 
Index: AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt	(revision bc86eea4642d2dde773f8f7f32db7481dd0b4565)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/AnkiDroidJsAPITest.kt	(date 1737148464912)
@@ -27,6 +27,7 @@
 import net.ankiweb.rsdroid.withoutUnicodeIsolation
 import org.hamcrest.CoreMatchers.equalTo
 import org.hamcrest.MatcherAssert.assertThat
+import org.json.JSONArray
 import org.json.JSONObject
 import org.junit.Ignore
 import org.junit.Test
@@ -417,6 +418,41 @@
             assertEquals(CardType.New, cardAfterReset.type, "Card type after reset")
         }
 
+    @Test
+    fun ankiGetNoteTagsTest() =
+        runTest {
+            val n = addBasicNote("Front", "Back")
+            n.tags = mutableListOf("tag1", "tag2", "tag3")
+
+            val reviewer: Reviewer = startReviewer()
+            waitForAsyncTasksToComplete()
+
+            val jsapi = reviewer.jsApi
+
+            // test get tags for note
+            println(n.tags)
+            println(getDataFromRequest("getNoteTags", jsapi, jsonObjectOf("noteId" to n.id)))
+        }
+
+    @Test
+    fun ankiSetNoteTagsTest() = runTest {
+        val n = addBasicNote("Front", "Back")
+        n.addTag("tag1")
+        n.addTag("tag2")
+        n.addTag("tag3")
+
+        val reviewer: Reviewer = startReviewer()
+        waitForAsyncTasksToComplete()
+
+        val jsapi = reviewer.jsApi
+
+        // test set tags for note
+        val newTags = JSONArray().put("tag4").put("tag5").put("tag6")
+        println(newTags)
+        println(getDataFromRequest("setNoteTags", jsapi, jsonObjectOf("noteId" to n.id, "tags" to newTags)))
+        println(n.tags)
+    }
+
     companion object {
         fun jsApiContract(data: String = ""): ByteArray =
             JSONObject()
@@ -450,5 +486,21 @@
             jsAPI
                 .handleJsApiRequest(methodName, jsApiContract(apiData), false)
                 .decodeToString()
+
+        suspend fun getDataFromRequest(
+            methodName: String,
+            jsAPI: AnkiDroidJsAPI,
+            apiData: JSONObject,
+        ): String =
+            jsAPI
+                .handleJsApiRequest(methodName, jsApiContract(apiData.toString()), false)
+                .decodeToString()
     }
 }
+
+
+private fun jsonObjectOf(vararg pairs: Pair<String, Any>): JSONObject = JSONObject().apply {
+    for ((key, value) in pairs) {
+        put(key, value)
+    }
+}
\ No newline at end of file

@david-allison Did some further logging on the API side, and found something interesting while logging.

[tag1, tag2, tag3]
NotedID = 1737141291287
NoteTags = []
[]
{"success":true,"value":"[]"}

So the tags are accessible on the test side, and the noteId is correctly passed to the API, but despite this the API is not able to access the tags? perhaps a sync diff?

Haz3-jolt · 2025-01-18T06:29:54Z

Should I make a separate commit for the Unit Tests or just amend commit?

david-allison · 2025-01-18T13:47:38Z

Typically, include the tests in the same commit as the test they're changing

If you're fixing a bug, you can include an ignored test, and un-ignore it in the commit which fixes the bug (not the case here, as it's a feature)

For your testing infra improvements: up to you of you want to make a new commit before your code changes. If you do, see if you can apply the pattern to existing test code

Haz3-jolt · 2025-01-18T17:26:16Z

@david-allison finally got both tests working, but for the setNoteTags test, I had to involve some unholy string manipulation, because I think there is a bug in the library during downstream conversion of a JSONArray, because when I had

val newTags = JSONArray().apply {
        put("tag4") 
        put("tag5") 
        put("tag6") 
    }

when checking logs:
New Tags: [tag4,tag5,tag6]
Size:1

With some whitespaces:

val newTags = JSONArray().apply {
        put("tag4") 
        put(" tag5") 
        put(" tag6") 
    }

when checking logs:
New Tags: [tag4,, tag5,, tag6]
Size: 3

Haz3-jolt · 2025-01-18T17:29:13Z

I think during downstream serialization during the back and forth JSON to string to JSON not having a whitespace or inherent delimiter breaks it on Kotlin side (Probably a non issue when actually using the jsapi in Js), causing it to serialize the JSONArray as a single object, I did try using the JSON serialization library to convert a mutableList into a JSONArray, but that didn't work out.

david-allison · 2025-01-18T17:42:56Z

@Haz3-jolt Please spend some time going through the Anki Desktop source and our Tags class.

If we have an issue, we need to deal with this properly.

Haz3-jolt · 2025-01-18T17:55:24Z

@Haz3-jolt Please spend some time going through the Anki Desktop source and our Tags class.

If we have an issue, we need to deal with this properly.

Sure!

Haz3-jolt · 2025-01-18T18:42:01Z

@david-allison false alert! before diving head first into anki or Tags class, I wanted to make sure I wasn't doing anything wrong,
so I did further logging at every single stage, and found the culprit!, in

"setNoteTags" -> {
                val jsonObject = JSONObject(apiParams)
                val noteId = jsonObject.getLong("noteId")
                val tags = jsonObject.getJSONArray("tags")
                withCol {
                    val note =
                        getNote(noteId).apply {
                            setTagsFromStr(this@withCol, tags.stringIterable().joinToString(","))
                        }
                    updateNote(note)
                }
                convertToByteArray(apiContract, true)
            }

I misunderstood what the separator in joinToString was and thought it was a delimiter, when replaced it with whitespace, it works!

Haz3-jolt · 2025-01-18T18:42:19Z

will redo the commits!

david-allison · 2025-01-18T19:36:57Z

Fantastic! Thanks for that

Haz3-jolt · 2025-01-19T07:15:06Z

Hi @david-allison I think the test was flaky, error was due to execution time? They both pass on my local machine, can you try restarting the job?

mikehardy reviewed Jan 11, 2025

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt Outdated Show resolved Hide resolved

mikehardy reviewed Jan 11, 2025

View reviewed changes

This comment was marked as spam.

Sign in to view

Haz3-jolt force-pushed the setTags branch 2 times, most recently from 783f9fa to 36e8da6 Compare January 11, 2025 16:02

Haz3-jolt force-pushed the setTags branch from 36e8da6 to fbdca30 Compare January 11, 2025 16:08

david-allison reviewed Jan 11, 2025

View reviewed changes

AnkiDroid/src/main/assets/scripts/js-api.js Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt Outdated Show resolved Hide resolved

Haz3-jolt force-pushed the setTags branch from fbdca30 to 6d3d270 Compare January 11, 2025 18:35

Haz3-jolt force-pushed the setTags branch from 6d3d270 to 3300a42 Compare January 11, 2025 18:57

david-allison added the Needs Review label Jan 11, 2025

david-allison approved these changes Jan 11, 2025

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jan 11, 2025

Haz3-jolt requested a review from mikehardy January 12, 2025 12:31

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

Haz3-jolt force-pushed the setTags branch from 3300a42 to bc86eea Compare January 14, 2025 09:35

This comment has been minimized.

Sign in to view

Haz3-jolt force-pushed the setTags branch from de9b988 to 24da72f Compare January 18, 2025 19:02

Haz3-jolt added 2 commits January 18, 2025 23:06

JS function to setTags and getTags

7f65fde

Unit Tests for getNoteTags and setNoteTags API calls

633f2a3

Haz3-jolt force-pushed the setTags branch from 24da72f to 633f2a3 Compare January 18, 2025 19:09

Haz3-jolt requested a review from david-allison January 18, 2025 19:10

JS function to setTags and getTags #17795

Are you sure you want to change the base?

JS function to setTags and getTags #17795

Conversation

Haz3-jolt commented Jan 11, 2025 • edited Loading

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Haz3-jolt commented Jan 11, 2025

mikehardy left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

Haz3-jolt commented Jan 11, 2025

Haz3-jolt commented Jan 11, 2025

mikehardy commented Jan 11, 2025

Haz3-jolt commented Jan 11, 2025

mikehardy commented Jan 11, 2025

Haz3-jolt commented Jan 11, 2025

mikehardy commented Jan 11, 2025

Haz3-jolt commented Jan 11, 2025

mikehardy commented Jan 11, 2025

Haz3-jolt commented Jan 11, 2025

david-allison Jan 11, 2025

Choose a reason for hiding this comment

Haz3-jolt Jan 12, 2025

Choose a reason for hiding this comment

david-allison Jan 13, 2025

Choose a reason for hiding this comment

Haz3-jolt Jan 14, 2025

Choose a reason for hiding this comment

david-allison Jan 14, 2025

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment has been minimized.

This comment has been minimized.

Haz3-jolt commented Jan 14, 2025

Haz3-jolt commented Jan 17, 2025

This comment has been minimized.

This comment has been minimized.

Haz3-jolt commented Jan 18, 2025

david-allison commented Jan 18, 2025

Haz3-jolt commented Jan 18, 2025

Haz3-jolt commented Jan 18, 2025 • edited Loading

david-allison commented Jan 18, 2025

Haz3-jolt commented Jan 18, 2025

Haz3-jolt commented Jan 18, 2025

Haz3-jolt commented Jan 18, 2025

david-allison commented Jan 18, 2025

Haz3-jolt commented Jan 19, 2025

Haz3-jolt commented Jan 11, 2025 •

edited

Loading

Haz3-jolt commented Jan 18, 2025 •

edited

Loading