convert synchronous js api to asynchronous and remove js interface

[krmanik]

Purpose / Description

The JS API is currently synchronous, and I think it might be a good idea to rethink this before people start using it in earnest. There may be a non-trivial delay between the time a request is made and it can be fulfilled, as another operation may already be running on the collection, and I/O may be slow. While the handler on the Kotlin side is called in a background thread, the JS side will block during that time, so the web interface will appear stuck until the request completes.

Fixes

• Fixes JS API should probably be asynchronous #14226

Approach

The JavaScript interface is removed. The idea for asynchronous implementation is inspired by this commit (aa13aebc8eb6f86). A JavaScript file containing all the JS API implementations is created in the assets directory. The fetch API is used to send HTTP (GET/POST) requests, and incoming requests are handled on the Kotlin side of the application.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

• Test to be updated/implemented
• A deck is created with all js api test
  AnkiDroid JS API Test.zip

Example test, for front field in deck

{{Front}}

<div id="result"></div>

<script>
    var jsApi = { "version": "0.0.2", "developer":  /*enter your email here*/ };

    var apiStatus = AnkiDroidJS.init(jsApi).then(function (response) {
        console.log(response);

        api = JSON.parse(JSON.stringify(response));

        if (api['markCard']) {
            console.log("API mark card available");
        }

        if (api['toggleFlag']) {
            console.log("API toggle flag available");
        }
        document.getElementById("result").innerHTML = JSON.stringify(response, null, "\t");

    });
</script>

Click to see more detailed example

{{Front}}

<div>
    Mark Card Test
    <button onclick='api.ankiMarkCard(1)'>Mark Card</button>
</div>
<br />
<div>
    Flag Test
    <button onclick='api.ankiToggleFlag("red")'>Red Flag</button>
    <button onclick='api.ankiToggleFlag("green")'>Green Flag</button>
</div>
<br />
<div>
    TTS Test
    <div>
        Text
        <input type='text' id='ttsText' value='Hello World!'>
    </div>
    <div>
        Language
        <input type='text' id='ttsLanguage' value='en-US'>
    </div>

    <div>
        Pitch
        <input type='text' id='ttsPitch' value='1.0'>
    </div>

    <div>
        Speech Rate
        <input type='text' id='ttsSpeechRate' value='1.0'>
    </div>
    <div id="isSpeaking"></div>
    <div id="isAvailable"></div>
    <button onclick='ttsSpeak()'>Speak</button>
    <button onclick='api.ankiTtsStop()'>Stop</button>

    <script>
        async function ttsIsSpeaking() {
            var result = await api.ankiTtsIsSpeaking();
            document.getElementById("isSpeaking").innerHTML = "Is Speaking: " + result;
        }

        async function ttsIsAvailable() {
            var result = await api.ankiTtsFieldModifierIsAvailable();
            document.getElementById("isAvailable").innerHTML = "Is Available: " + result;
        }

        async function ttsSpeak() {
            var text = document.getElementById("ttsText").value;
            var language = document.getElementById("ttsLanguage").value;
            var pitch = document.getElementById("ttsPitch").value;
            var speechRate = document.getElementById("ttsSpeechRate").value;

            api.ankiTtsSetLanguage(language);
            api.ankiTtsSetPitch(pitch);
            api.ankiTtsSetSpeechRate(speechRate);
            api.ankiTtsSpeak(text, 0);
        }
    </script>
</div>
<br />
<div>
    Search Test
    <div>Search in Card Browser</div>
    <input type='text' id='searchText' value='anki'>
    <button onclick='searchBtn()'>Search</button>
    <br />
    <div>Search in Callback</div>
    <input type='text' id='searchTextCallback' value='anki'><button onclick='someButtonClickFunction()'>Search</button>
    <div id='searchResult'></div>

    <script>
        function searchBtn() {
            api.ankiSearchCard(document.getElementById("searchText").value)
        }

        function parseResult(result) {
            for (res of result) {
                document.getElementById("searchResult").innerHTML += "<br/>" + JSON.stringify(result);
            }
        }

        // add hook for result
        if (typeof AnkiDroidJS !== 'undefined') {
            addHook("ankiSearchCard", parseResult)
        }

        function someButtonClickFunction() {
            api.ankiSearchCardWithCallback(document.getElementById("searchTextCallback").value)
        }
    </script>
</div>
<br />
<div>
    Bury Test
    <button onclick='api.ankiBuryCard(1)'>Bury Card</button>
    <button onclick='api.ankiBuryNote(1)'>Bury Note</button>
</div>
<br />
<div>
    Suspend Test
    <button onclick='api.ankiSuspendCard(1)'>Suspend Card</button>
    <button onclick='api.ankiSuspendNote(1)'>Suspend Note</button>
</div>
<br />
<div>JS API Result</div>
<div id="result"></div>

<script>
    if (typeof AnkiDroidJS !== 'undefined') {
        var jsApiContract = { "version": "0.0.2", "developer": "infinyte01@mail.com" };

        var api = new AnkiDroidJS(jsApiContract);

        async function apiResult() {
            for (var key in api) {
                if (key.startsWith("ankiGet")) {
                    var method = key;
                    var result = await api[key]();
                    document.getElementById("result").innerHTML += "<br>" + method + " : " + result;
                }
            }

            for (var key in api) {
                if (key.startsWith("ankiIs")) {
                    var method = key;
                    var result = await api[key]();
                    document.getElementById("result").innerHTML += "<br>" + method + " : " + result;
                }
            }
        }
        apiResult();
    }
</script>

Learning (optional, can help others)

Describe the research stage

Links to blog posts, patterns, libraries or addons used to solve this problem

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

[mikehardy]

Definitely a positive direction with regard to handling the reality of possible delays and keeping the UI responsive, I think most APIs (ours included) should be async for this reason and this seems like a nice way to go about it. Interested to see how it develops as you work through the CI issues

[dae]

Not related to your recent changes, but I don't really understand the logic of isInit(). You're storing init state on the Kotlin side, so isn't that going to break when you have more than one add-on active? If the first one correctly initialized the API, won't that allow a second add-on that doesn't do so correctly to still access the API? Couldn't this instead be enforced on the JS end?

[dae]

Some recommendations:

• Split the jsAPI checks into a separate function, so this function tests getScheduling/setScheduling/else: handleJsApiPost()
• Consider using a path prefix, eg ankiSearchCard -> /jsapi/searchCard. That way you can strip the prefix and just compare the last part.
• Use POST everywhere. We don't need or want caching, and it will simplify the code
• This method is currently not async because the only two existing uses don't require collection access. But since you're going to be accessing the collection, the correct way to do so would be to use withCol instead of getColUnsafe. I suggest you convert this method to suspend, and use AnkiServer's buildResponse(). That way you don't need runBlocking in each individual method, and it takes care of returning errors properly. The JS API can then withCol as necessary.

[dae]

Are you handling threads correctly at the moment? ReviewerServer runs on a background thread IIRC, so you won't be able to call UI methods directly or access activity. If you switch to suspend as recommended above, then you could use withContext(Dispatchers.Main) {...} in the routines that access UI elements.

[krmanik]

Couldn't this instead be enforced on the JS end?

For current session the API initiated if version and developer mail provided but it needs to change to handle for multiple addon support. Currently only JS API is being used so kotlin side init once. I will move it to JS side for more better check.

If you switch to suspend as recommended above, then you could use withContext(Dispatchers.Main) {...} in the routines that access UI elements.

I have changed the method to suspend and used buildResponse along with withContext(Dispatchers.Main) {...}, but toggleFlag getting empty string I tried to log but issues are not found. Other UI related task like mark card, isDisplaying answer is working.
Found it.

[dae]

The failing tests are likely deadlocks caused by the use of runBlocking. If you wrap the unit tests in runTest { } like ' fun doNotShowOptionsMenuWhenCollectionInaccessible() = runTest {' then you can call suspend funs without needing to use runBlocking, and it may fix the issues.

[dae]

Just a couple more minor issues:

[dae]

Thanks Mani, no further suggestions from my end.

[krmanik]

This is ready for review.
There is one issue with this if once the api is activated then it is available for entire session.
I think my idea will be passing api developer contract for each api request.

[mikehardy]

This is ready for review. There is one issue with this if once the api is activated then it is available for entire session. I think my idea will be passing api developer contract for each api request.

Interesting! I wonder if that will add to the processing overhead / latency a lot? It may not, I don't know. We definitely don't want a sneaky JS plugin to never register with the API (and thus not check compatibility) but work anyway just because a different plugin already registered...

[krmanik]

Then I will update this to pass developer contract for each request.

[mikehardy]

This looks good to me, thank you Mani!

[mikehardy]

I think this does it for me - I like it. I understand there is a tension between a large when / switch statement and separating things out where some people may feel differently

But I feel in an API handler there is always a switch statement somewhere where traffic is sent to the correct logic, and as long as the logic is small (as it is here...) then having it right in the when is in my opinion best because it is easy to see where things are / how to add them --> add them right there and done...

[mikehardy]

@krmanik just wanted to give a specific thanks for working through this with me. Quite an effort when I think about the sustained attention over time, it really adds up. This should be a big improvement to the API while it's still young though as Damien noted. Hopefully this goes in, then we can finish the module management stuff then I believe I/we will have honored all your efforts here with merges + getting them released. That's my dream anyway :-). Cheers

[david-allison]

Looks great!

blocking:

• discussion on returning -1 rather than an object
• searchCards ambiguity

I know Mike's been with this for a long time, so if these have been discussed previously, mark as resolved

Follow-ups can be done later, feel free to make an issue + assign to me

[david-allison]

Implementer's choice

would be cleaner to cast to an interface and call a method if the cast succeeds

[david-allison]

We should rethink the names here:

• searchCard opens the browser
• searchCardWithCallback performs a search and returns results

[krmanik]

During implementation of api, only searchCard are implemented but later user wanted result in reviewer also, so used call back, poor naming choice, it can be solved in separate PR, with better naming, because we are upgrading api, so it can also be upgraded.

[krmanik]

I have updated some review but going to create issue and assigning to you for further refactor and improvement of this.

[david-allison]

Happy with that. A few tests are failing

I'll want to give this a second look through once CI is green, but assume it'll be approved

[david-allison]

One question: global variables in a request pipeline is a concern

[krmanik]

One question: global variables in a request pipeline is a concern

Used data class for parsing developer contract so now there is no global variables.

[david-allison]

Awesome, thanks so much!!!!

[david-allison]

@mikehardy @krmanik
This isn't able to be rebased due to test failures.

Acceptable to squash? I'm happy, but want to confirm due to the size

[krmanik]

It is okay to squash into one commit.

[david-allison]

Thanks so much!

[mikehardy]

Fantastic!

[github-actions]

Hi there @krmanik! This is the OpenCollective Notice for PRs merged from 2023-12-01 through 2023-12-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!