ankitects / anki

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

Changing IO field order causes note type to malfunction in various ways #2621

Closed glutanimate closed 1 year ago

glutanimate commented 1 year ago

Steps to reproduce

  1. Head to ToolsManage note types, select the Image Occlusion note type, and click Fields
  2. Change one of the following:
    • Move the Occlusion field to any position other than the first
    • Move the Image field to any position other than the second
  3. Confirm your changes and proceed to add or edit an IO note

Expected behavior

Adding, editing, and reviewing IO notes works as usual.

Actual behavior

Adding, editing, or reviewing IO notes no longer works, in some scenarios silently, while in others with an error message that's not easily interpreted as a user.


Screencasts

Scenario 1: Moving the Header field to position 1, and thus bumping Occlusion and Image down by one. Result: IO notes can no longer be edited or added.

https://github.com/ankitects/anki/assets/5459332/303c1801-afb2-4e96-992c-30ef17549d0b

Scenario 2: Moving the Image field down by 1. Result: IO notes can no longer be reviewed properly.

https://github.com/ankitects/anki/assets/5459332/332cb96d-413e-478a-9dd1-52fc4bb697eb


Thoughts on the impact

glutanimate commented 1 year ago

Thoughts on a solution

Given that Anki field names are fully internationalized, mapping and identifying fields by their display name does not seem like a viable solution. Instead, I would propose the following:

In this scenario users would still be able to delete key fields like Occlusion and Image from the Fields dialog, and recreating them with the right identifiers specified would not be user-accessible. So I would additionally propose that we prevent users from deleting these fields, e.g. by extending the note type config with a list of required field identifiers, or by encoding the removability of a field in the note field config.

You could argue that users can also easily break other default note types by deleting key fields, but I would say that the situation is quite different here: For regular text note types, users have full control over their editing and rendering end-to-end. They can freely play around with field order, naming, and their referencing in their templates with no impact on internal Anki logic. With IO this is not the case as the editing and rendering behavior of IO notes lies outside the control of users.

WDYT @dae @krmanik ?

glutanimate commented 1 year ago

Another case to add to the scenarios above: Deleting either the Header or Back Extra field (or, more accurately, modifying the note type so that it has <4 fields) will also cause IO to stop working.

To handle this one, perhaps we could perform the check in image_occlusion::notetype -> io_notetype_if_valid before saving the notetype and thus prevent users from creating an invalid notetype? Right now these changes are accepted without an error in the Fields dialog. But I would also argue that, in order to be consistent with other default note types, we shouldn't depend on fields being present that aren't essential to internal editing & rendering logic.

dae commented 1 year ago

Header and Back Extra are currently essential, because in the mobile use-case those fields are shown in the I/O editor, and the Rust code expects to be able to write them when a note is added or updated.

On adding a unique identifier instead of using field index: you make some good points. It would need to fall back on the indexes if the identifiers were not found though, as these notetypes are already in use by AnkiMobile users.

A separate option for whether a field is removable or not feels like it might be overkill - what if we just made any field with a unique identifier protected?

glutanimate commented 1 year ago

Header and Back Extra are currently essential, because in the mobile use-case those fields are shown in the I/O editor, and the Rust code expects to be able to write them when a note is added or updated.

I see. I don't think there's any harm in also protecting them in that case. Albeit I'd still argue that, if we want to be fully consistent, it would be nice if we could treat all fields other than Image and Occlusion as regular fields at some point in the future.

(I had forgotten that the custom IO fields tab implementation on mobile only shows Header and Back Extra. I do think that this is something more advanced users will run into and be confused about, but IMHO not a major issue for now).

It would need to fall back on the indexes if the identifiers were not found though, as these notetypes are already in use by AnkiMobile users.

Good point :+1:

Though would it perhaps make sense to also have some kind of migration routine for those existing notetypes? I would assume that these could already be present in a significant number of user collections, even if those users haven't used IO, yet (as I assume it's similar to desktop where the note type will get created either at launch or when using the editor).

what if we just made any field with a unique identifier protected?

I think that would work nicely as well.

krmanik commented 1 year ago

I think preferences can also be used, it will be up to user to select the field for adding occlusion data and image data. Using the prefs we can directly access the fields and hide the field in add, edit, browse mode, and show it in configuration.

dae commented 1 year ago

I'm afraid I'm not sure that's the best idea - it adds extra complexity to the UI, and more ways things can go wrong. The user will already be able to rename the fields if they want - isn't that enough?

Though would it perhaps make sense to also have some kind of migration routine for those existing notetypes?

Sure, this could be handled in Check DB fairly easily.