zjosua / anki-mc

Multiple choice questions for Anki.
https://ankiweb.net/shared/info/1566095810
GNU Affero General Public License v3.0
49 stars 16 forks source link

Add automatic template editing on question field addition or removal #109

Closed 3ter closed 1 year ago

3ter commented 1 year ago

As asked the last time in #108 sometimes people forget to edit their template when they want to add more than 5 questions to their multiple choice pool. This should happen automatically.

I was missing a hook (adding a field to a note type) so I added it. My PR in the main anki repository already got merged ~so this is going to stay a draft until the next anki release is out~. The changes are backwards compatible though.

@zjosua You can have a look before that though as the use of the addition hook is analogous to the deletion hook (which already has been present). And there are unit tests now 😉 .

3ter commented 1 year ago

Now the PR is feature complete and backwards compatible so it is ready to be merged after a short review. I haven't yet bumped the version I just now recognized.

3ter commented 1 year ago

To run the tests you only have to install (pip install pytest) and run pytest (there are handy extensions as well of course).

Thanks for your comments!

3ter commented 1 year ago

There's still one thing that's bugging me though... When there's a new version of the addon and automatically updates the note type it replaces the template code but doesn't (yet) look at the fields potentially modified by the user.

What do you think?

EDIT: I think we could (and probably should, because the user shouldn't be forced to use the question id Q_\d+ correctly) make the number of questions a setting in the JSON config. Then when the config is saved we'll both add the proper changes to the template and the fields.

3ter commented 1 year ago

Why haven't you said that you did something very helpful last year 😄 : https://github.com/ankitects/anki/pull/2205

👍

zjosua commented 1 year ago

TL;DR:

make the number of questions a setting in the JSON config

I thought the point of this whole PR was that users don't have to manually tell the add-on that they changed the number of question fields. Now if we change it from having to edit the template to having to edit the add-on config, that's not much of an improvement in my opinion.

When there's a new version of the addon and automatically updates the note type it replaces the template code but doesn't (yet) look at the fields potentially modified by the user. What do you think?

Preventing the loss of custom changes of the template should be possible by placing the files (front, back and css) in the user_files folder. We should still pack the default files. It'd probably be best to leave the defaults in the card folder and only create copies in user_files when custom changes are made.

This also has the potential to create problems with backwards compatibility between the custom templates that don't get updated for ages and the python side of the add-on. Therefore it might be sensible to add the version to the template files so we can notify users with old templates files when we introduce changes that break backwards compatibility.

However for the scope of this PR, I think the best solution is to read the number of question fields from the model after updating the template and then adjust the template to match the number of question fields. And then do the copy-template-files-to-the-user_files-folder-to-preserve-other-customizations stuff in another PR if you want customizations other than the number of question fields to be saved.

the user shouldn't be forced to use the question id Q_\d+ correctly

Do you mean that things like Q_one should also be possible? That would be fine with me. Q_\w+ would be a good choice imo. I think we should force the Q_ prefix though, because with no limitations at all it will be difficult to identify the question fields reliably.

3ter commented 1 year ago

I agree with everything you've said. Thanks for elaborating on what would be nice for the user to have and keeping a PR as small as possible ("Do One Thing").

EDIT: I wanted to add that I didn't mean to allow anything else than the question id we already use. This only made sense when using (only) the config and not touching anything else, which doesn't reflect reality 😄 .