Migrate config to standard anki scheme (fixing 91 and 73) #96
Conversation
Migrate config to the standard anki scheme - load with the anki function (support config-meta scheme) - monitor changes from inside the anki and update internal data (do not overwrite these changes with defaults)
There was a problem hiding this comment.
Thanks for the PR! A few comments from me:
-
This changes the semantics of
self.config. Previously it was adefaultdictin which aKeyErrorwould not be thrown on an attempt to access a key which did not exist but now it is only adict. Is this intentional and have you confirmed this change is safe? -
The proposed solution here will mean that any existing
config_saved.jsonwill be totally discarded. In order to try and be backwards compatible, we should first check for the existence of this file and if it exists thenaddonManager.writeConfigit, at least for the next few releases then the check can be ripped out. -
This introduces a race condition where if config is updated from within the addon and then config is updated from within the Anki UI, the config from within the addon will be wiped out. Additionally, any config changes made from within the addon will not be visible in the Anki UI until
saveis called. -
It looks like the config updated callback passes the new config as an arg, we can use this for some cleaner code to avoid having intercept things with a lambda to ignore the args then calling
getConfigagain. Just usingself.updateas the callback could take care of this and also my point above about the race condition but not the point about changes not appearing in the UI. To fix everything and keep old behaviour would get kind of complicated but IMO we should go for the simplest thing here and rip out all the caching and thatsavemethod and fully rely on Anki for the config, no maintaining of aself.configcopy. -
Now that we have stopped using them, could you remove the extra imports, namely:
from collections import defaultdict
from json import dump, load
from os.path import dirname, exists, join, realpath
Solution for installing in separate plugin directory
There was a problem hiding this comment.
Hey, I appreciate the effort here. I'm thinking this is not the right way forwards.
I think almost all the pitfalls we are looking at can be avoided by removing the self.config variable and instead going through this procedure:
- On init, check if old
config_saved.jsonexists, if it doesn't then we are done (firstRundoesn't matter here). - If it does exist then load it,
.updatethemw.addonManager.getConfig(__name__)config with it then immediatelymw.addonManager.writeConfig(__name__, updated_config)it. - If all this worked then move
config_saved.jsontoconfig_saved.json.bak.
| def _get_old_addon_config_file() -> Path: | ||
| apath = Path(mw.pm.addonFolder()) | ||
| # This is mainly for tests, since 'current-module-dir is mocked, but we cannot mock/patch this function, | ||
| # it is called at the import time, by main.py, ugh | ||
| if exists(apath): | ||
| ret = apath.resolve(strict=True) / '1752008591/config_saved.json' | ||
| else: | ||
| ret = None | ||
|
|
||
| return ret |
There was a problem hiding this comment.
- it looks like
mw.addonManager.addonsFolder(__name__)does what you want here? - if this can return
Nonethen the type hint should indicate that - we shouldn't be mixing old
os.pathstuff with the newpathlibstuff - it feels wrong that this could return
Noneif the path doesn't exist in one case but return a path that doesn't exist in another case, why do both? I think you can get rid of this function entirely.
| self._load_config() | ||
| mw.addonManager.setConfigUpdatedAction(__name__, lambda *_: self._config_updated_handler()) | ||
|
|
||
| if self.config['firstRun']: |
There was a problem hiding this comment.
But now this forces everyone who updates to have firstRun trigger to true, but it isn't necessarily their first runs.
| spath = _get_old_addon_config_file() | ||
|
|
||
| if spath and spath.is_file(): | ||
| # Old MacDonald had a farm. E-I-E-I-O. |
| showInfo( 'Configuration from the old "Chinese Support 3" was imported.\n\n' | ||
| 'This is a one-time post install action, if you update the configuration in the original addon, ' | ||
| 'it will not be synchronized.\n\n' | ||
| 'It is recommended to disable the old addon.') |
There was a problem hiding this comment.
I don't understand this, since there is an update there is no addon which needs to be disabled. I think this message should be removed entirely. We can perform this completely unbeknownst to the user.
| for k in ['enabledModels', 'speech', 'target', 'max_examples', 'fields']: | ||
| self.config[k] = config_saved[k] |
There was a problem hiding this comment.
No need to manually copy select fields, just copy all of it.
| @@ -36,6 +36,7 @@ | |||
| if config['firstRun']: | |||
| dictionary.create_indices() | |||
| config['firstRun'] = False | |||
| config.save() # need for correct meta.json handling | |||
There was a problem hiding this comment.
We shouldn't need this. Our view of the config shouldn't get out of sync with the Anki view.
Hopefully fix #91 and fix #73
Migrate config to the standard anki scheme