Audio system refactoring

[toasted-nutbread]

Various refactorings/fixes to the audio-related classes.

• Refactored the arguments that are passed to AudiUriBuilder.getUri, AnkiNoteBuilder.injectAudio, and AudioSystem.constructor. The intent is to make it so that only the relevant details are passed, rather than the entire options object or an optionsContext. This was mentioned in the first bullet point of Anki card media injection refactor #427 (comment).
• Disabled audio caching on the backend when creating cards. Caching should not be necessary and could potentially cause unnecessary memory to remain allocated.
• Updated the audio cache to only return a value if the source is in the sources parameter.
• Fixed a bug where the cache didn't work correctly (was using the wrong key).
• Fixed a bug on the audio playback buttons in merged mode where the title text wouldn't be updated properly to display what the audio source is.
• General organization of what data is passed in and where it is coming from, intended to remove redundancies.

[siikamiika]

I think this would be easier to read as

            audioUriBuilder: {
                getUri: async (definition, source, details) => {
                    return await apiAudioGetUri(definition, source, details);
                }
            },

[toasted-nutbread]

Curse you Javascript for giving us multiple ways to do the same thing!

0a9f5f7

[siikamiika]

It would be safer in general to avoid passing mutable object indices in async code. This should be fine because sources is destructured from options (and updated by replacing the entire object), but this could break if the last line was

info = `From source ${1 + index}: ${this.options.audio.sources[index]}`;

There's a similar issue with profile index in #389 (comment)

[toasted-nutbread]

The reason I changed it here was to avoid the now redundant indexOf call. For another point of reference, the backend database also returns indices corresponding to a function argument. These types of functions have a general assumption that the data being passed in is not going to be mutated during execution, and the resulting index is only intended to be consistent with the input value it received at the instance the function is invoked.

Technically, even though you mention that it "should be fine" since sources is destructured from options, it could still "break" if something like this this.options.audio.sources.length = 0; is called from elsewhere, since those both reference the same array. However, at least in this situation, the worst theoretical case is just something like info = 'From source 1: undefined'.

In general, this isn't something that can be guaranteed in all cases without frequently doing something like a deepCopy on arguments, and that comes with a non-trivial amount of overhead. Instead, like most APIs in the project, functions are relying on the developers to use them correctly, at least to a certain extent. Technically it's possible to do functional programming, but it's probably best to not even think about that :^)

[siikamiika]

This was something fresh in mind from doing changes to options code. I agree that it would be overkill to protect against typos like this.options.audio.sources.length = 0; by copying the object every time. Safeguards like this should be opt out rather than opt in to work properly (copy on write and explicit reference). Functional programming can give useful tools and patterns but you have mutate the state to do anything useful 😄

[siikamiika]

There's another issue in audio playback outside of the scope of refactoring only, so it can either be included here or done separately depending on how complex it is to fix.

If you repeatedly click or hold the hotkey to play audio, you will have it played multiple times simultaneously when the server replies. I think this should be fixable by storing a promise and awaiting that for any requests that arrive in the pending state.

[toasted-nutbread]

There's another issue in audio playback outside of the scope of refactoring only, so it can either be included here or done separately depending on how complex it is to fix.

If you repeatedly click or hold the hotkey to play audio, you will have it played multiple times simultaneously when the server replies. I think this should be fixable by storing a promise and awaiting that for any requests that arrive in the pending state.

Fixed in 9c6845e.

Also fixed another potential console warning that could occur due to audio.play() being async in modern browsers: e29527f. See: https://developers.google.com/web/updates/2017/06/play-request-was-interrupted

[siikamiika]

Fixed in 9c6845e.

The request still seems to be done multiple times (at least according to the Firefox dev console), but the sound doesn't overlap anymore.

Also fixed another potential console warning that could occur due to audio.play() being async in modern browsers: e29527f. See: https://developers.google.com/web/updates/2017/06/play-request-was-interrupted

Thanks 👍

[toasted-nutbread]

The request still seems to be done multiple times (at least according to the Firefox dev console), but the sound doesn't overlap anymore.

I will leave this as a task to fix at another time, since this issue is not introduced by this change. For posterity, an easy way to trigger the issue is to switch to JapanesePod101 (Alternate) audio source and replace:

    async audioPlay(definition, expressionIndex, entryIndex) {

with:

    async audioPlay(definition, expressionIndex, entryIndex) {
        const v = this.audioPlay2(definition, expressionIndex, entryIndex);
        this.audioPlay2(definition, expressionIndex, entryIndex);
        return await v;
    }

    async audioPlay2(definition, expressionIndex, entryIndex) {