edit: allow interactive editing during the importer #1846

diego-plan9 · 2016-01-29T17:10:18Z

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:

during an interactive import, the Items do not have a valid id field, as they are not on the database - and the edit plugin relied on it for reconciling back the user changes and other internal functionality. I have resorted to using the path field instead of id 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 the EditPlugin.mapping_field (and using the original items for showing changes during edit_objects).
I have tried to allow the user two main choices - edit based the tags of the files to be imported directly (currently eDit -> importer_edit) and edit based on a candidate (currently edit Candidate -> importer_edit_candidate), as they seemed to be two rather common use cases (cleanup files, and fix MusicBrainz problems).
- The main challenge is the first one, as I could not really find a way to implement it while keeping all the code within the plugin (hence the dirty modification to 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 the Items 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.
- For the second one, the implementation currently simply forces an task.apply_metadata() before invoking the edits so the Items 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 during importer_edit_candidate, let the importer proceed, and actually invoke the editor later (on import_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.

* 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.

diego-plan9 · 2016-01-29T17:41:59Z

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.

sampsyo · 2016-01-29T18:38:14Z

The main challenge is the first one, as I could not really find a way to implement it while keeping all the code within the plugin

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.

and actually invoke the editor later (on import_task_apply or a new pipeline stage),

Sadly, all user interaction has to happen in the choose_match stage. Otherwise, there will be multiple threads trying to talk to the user at once.

sampsyo · 2016-01-29T18:38:41Z

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.

diego-plan9 · 2016-02-01T18:21:53Z

Thanks for the thoughtful and useful comments, as usual!

The latest commits intend to add a new RETAG (temptative name) action, which basically signals the importer not to apply metadata from the candidates and reach manipulate_files() properly, writing the changes to disk if needed. Other than that, it isolates the dealing with the "reference" field into _set_reference_field, tidying up the code a bit. Could you please take a look and let me know if this goes more or less along the solution you suggested?

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 DBModel (o.keys(), o[key], o.key, o.read(), formatting, ui_show_changes(), etc). Nothing struck me as too difficult or unsolvable, though (the way I see it, it would basically consist of having a couple of different adapter/strategies for each kind of object to be edited, moving some of the existing functions there and maybe splitting some existing functions) - I mainly abandoned it temporarily as I could not invest enough time on it yet, and did want to try if the "new action" approach would suffice first.

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 edit choice is selected, something like:

[A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit? d

e[D]it items, edit Album, [1-5]...?

which might also make it more consistent by always having just "eDit" on the main prompt.

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.

sampsyo · 2016-02-02T06:23:29Z

beets/importer.py

@@ -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):


Or perhaps self.apply should be set in the RETAG case? (I'm not certain; it's just worth considering.)

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.

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.

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!

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.

Great; thanks for looking into it. This is a very reasonable conclusion.

sampsyo · 2016-02-02T06:32:18Z

Fantastic! This is definitely the right direction. Thanks for exploring all the possibilities—I'm convinced this is the way to go.

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

That is indeed tricky. Two unsatisfying hacks come to mind:

Take the album-level data from the first item.
Include a separate section for album fields in the temporary text file—basically your idea, but two sections in one file instead of two separate files.

Very tricky! There's no obvious answer here.

diego-plan9 · 2016-02-02T15:16:32Z

Fantastic! This is definitely the right direction. Thanks for exploring all the possibilities—I'm convinced this is the way to go.

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.

That is indeed tricky. Two unsatisfying hacks come to mind:

Take the album-level data from the first item.

Include a separate section for album fields in the temporary text file—basically your idea, but two sections in one file instead of two separate files.

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 --all or --field arguments when running the plugin from the importer (user can always resort to editing the config.yaml file, but might be a bit inconvenient, specially for the --all flag).

sampsyo · 2016-02-02T15:58:17Z

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

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 --all and --field for now; changing the config file seems like an OK workaround for the first iteration.

* 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.

diego-plan9 · 2016-02-03T17:02:55Z

The latest commits provide an implementation for:

Include a separate section for album fields in the temporary text file—basically your idea, but two sections in one file instead of two separate files.

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 FakeAlbum object that inherits from lib.Album in order not to alter the edit plugin flow too much - it is still not covered by the tests, and does some not so elegant things such as overriding the getter for the path field.

There are also some further modifications to the importer, in order to allow SingletonImporterTask to use the new RETAG action properly, and a slight tweak on ImportTask.align_album_level_fields which is a bit fragile: is there a better way to find out if, at that stage, we come from a RETAG-ASIS or a RETAG-APPLY?

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 unload_plugins() call, with all the instances from previous tests receiving the before_choose_candidate events (and the garbage collector showing that even after the unload_plugins, the instances do have some references lying around). I'm wondering if there are some extra steps that need to be performed to really unload the plugin?

EDIT: Update on the testing issue above: seems like manually cleaning the plugin listeners (EditPlugin.listeners = None, inspired by tests_filefilter.py) does the trick, but I'm still wondering if there is a better solution.

sampsyo · 2016-02-03T17:27:27Z

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 unload_plugins to do this by default?

diego-plan9 · 2016-02-03T18:12:09Z

Thanks, looking forward to the review!

About that testing issue: I don't know of a better way currently. Maybe we need to fix unload_plugins to do this by default?

It seems doable (iterate over beets.plugins._classes and set listeners to None for each item?) and harmless at a first glance, so maybe it is a good idea indeed! A bit off-topic, but has there ever been a demand for a BeetsPlugin.unregister_listener() function?

sampsyo · 2016-02-04T00:47:05Z

A bit off-topic, but has there ever been a demand for a BeetsPlugin.unregister_listener() function?

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.

sampsyo · 2016-02-04T04:32:25Z

beets/importer.py

+        # 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)


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?

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.

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.

sampsyo · 2016-02-04T04:58:29Z

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 beet edit -a command also?

Beyond that, this is looking like it's on the right track!

diego-plan9 · 2016-02-04T12:46:54Z

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).

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!

sampsyo · 2016-02-04T21:06:09Z

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.

diego-plan9 · 2016-02-05T13:46:50Z

beetsplug/edit.py

+        there are candidates) to the interactive importer prompt.
+        """
+        choices = [PromptChoice('d', 'eDit', self.importer_edit)]
+        if task.candidates:


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.

Ah, good point. I also don't see an immediate solution, so let's revisit the problem later on.

diego-plan9 · 2016-02-05T13:51:11Z

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 ids), and added some basic documentation and docstrings cleanup.

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 RETAG into apply() on the importer #1846 (comment); but hopefully we are getting closer!

sampsyo · 2016-02-05T17:24:48Z

Awesome! Thank you for the tremendous amount of work this took! ✨ This looks ready, IMO, so feel free to merge.

Woohoo!

diego-plan9 · 2016-02-05T18:06:43Z

Cool, thanks for the feedback and for the help and guidance in bringing this iteration of the edit plugin to life! I still have a slight feeling of the feature falling a bit short of the potential user's expectations due to the (reasonable, on the other hand) compromises we had to make ... that will most likely result in another round of discussion and crazy ideas from me in regards to the plugin in the not-so-distant future! 🔁

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?

Kraymer · 2016-02-05T23:29:23Z

Kudos @diego-plan9 , impressive quality stuff ! 💸

edit: allow interactive editing during the importer

sampsyo · 2016-02-07T22:39:00Z

Done! Let me know if I did the work justice, @diego-plan9.

diego-plan9 · 2016-02-08T10:46:56Z

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!

diego-plan9 added 2 commits January 28, 2016 13:01

Fix docs on plugin prompt choices conflict solving

b7a2a42

* 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).

diego-plan9 added 4 commits January 29, 2016 19:54

Fix flake8 error

30927a9

Merge branch 'master' of github.com:sampsyo/beets into interactiveedit

b472e1e

Add importer RETAG action

e23718d

* 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.

sampsyo reviewed Feb 2, 2016
View reviewed changes

diego-plan9 mentioned this pull request Feb 2, 2016

Convert SKIP_SLOW_TESTS to its own decorator #1809

Merged

diego-plan9 added 3 commits February 3, 2016 17:13

edit: fix Plugin not unloaded during tests

7b6c2c3

diego-plan9 force-pushed the interactiveedit branch from 121d596 to 7b6c2c3 Compare February 3, 2016 18:06

sampsyo reviewed Feb 4, 2016
View reviewed changes

diego-plan9 added 3 commits February 5, 2016 12:35

edit: RETAG importer fixes and docstrings

7f75a06

* 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.

edit: add documentation

763813f

* Add documentation to plugins/edit.rst about the execution of the plugin during the importer.

diego-plan9 reviewed Feb 5, 2016
View reviewed changes

sampsyo merged commit 763813f into beetbox:master Feb 7, 2016

sampsyo added a commit that referenced this pull request Feb 7, 2016

Merge pull request #1846 from diego-plan9/interactiveedit

6e3d72a

edit: allow interactive editing during the importer

sampsyo added a commit that referenced this pull request Feb 7, 2016

Changelog for #1846; fix #396

66fa962

diego-plan9 deleted the interactiveedit branch October 17, 2016 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

edit: allow interactive editing during the importer #1846

edit: allow interactive editing during the importer #1846

diego-plan9 commented Jan 29, 2016

diego-plan9 commented Jan 29, 2016

sampsyo commented Jan 29, 2016

sampsyo commented Jan 29, 2016

diego-plan9 commented Feb 1, 2016

sampsyo Feb 2, 2016

diego-plan9 Feb 2, 2016

sampsyo Feb 4, 2016

diego-plan9 Feb 4, 2016

diego-plan9 Feb 5, 2016

sampsyo Feb 5, 2016

sampsyo commented Feb 2, 2016

diego-plan9 commented Feb 2, 2016

sampsyo commented Feb 2, 2016

diego-plan9 commented Feb 3, 2016

sampsyo commented Feb 3, 2016

diego-plan9 commented Feb 3, 2016

sampsyo commented Feb 4, 2016

sampsyo Feb 4, 2016

diego-plan9 Feb 4, 2016

sampsyo Feb 4, 2016

sampsyo commented Feb 4, 2016

diego-plan9 commented Feb 4, 2016

sampsyo commented Feb 4, 2016

diego-plan9 Feb 5, 2016

sampsyo Feb 5, 2016

diego-plan9 commented Feb 5, 2016

sampsyo commented Feb 5, 2016

diego-plan9 commented Feb 5, 2016

Kraymer commented Feb 5, 2016

sampsyo commented Feb 7, 2016

diego-plan9 commented Feb 8, 2016

function	`RETAG` follows
chosen_ident()	ASIS
imported_items()	ASIS
align_album_level_fields()	APPLY
manipulate_files()	~APPLY

edit: allow interactive editing during the importer #1846

edit: allow interactive editing during the importer #1846

Conversation

diego-plan9 commented Jan 29, 2016

diego-plan9 commented Jan 29, 2016

sampsyo commented Jan 29, 2016

sampsyo commented Jan 29, 2016

diego-plan9 commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo commented Feb 2, 2016

diego-plan9 commented Feb 2, 2016

sampsyo commented Feb 2, 2016

diego-plan9 commented Feb 3, 2016

sampsyo commented Feb 3, 2016

diego-plan9 commented Feb 3, 2016

sampsyo commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo commented Feb 4, 2016

diego-plan9 commented Feb 4, 2016

sampsyo commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego-plan9 commented Feb 5, 2016

sampsyo commented Feb 5, 2016

diego-plan9 commented Feb 5, 2016

Kraymer commented Feb 5, 2016

sampsyo commented Feb 7, 2016

diego-plan9 commented Feb 8, 2016