mailpile
"Factory Reset" for user tags
This PR implements "Factory Reset" for user tags, it works basically as described in #2031 .
I added a new "default" attribute to tags that are initially configured on setup, so that we know what was the original tag.
Hello! Thank you so much for working on this.
Sorry it's taken me a few days to respond, but I have a couple of concerns about the implementation and I wanted to take the time to give good feedback. My concerns:
Most pressingly, your ResetTags.setup_command is duplicating all sorts of settings from the dict() in SetupMagic. I feel quite strongly that the same data should never exist in two places, because that becomes a trap for future developers - if things get changed in one place but not the other later on, then we end up with nasty, sneaky bugs. Both commands should use the same data source for their work; since SetupMagic is an ancestor of your ResetTags, you could just access it directly as self.TAGS, right?
Secondly: are you sure it is necessary to add the default attribute to the tags? This adds a field to every single tag in the user's configuration; if this can be done in a less bloated way that would be preferable. It seems to me like a simple list in ResetTags declaring which tags are affected by the factory reset would suffice. Do you agree?
Thirdly: Please don't add UI elements using Javascript. For consistency with the rest of the app they should be declared in the HTML template along with the other buttons. The template that needs changing is ..../shared-data/default-theme/html/partials/sidebar.html, grep for Done.
...
Sorry I had so many complaints, but keeping the code maintainable is quite important.
It does look like what you've written will work (given the current set of tags); I think the only feature request I would have would be to present the user with a modal dialog instead of comfirm() that lets them choose which tags to reset. But that's a stretch goal (maybe a 2nd iteration), I would definitely not reject the PR for lack of that.
Note to readers: This isn't getting merged as is, and the conversation has stalled. If anyone else wants to pick this up and implement my requests that would be super awesome. Otherwise I might do it myself someday, but I have bigger fish to fry so it might take a while.