ankitects / anki

Anki's shared backend and web components, and the Qt frontend
https://apps.ankiweb.net
Other
18.95k stars 2.14k forks source link

[Bug] Export/Import deck options not working when using default options group #3023

Open dae opened 8 months ago

dae commented 8 months ago

@dae The issue appears when exporting the Default deck (The skip_default=true case in gather_decks())

Originally reported on https://forums.ankiweb.net/t/bug-export-import-deck-options-not-working/41428/3

If a deck is set to the default preset (dcid=1), options will be taken from the default preset in the importing collection, as we don't want shared decks to alter the defaults the user has configured. But this is confusing when "include deck configs" is enabled. Perhaps we should return an error if that option is enabled and the default config is encountered? Something like "Deck presets can not be included when your deck is using the default deck preset"?

abdnh commented 8 months ago

Perhaps we should return an error if that option is enabled and the default config is encountered? Something like "Deck presets can not be included when your deck is using the default deck preset"?

Ideally we should clone the preset instead? Not sure how complex is that.

dae commented 8 months ago

I think that might be problematic in the scenario where the user is repeatedly sharing a deck with others - we don't want to be creating a new deck preset each time we export, as they'd build up in the importing user's collection. And the type of people who want to export deck presets tend to be teachers sharing with students, or students sharing with friends, so repeated imports aren't too uncommon I suspect. Maybe it's simplest just to tell the user that they can't use the default deck preset if they want to share their presets? Perhaps we should also make the default preset easier to see, such as appending " (Default)" to the name in the dropdown?

abdnh commented 8 months ago

Makes sense.

Perhaps we should also make the default preset easier to see, such as appending " (Default)" to the name in the dropdown?

Good idea.

RumovZ commented 7 months ago

How about replacing the id on export with a fixed value like the collection's creation timestamp? Optionally, the replacement could be reversed on import (if exporter equals importer).

dae commented 7 months ago

That'll break if the exporter imports their cards into a new collection, then exports them again, as the timestamp would be different. If we require the user to select a non-default preset, it's not going to break in such cases, and it lets us avoid extra complexity in the code, which is appealing. Hopefully changing the preset is not much to ask of users who are sharing?

RumovZ commented 7 months ago

That'll break if the exporter imports their cards into a new collection, then exports them again, as the timestamp would be different.

Not sure I follow. Let's say we have an collection X created at t_X. On export, the deck config id 1 would be replaced with t_X. It could then be imported into and exported from collection Y, with the id remaining t_X. Only when importing into X again, the id could be interpreted as 1 – or just treated like any other id ≠ 1 and imported as a new deck config.

But I totally understand you preferring the explicit and less complex way. Just wanted to put all options on the table.

dae commented 7 months ago

What I was thinking of is X's owner moves their cards into a new profile (thus new timestamp), and tries to share again. The importer would end up with a different deck config to the previous time, unless I'm missing something.

Options are always good :-) But I think simple is probably best in this case.

RumovZ commented 7 months ago

If they move the cards via apkg, the id would be t_X (not 1) in the new profile, and any exports from there would not depend on the collection's creation timestamp. If they move via colpkg, I think the creation timestamp is copied as well. So exports would depend on the creation timestamp, but it would be the same as in the old collection.

Sorry for flogging a dead horse here. 😄

dae commented 7 months ago

I was assuming that when "include scheduling" is enabled in the export to the other profile, the deck config would remain unchanged, and be 1 in the new profile as well. I assumed this t_X transform would only apply when scheduling=false and deck_config=true. Were you thinking differently? Or perhaps I've missed something else? :-)

RumovZ commented 7 months ago

I had only considered with_deck_configs. Why do you think the behaviour should depend on with_scheduling?

dae commented 7 months ago

The original use case for include_scheduling was for users moving their own cards between profiles, so I tend to think of that option as "export for myself", though this applies less since deck_configs was split into a separate option. When exporting for oneself, keeping things the same as in the original collection is probably best, so the user doesn't need to switch their decks based from the new tX preset back to the default preset if they want the exact setup as before. But that's not what the option is called, so this may be more me thinking about the original intent than what we're actually doing/presenting to the user ^^;

RumovZ commented 7 months ago

When exporting for oneself, keeping things the same as in the original collection is probably best

This case isn't supported if exporting the default config is prevented entirely by showing an error, either. But I get the point: With an ad-hoc id it seems like you can export a perfect copy of a deck, but it doesn't completeley work; the deck config would be the same, but it wouldn't be the default config anymore.

However, thinking more about the error solution it seems overly restrictive. If I want to export multiple decks and only really care about non-default configs, I would still need to assign different configs to all decks with the default one first.

What about migrating the export screen to web and adding a tooltip about the special default config handling?

dae commented 7 months ago

If I want to export multiple decks and only really care about non-default configs, I would still need to assign different configs to all decks with the default one first.

I'm trying to imagine a scenario where a deck sharer would want to pass some deck configs along, but export other (sub)decks with the default preset. Is it really unreasonable to expect the user to limit themselves to non-default presets when they use the 'include presets' option?

What about migrating the export screen to web and adding a tooltip about the special default config handling?

We'll want to migrate it at one point anyway, so no objections from me :-)

RumovZ commented 7 months ago

I'm trying to imagine a scenario where a deck sharer would want to pass some deck configs along, but export other (sub)decks with the default preset.

For one thing, I was thinking about the export-for-myself use case. Then I might just as well want to export all decks at once. At the moment, having to synchronize the default preset some other way is not ideal, but manageable. I think it would be more annoying if you couldn't export your deck tree as-is at all.

We'll want to migrate it at one point anyway, so no objections from me :-)

I'll go ahead with it then!