Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add APIs for IO card rendering #2739

Merged
merged 10 commits into from
Oct 19, 2023
Merged

Conversation

glutanimate
Copy link
Contributor

@glutanimate glutanimate commented Oct 15, 2023

Extends IO with a number of JS APIs that allow altering IO card rendering:

  • Add onWillDrawShapes and onDidDrawShapes callback parameters to IO setup function, allowing consumers to modify rendering both before and after the occlusion masks are drawn
  • Add cloze ordinal to Shape objects
  • Allow consumers to draw their own shapes directly by exposing drawShape and shape classes

Refactoring changes as a result of these additions:

  • Create a new namespace for all public IO APIs available at card render time under globalThis.anki.imageOcclusion
  • Accordingly, change the image occlusion setup call from anki.setupImagecloze() to anki.imageOcclusion.setup() while retaining backwards compatibility with old templates
  • Update (de)normalization of shapes to be non-mutating. Mutating shapes would likely be a recipe for trouble when combined with IO API use by external consumers.
  • Consistently use "image occlusion" terminology for all identifiers, compared to the previous mix of "image occlusion" and "image cloze"

I was a bit unsure on the best way to add these APIs, walking through multiple different implementations and e.g. also experimenting with an extended hook system, but I eventually ended up settling on this template-centric approach with callback parameters.

(FWIW, pushed the extended hook system here, in case it could still come in handy in the future)

One point still has me a bit confused, however. We currently expose globals in the reviewer context through a number of means:

  • For somewhat older private APIs used in Python → JS communication (e.g. _drawMark), we export the corresponding symbols in reviewer/index.ts
  • For newer public APIs like mutateNextCardStates and setupImageCloze, we register them to a globalThis.anki object. We do so in two places, reviewer/index.ts (for desktop) and reviewer/reviewer_extras.ts (I assume for AnkiMobile, AnkiWeb and maybe AnkiDroid?)
  • Finally we have public template APIs like onUpdateHook that are both exposed via exports and registerPackage, but do not seem to be exposed in reviewer/reviewer_extras.ts, while still being available on mobile

Which option, or combination thereof would you recommend using here? Given that we were already using the anki global for setupImageCloze, that's what I ended up going with. But I was wondering if we have started migrating more towards the registerPackage approach (or started and then stopped, given mutateNextCardStates and setupImageCloze?).

We currently use "image occlusion" in most places, but some references to "image cloze" still remain. For consistency's sake and to make it easier to quickly find IO-related code, this commit replaces all remaining references to "image cloze", only maintaining those required for backwards compatibility with existing note types.
Mutating shapes would be a recipe for trouble when combined with IO API use by external consumers.

(makeNormal(makeAbsolute(makeNormal())) is not idempotent,
and keeping track of the original state would introduce
additional complexity with no discernible performance benefit
or otherwise.)
For consistency with previous implementation
export { extractShapesFromRenderedClozes } from "./from-cloze";
export { Polygon } from "./polygon";
export { Rectangle } from "./rectangle";
export { Text } from "./text";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some people consider this an anti-pattern, and I've started to feel the same way. I know we have other examples in the codebase already that do this, but I'd suggest going forward, we limit this to public API exports, and avoid making these for our own use.

https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can definitely see that. I would also agree that the convenience factor in internal use really isn't that high.

However, given that third-party code consuming these APIs will likely also want to have access to their corresponding types, I'm wondering how we should best handle this here.

E.g., with the current state I can just import the various Shape types from this barrel file without having to worry about the internal structure of the shapes package (annotating shape types with e.g. typeof globalThis.anki.imageOcclusion.Shape doesn't work without some major TS acrobatics which are likely a bad practice of their own).

Perhaps just bundling type exports in barrel files like these could be an option? Or splitting out type exports in a different way like d.ts files?

FWIW, I don't feel particularly strong about this from a stylistic standpoint, just want to make sure that we go with as defensive a solution as possible, so that using anki types is not too brittle long-term.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're consuming the types then it's arguably public API, and I remove my objection.

I'd like to return to the question of the best way to expose these to add-on authors in the future - some previous discussion about it can be found at #1764.

@dae
Copy link
Member

dae commented Oct 17, 2023

Apart from that, I'm ok with the changes you have here, but a regression in the text tool will need to be fixed first. Depending on the size of the review window, the text aspect ratio is getting messed up, and at some sizes the text will disappear completely.
2023-10-17T10:14:45,761953419+10:00
2023-10-17T10:15:21,906738824+10:00

Re exporting:

  • mobile currently has its own reviewer implementation that exposes onUpdateHook. It imports reviewer_extras.
  • the motivation for registerPackage() was allowing Svelte components authored by add-on authors to be integrated into our pages, as if they bundle their own copy of Svelte, things break. I'd suggest avoiding registerPackage() for now - Svelte 5's runes may make it no longer necessary, or we may end up avoiding dynamic components and exposing add-ons is a generic JS way instead in the future.

@glutanimate
Copy link
Contributor Author

regression in the text tool will need to be fixed first. Depending on the size of the review window, the text aspect ratio is getting messed up, and at some sizes the text will disappear completely.

Huh, that's a weird one, good catch! But it looks like it's not specific to this PR – I can also reproduce with main (0e24532):

Screencast-2023-10-19_17.30.34.mp4

So I would propose we split it off into a separate issue.

@dae
Copy link
Member

dae commented Oct 19, 2023

Sorry, I did briefly test this on main before reporting it; not sure why I didn't spot the issue then.

@dae dae merged commit c828a2e into ankitects:main Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants