Fix edit plugin cancel flow restoring in-memory tags (#6104)
Description
Fixes #6104.
When using the fromfilename and edit plugins together during import, aborted edit sessions could silently discard the temporary tags injected by fromfilename (e.g., track number and title derived from the filename). This happened when using eDit or edit Candidates and then cancelling: the edit plugin reverted objects by re-reading from disk, which does not contain the fromfilename-generated metadata.
This PR changes the edit plugin so that cancel and “continue Editing” both roll back objects to the original in-memory snapshot captured before opening the editor, instead of reloading from the files. This preserves temporary tags provided by other plugins (like fromfilename) across aborted edit sessions, while still only writing to disk when the user chooses Apply.
importer_edit is also updated to rely on this rollback behavior when edits are cancelled, rather than re-reading from disk, so interactive imports resume with the same in-memory metadata they started with.
To Do
- [x] Documentation
- [x] Changelog (Entry added under the Unreleased “Bug fixes” section.)
- [x] Tests (Existing
editplugin tests exercised and extended to cover the updated cancel/rollback behavior.)
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 68.22%. Comparing base (5d1210a) to head (53a42bf).
:warning: Report is 7 commits behind head on master.
:white_check_mark: All tests successful. No failed tests found.
Additional details and impacted files
@@ Coverage Diff @@
## master #6200 +/- ##
==========================================
+ Coverage 68.20% 68.22% +0.02%
==========================================
Files 138 138
Lines 18775 18772 -3
Branches 3173 3170 -3
==========================================
+ Hits 12805 12808 +3
+ Misses 5296 5290 -6
Partials 674 674
| Files with missing lines | Coverage Δ | |
|---|---|---|
| beetsplug/edit.py | 85.62% <100.00%> (+3.27%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Thanks for the PR. Finally had some time to have a look here.
Could you simplify the comments a bit they feel quite verbose. Also could you please add a test for the
["e","c"]case?Other than that this looks like a quite elegant solution to me.
Hi @gabe-push I also had a look here yesterday and thought the same. Even though the comments are well written and useful they could be a little shorter, or, as an alternative idea for documenting what's happening, you could try to to describe everything in one comment block per section. Keeping code readable / together and putting one larger block above it to describe more briefly what the flow of that section is. Or maybe sometimes the whole flow of a function can be briefly described in the docstring (that strategy won't work well here I suppose, just mentioning)
This plugin had an extensive amount of comments already, I just realized now. We won't be mad if you even try to shorten some of the existing one's if it fits your with your new descriptions better!
Hope that helps!
And by the way: I love this feature / this fix! Very nice! Thanks!
Hello @semohr and @JOJ0 , thank you both for the helpful feedback! I agree and apologize for the very verbose comments - definitely don't want to negatively effect code readability and maintainability in any way. I've just updated the comments and added a test for the ["e","c"] case. Please let me know if there's anything missing! Thank you
Hi @gabe-push, thanks for the comments changes! Great! Please resolve the changelog conflict, I've set the PR to auto-merge once that's done. Many thanks again for the contribution! See you next time!
Since the PR was sitting for a bit I resolved the issues myself before we start to forget about this :upside_down_face: