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

Migrate export dialog to web #3145

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

RumovZ
Copy link
Collaborator

@RumovZ RumovZ commented Apr 15, 2024

Closes #3023.

I deleted the now unusable hooks. Do we have to be more considerate?

@glutanimate
Copy link
Contributor

glutanimate commented Apr 15, 2024

This looks like a nice implementation @RumovZ and sensible step forward in Anki's UI migration.

However, looking at the impact of this on the add-on ecosystem, this will break almost all add-ons extending the export system, including:

  1. add-ons that observe and react to deck exports, which is quite important for addon-managed template modifications (e.g. AMBOSS add-on, cf. Add deck/collection export hooks #1971)
  2. add-ons that register a custom exporter (numerous custom export format add-ons, e.g. CrowdAnki)

Add-ons in 2) could create their own export UIs, but it would be a significant downgrade in Anki's extensibility if add-ons have to ship custom solutions for this, rather than hooking into an exporter API (not to mention a worse UX, with native and addon-provided export options spread apart).

Add-ons in 1) would have no recourse and would have to drop functionality.

I think it would be important for us to think through how we can maintain extensibility here in a sensible way.

@glutanimate
Copy link
Contributor

glutanimate commented Apr 15, 2024

To summarize, based on existing use cases, the three most important abilities for add-ons would be:

  1. Registering a custom exporter that would be selectable in the exporters list
  2. Being notified of an upcoming export and performing changes to the collection before it
  3. Being notified of a completed export and performing changes to the collection after it

I can look into 2.) and 3.) as these needs are essentially only present for the AMBOSS add-on at the moment.

1.) seems like it will require a bit more consideration, and I'd be curious what your thoughts are on how we can best do this (if you are on board with adding an exporters API, @dae). It seems like a JS API that would expose lib.ts > createExporter and allow extending the list returned by getExporters could work.

@RumovZ
Copy link
Collaborator Author

RumovZ commented Apr 16, 2024

Appreciate your input! As long as we don't have to keep a second legacy exporting module this all sounds good to me. I will have to look into how add-on integration in webviews works.

@dae
Copy link
Member

dae commented Apr 19, 2024

As long as we don't have to keep a second legacy exporting module

In the short term, we may want to do this, as the existing add-ons that make use of the hooks will take some time to get updated. Can't we just put it under the same legacy import/export flag we already have?

I will have to look into how add-on integration in webviews works.

I wouldn't say we have a well-established pattern for this yet. Some of our screens provide the ability for add-ons to extend them, but it hasn't been widely used yet, adds a bunch of noise to our implementation, and locks users into using Svelte.

The reviewer screen exports some globals that can be appended to as a makeshift hook system, but we only do it there so far, if I recall correctly.

@RumovZ
Copy link
Collaborator Author

RumovZ commented Apr 20, 2024

Can't we just put it under the same legacy import/export flag we already have?

And what about the legacy-legacy dialog? Do we drop that for good then?

The reviewer screen exports some globals that can be appended to as a makeshift hook system, but we only do it there so far, if I recall correctly.

Seems reasonable here. Being able to extend or modify the list of exporters should cover most use cases, and it's similar to how the current API works.

@dae
Copy link
Member

dae commented Apr 22, 2024

And what about the legacy-legacy dialog?

I'd honestly forgotten about it ^_^; I know that we can't remove the legacy import code yet due to the popularity of the special fields add-on, but I'm not aware of any big add-ons that use the old exporting path. Do you have any thoughts on this @glutanimate?

@RumovZ RumovZ force-pushed the migrate-export-dialog branch from 32c6983 to c6f76a9 Compare April 28, 2024 20:04
@RumovZ
Copy link
Collaborator Author

RumovZ commented Apr 28, 2024

I have spent some time looking into add-on integration now, and dispatching an event with a writable store is the best I could come up with: ankitects/anki-addons#21

Originally, I was imagining the page load to look something like this:

  1. Setup some hooking system.
  2. Have add-ons hook into it.
  3. Load the Svelte page, calling the hooks at the appropriate time.

However, steps 1. and 3. seem inseparable, at least in the new SvelteKit views, and any add-on code is loading in parallel. That means add-on code cannot rely on any already existing variables to hook into, and our Svelte code must be reactive to changes made by add-ons after the fact.

I had looked at https://github.com/hgiesel/closet and https://github.com/kleinerpirat/anki-tooltips, but they use the webview_will_set_content hook which is only available in the old-style webviews. https://github.com/ankitects/anki-addons/tree/main/demos/deckoptions_raw_html doesn't even work.

Do either of you see a better alternative, @dae, @glutanimate?

@RumovZ RumovZ marked this pull request as draft April 28, 2024 20:07
@RumovZ RumovZ force-pushed the migrate-export-dialog branch 2 times, most recently from 77e22a2 to f88f05a Compare April 29, 2024 15:45
@dae
Copy link
Member

dae commented May 1, 2024

I think your assessment is correct - reactivity+stores is probably our best option for now.

As an aside, Svelte 5 is not too far off though (RC announced a few days ago: https://www.youtube.com/watch?v=gkJ09joGBZ4&t=8610s), and the new Rune system looks like it'll be a fair bit easier to use than stores. I am tempted at this point to hold off on porting the editor to SvelteKit until we've upgraded to Svelte 5, as it's going to need a bunch of refactoring anyway, and the new functionality needs to be opted into a file at a time.

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.

[Bug] Export/Import deck options not working when using default options group
3 participants