-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
edit: allow interactive editing during the importer #1846
Conversation
* Fix docstring and `plugins.rst` regarding how the PromptChoices that use the same short letter are handled (they are discarded instead of raising an Exception).
* Initial draft for invoking the edit plugin during an importer session. * Add prompt choices for editing the original file tags ("eDit") and apply a candidate and then edit ("edit Candidates"). * Modify plugin (_get_fields, apply_data, edit_objects) so "path" can be used as a reference field instead of "id", as the Items are not still on the database when the plugin is invoked via the importer. * Modify ImportTask.manipulate_files() with a temporary flag for writing the item tags even if ASIS was selected.
I forgot to mention that another approach could be to make the edit plugin work on the candidates directly instead of the items (creating a "fake" candidate populated from the items if needed) - this would play nicer with the importer, be self-contained, and have some added niceness (such as displaying the changes in the same way as the regular importer would), although the main drawback is that some information (ie. file tags that are not mapped onto the candidates) might be lost in the process. |
Great point. Exactly as you say, the new action here is somewhere halfway between the ASIS action (use the tags as they currently are on the Items) and the APPLY action (which invokes the bits that move files, write tags, and save to the database). Maybe we need a new action type for exactly situation: don't apply any match, but do record new metadata. (You can imagine other plugins using a similar mode when they have some way of automatically applying metadata independent of match candidates.) I also like your "fake candidate" idea, which seems quite elegant to me—we could try it and see if the un-applied fields issue becomes a problem.
Sadly, all user interaction has to happen in the |
And I forgot to write: thank you for looking into this! ✨ This is a thorny issue, and an important feature, so you're brave to jump right in. |
* Add importer "RETAG" action, to represent the case where the files metadata has been modified by some mean other than applying metadata (via a plugin), and as a result needs to be written to disk if the "write" config flag is set.
* Make the edit plugin return action.RETAG when invoked during an interactive import session, making the importer handle the writing of the tags to the files (if needed) properly. * Move the logic relative to the "reference field" to _set_reference_field(), simplifying a bit the functions that depend on this field. * Hide the "edit Candidates" choice if no candidates are found.
Thanks for the thoughtful and useful comments, as usual! The latest commits intend to add a new I have also made an attempt at working on the "edit candidates instead of items" approach, but turned out to involve quite a refactoring of the plugin as it currently depends on a bunch of properties of One issue that would ideally need to be discussed regardless of the approach would be how to include the "edit album" (as oposed to edit items) choice without cluttering the interface - the simplest way I can think of is just doing two "editing rounds" always (one for the album fields, one for the item fields), with the rationale that users willing to edit something will already expect some manual input and wouldn't mind having to do a couple more of keypresses if they are not interested in editing one of the entities (album or items). Another option could be having a new prompt whenever the
which might also make it more consistent by always having just Acknowledged TODOs: cleanup comments, revise docstrings (plugin and importer functions that use the new action), add tests, take into account the editing of singletons, add plugin documentation. |
@@ -672,7 +673,7 @@ def manipulate_files(self, move=False, copy=False, write=False, | |||
# old paths. | |||
item.move(copy, link) | |||
|
|||
if write and self.apply: | |||
if write and (self.apply or self.choice_flag == action.RETAG): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps self.apply
should be set in the RETAG case? (I'm not certain; it's just worth considering.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit uncertain about what you mean: modifying the task.apply()
method so it returns True for both self.choice_flag in (action.APPLY, action.RETAG)
? I wouldn't oppose to that or have a strong opinion (didn't go through the implications yet), as I merely opted for the most explicit and simple way possible of adding the new action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's exactly what I meant. I also don't have a strong opinion; I was just noticing that we were checking flag in (APPLY, RETAG)
quite often, so it might make sense to use a shortcut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, and yes, indeed makes sense. However, it seems it would require a small modification to importer.apply_choice()
in order to avoid incorrectly applying the metadata changes if it is a RETAG
(something similar to):
@@ -1353,7 +1353,7 @@ def apply_choice(session, task):
return
# Change metadata.
- if task.apply:
+ if task.apply and task.choice_flag == action.APPLY:
task.apply_metadata()
plugins.send('import_task_apply', session=session, task=task)
and probably a similar tweak in regards to the align_album_level_fields
issue at #1846 (diff).
Both the existing approach and your suggested one sound like a reasonable trade-off to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking this issue again, I have decided to keep the explicit comparison with RETAG
at the moment, mainly because the behaviour of RETAG
does not always fall under the APPLY
choice:
function | RETAG follows |
---|---|
chosen_ident() | ASIS |
imported_items() | ASIS |
align_album_level_fields() | APPLY |
manipulate_files() | ~APPLY |
It seemed to me that merging both on task.apply()
would end up resulting in more confusion and a harder to follow flow! Again, it's rather subjective, so please let me know if you prefer the other approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great; thanks for looking into it. This is a very reasonable conclusion.
Fantastic! This is definitely the right direction. Thanks for exploring all the possibilities—I'm convinced this is the way to go.
That is indeed tricky. Two unsatisfying hacks come to mind:
Very tricky! There's no obvious answer here. |
Great! I'd still be up for giving another try at the "edit candidates" approach if needed, if in the end we are not happy with the results.
Not ideal indeed, but I think the second option is a reasonable compromise and does not look too hard to pull off! I'll work in that direction, at least until a more creative solution appears. Incidentally, I'm wondering if something should (and can) be done for allowing the plugin to receive command line arguments when not run as a SubCommand: in particular, I see the potential for using the |
It's probably best to just make those part of the Python interface to the plugin code—I don't see a clear way to re-use the argument-parsing logic without making things too messy. For this case, I suggest we don't worry about |
* Add support for the RETAG action to SingletonImportTask. * Modify ImportTask.align_album_level_fields() so the source of the information is a bit more intelligent in the RETAG case instead of always assuming the items come from applied metadata.
* Add EditDuringImporterTest test case, covering the running of the plugin during an import session. Includes editing the "album" field and applying/discarding for both editing from items and editing from a candidate; and editing and applying for singletons for both editing from items and editing from a candidate.
* Add support for editing both the item fields and the album fields in a single YAML file, by appending an Album-like object as the front of the objects to be edited. * The FakeAlbum class provides that object, mimicking the original Album behaviour and including an _apply_changes() method that propagates the changes read from the yaml onto the Items. * Modify edit_objects() so the flattening of the objects takes into account the type of object, using different fields for Albums and for Items. * Renamed apply() to apply_() to prevent an IDE warning about reusing a reserved built-in symbol.
The latest commits provide an implementation for:
It basically appends a "fake" Album object when running during the importer to the list of objects to be considered on the yaml, propagating the changes to the items if the editing was successful. The implementation relies on a There are also some further modifications to the importer, in order to allow Finally, added some tests for executing the plugin during an interactive importer run, in order to provide some confidence for further modifications (that, as mentioned, do not include probably-needed tests for the "editing album and item fields at the same time" feature), and also cleaned up and reorganized the previous test case a bit. I'm running into a slight problem with the tests: it seems that the plugin is not really unloaded between executions despite the EDIT: Update on the testing issue above: seems like manually cleaning the plugin listeners ( |
Cool! I'll do a full review soon. About that testing issue: I don't know of a better way currently. Maybe we need to fix |
121d596
to
7b6c2c3
Compare
Thanks, looking forward to the review!
It seems doable (iterate over |
Hmm, not except for during testing. I would hope that plugins wouldn't need to dynamically add and remove listeners all the time for correctness—that seems like a bit of a complexity nightmare. |
# Determine where to gather the info from for the RETAG action. | ||
retag_asis = (self.choice_flag == action.RETAG and | ||
not self.items[0].artist and | ||
not self.items[0].mb_artistid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow why these fields are the right ones to check for this... is there a particular reason why you chose artist
and mb_artistid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry: artist
should have been albumartist
, in a lazy attempt to just use the same fields that the elif
is using.
The ultimate reason is trying to find out if we get here by doing a RETAG-based-on-asis
or
RETAG-based-on-candidate
, as there is no self.match
in either case. I'm not sure if this can be achieved without further "polluting" the class (by adding a flag variable of sorts) or similar (the match
doesn't get returned from commands.choose_candidate()
either, just action.RETAG
). Even if it is not bulletproof, it seemed like a good compromise to check the mb_artistid
existence in particular as a decent indicator that we are coming from RETAG-based-on-candidate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for the explanation---I understand now.
Here's a crazy proposal: what if we never apply the "as-is fixup" logic for the RETAG case? The philosophy would be: if you're willing to edit the data manually, then you can take matters into your own hands.
Seeing the complexity it's caused, I wonder if we shouldn't reconsider the feature that lets the user edit album-level fields as well as item-level ones. I know that it's useful (and I originally suggested it!), but this may be a case where the benefit is outweighed by the extra complexity in the code (FakeAlbum and all that). Maybe we should revisit this as part of a later change—maybe a more general one, since the same kind of thing would be useful during an explicit Beyond that, this is looking like it's on the right track! |
Yes, I definitely agree that things have gotten quite messy as a consequence of that change, and even if it is a highly desirable feature it is probably best to leave it out for the next iteration, sadly. I'll work on reverting the changes related to editing both album- and item-level fields, plus using temporary ids, which should take care of most of the comments on the latest review! |
Awesome! I'm excited---and it will be interesting to tackle the album-level problem in the next iteration. |
* Revert the changes related to allowing the album- and item-level fields to be edited at the same time, as the increase in complexity was deemed excesive during review. * Modify the interactive execution so temporary Item.id's are used, removing the extra functionality needed for dealing with both id and path as reference fields. * Docstrings and comments cleanup.
* Remove the RETAG-specific logic on align_album_level_fields, assuming that the user will always make sure to have proper data on the first item. * Revise some docstrings and comments in order to clarify the use of RETAG.
* Add documentation to plugins/edit.rst about the execution of the plugin during the importer.
there are candidates) to the interactive importer prompt. | ||
""" | ||
choices = [PromptChoice('d', 'eDit', self.importer_edit)] | ||
if task.candidates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one actually revealed a problem if the user uses Enter search
or enter Id
to find a candidate not found during the initial lookup: as things stand, I believe this listener has no way of retrieving that candidate (as the new candidate
it is handled within TerminalImportSession.choose_match()
, and only saved when exiting the prompt by applying). I have added a note to the docs, but I'm wondering if something else should be done about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. I also don't see an immediate solution, so let's revisit the problem later on.
Rolled back the two features that were the main cause of the increase in complexity (discarding completely the album-fields feature and adding a note to the documentation; and using temporary There are still some issues open which I hope did not get swallowed by github collapsing old diffs - the saving of the old data for comparison #1846 (comment), and the merging of |
Awesome! Thank you for the tremendous amount of work this took! ✨ This looks ready, IMO, so feel free to merge. Woohoo! |
Cool, thanks for the feedback and for the help and guidance in bringing this iteration of the Would it be ok to leave writing the changelog to you, as you'll probably be able to find a better way of summarizing the changes and conveying the half-baked-ish state of the feature? |
Kudos @diego-plan9 , impressive quality stuff ! 💸 |
edit: allow interactive editing during the importer
Done! Let me know if I did the work justice, @diego-plan9. |
It does, thanks a lot for taking care of the changelog with such a neat choice of words! And thanks @Kraymer for the kind words, although I have to return the kudos for your fantastic amount of contributions to beets throughout the project's history! |
As a follow up to #396, an attempt to allow the user to launch the edit plugin during an interactive import session.
At the moment the code is still not complete and rather unpolished and full of TODOs and other comments - but hopefully it will serve as a way to discussing some issues that are not as straight-forward as I'd hoped:
Items
do not have a validid
field, as they are not on the database - and theedit
plugin relied on it for reconciling back the user changes and other internal functionality. I have resorted to using thepath
field instead ofid
when the plugin is invoked on interactive mode, as it seems to satisfy the requirements for a "reference/mapping" field (unique for each item, non-editable by the user). This is done mainly via the introduction of theEditPlugin.mapping_field
(and using the original items for showing changes duringedit_objects
).eDit
->importer_edit
) and edit based on a candidate (currentlyedit Candidate
->importer_edit_candidate
), as they seemed to be two rather common use cases (cleanup files, and fix MusicBrainz problems).importer.py
): editing the Item tags is similar to ASIS ... but not quite. If marked as an ASIS, I could not find an importer code path where theItems
are really written(), making the changes to be stored correctly on the database but not on the files. If marked as an APPLY, the candidate takes preference and hence the user edits are discarded in favor of the first candidate.task.apply_metadata()
before invoking the edits so theItems
are modified temporarily, and then proceed as in the first case. It is probably more correct (and doable!) to just set a flag on the plugin duringimporter_edit_candidate
, let the importer proceed, and actually invoke the editor later (onimport_task_apply
or a new pipeline stage), which would also get rid of the current extra prompt for selecting a candidate.I'm wondering if you could point me in the right direction for solving the "edit item tags" properly? I have played around with delaying the invoking of the editor until another event for that case as well, but unfortunately could not find an event that is launched at the right moment reliably (using ASIS, and taking into account the different write/copy/link options). Also, the pipeline stage (which could be a good candidate) does not seem to be reachable for an ASIS choice.