mbsubmit: Add picard PromptChoice

[doronbehar]

Make it possible to open picard from the import menu when there are weak recommendations. Add another menu option to print the tracks into a beets-unmatched file for later parsing.

Description

A better alternative to #4749 - closes #4749 . Also provides an intermediate solution to #1866 .

cc @ybnd .

To Do

• Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• Tests - test_mbsubmit.py was modified to accommodate the change.

[JOJ0]

Hi, interesting idea. Why not make the tag-editor configurable? Me personally I prefer puddle-tag or mp3tag (on macOS) over Picard when tags are so bad that beets alone is not sufficient.

[doronbehar]

Hi, interesting idea. Why not make the tag-editor configurable? Me personally I prefer puddle-tag or mp3tag (on macOS) over Picard when tags are so bad that beets alone is not sufficient.

The point of opening Picard is not for tagging the files of the release, but to read the already tagged files and open a 'add release' page in musicbrainz' website, that will include most of the information needed for a proper release entry in the database. Then, once it's in the database, you continue the import process, using the release id you got from musicbrainz, and beet tags the files for you.

[JOJ0]

Oh I see! That is indeed a cool feature!!

[jackwilsdon]

Is it possible for us to link directly to the release creation page on MusicBrainz (with the fields pre-filled) instead of doing it via Picard?

[doronbehar]

Is it possible for us to link directly to the release creation page on MusicBrainz (with the fields pre-filled) instead of doing it via Picard?

That is the idea suggested in #1866 . It's a great idea, but it's much harder to implement. See my comment there linking picard's relevant source code.

[arsaboo]

Short of being able to directly add information to MB, using Picard seems like a nice middle ground. Looking forward to this 👍

[arsaboo]

@doronbehar can you resolve the conflicts so that we can move forward with this?
@jackwilsdon @JOJ0 Assuming we are fine with using Picard as the middle ground, is there anything else that needs to be done?

[doronbehar]

Fixed merge conflicts via a rebase.

[doronbehar]

Hmm not sure why CI says my order of imports is incorrect...

[arsaboo]

Hmm not sure why CI says my order of imports is incorrect...

Use isort to sort imports.

[doronbehar]

Hmm not sure why CI says my order of imports is incorrect...

Use isort to sort imports.

Done!

[doronbehar]

Fixed black formatting as well.

[arsaboo]

There are still a couple minor lint errors.

[doronbehar]

Thanks for help. Hopefully it'll be green now.

[doronbehar]

Added docs and a changelog line.

[arsaboo]

probably revert this change

[doronbehar]

I don't understand how that test passed... The error now, from CI is:

AssertionError: 'Print tracks,02. Tag Title 2 - Tag Artist (0:01)' not found in '\n/tmp/tmpcyx2ztde/testsrcdir/the_album (2 items)\n\nFinding tags for album "Tag Artist - Tag Album".\n  Candidates:\n  1. (51.5%) Applied Artist M - Applied Album M\n             ≠ album, artist, tracks\n             match_source, None, None, None, None, None, None\n  2. (49.8%) Applied Artist MM - Applied Album MM\n             ≠ album, artist, tracks\n             match_source, None, None, None, None, None, None\n➜ # selection (default 1), Skip, Use as-is, as Tracks, Group albums,\nEnter search, enter Id, aBort, Print tracks,\nprint tracks into a "beets-unmatched" File,\nOpen Picard with the relevent files? \n/tmp/tmpcyx2ztde/testsrcdir/the_album/track_1.mp3\n\n  Match (61.5%):\n  Applied Artist - Applied Title 1\n  ≠ title, artist\n  None, Index 0, Track None, \n  ≠ Artist: Tag Artist -> Applied Artist\n  ≠ Title: Tag Title 1 -> Applied Title 1\n➜ Apply, More candidates, Skip, Use as-is, Enter search, enter Id, aBort,\nPrint tracks, print tracks into a "beets-unmatched" File,\nOpen Picard with the relevent files? \n/tmp/tmpcyx2ztde/testsrcdir/the_album/track_2.mp3\n\n  Match (61.5%):\n  Applied Artist - Applied Title 2\n  ≠ title, artist\n  None, Index 0, Track None, \n  ≠ Artist: Tag Artist -> Applied Artist\n  ≠ Title: Tag Title 2 -> Applied Title 2\n➜ Apply, More candidates, Skip, Use as-is, Enter search, enter Id, aBort,\nPrint tracks, print tracks into a "beets-unmatched" File,\nOpen Picard with the relevent files? 02. Tag Title 2 - Tag Artist (0:01)\n\n  Match (61.5%):\n  Applied Artist - Applied Title 2\n  ≠ title, artist\n  None, Index 0, Track None, \n  ≠ Artist: Tag Artist -> Applied Artist\n  ≠ Title: Tag Title 2 -> Applied Title 2\n➜ Apply, More candidates, Skip, Use as-is, Enter search, enter Id, aBort,\nPrint tracks, print tracks into a "beets-unmatched" File,\nOpen Picard with the relevent files? '

I changed it now to something which might work.

[JOJ0]

Agree with everything, work away :-)

[doronbehar]

Thanks for the cooperation, and again sorry for the slow replies :). I removed the beets-unmatched file behavior, due to the reasons mentioned above. The tests pass on my machine, and formatting should be OK as well according to my tests. Hopefully CI will be green now, and this should be pretty much ready for final approvals.

[JOJ0]

Thanks for the updates. No worries about response times. We are getting there! :-) One question within.

[JOJ0]

Sorry I still don't quite understand this part. Can you explain what the indented functionality is?

[doronbehar]

Sorry I still don't quite understand this part. Can you explain what the indented functionality is?

It's OK :) I updated this section with 2 more comments.

[JOJ0]

Hmm still I don't understand why we need that much logic to try running a path that's configured by the user anyway.

Simplification suggestion:

remove the which(),

just check if what the user configured looks like a path (you did that using confuse's path function, perfect)

just try running and fail gracefully. that's the way other beets plugins run external programs too.

I think with this conversation we decided on that approach: #4807 (comment)

[JOJ0]

Maybe a simplified version of like the play plugin does it. A try/except block using subprocess.call and then fail gracefully if that doesn't work out?

beets/beetsplug/play.py

Lines 50 to 58 in 951b0f5

	try:
	if keep_open:
	command = shlex.split(command_str)
	command = command + open_args
	subprocess.call(command)
	else:
	util.interactive_open(open_args, command_str)
	except OSError as exc:
	raise ui.UserError(f"Could not play the query: {exc}")

or maybe use util.interactive_open()

not sure which way is more appropriate here. you choose.

[doronbehar]

Maybe a simplified version of like the play plugin does it.

Thanks for the link :) I used it as a reference, though haven't tested in an environment with no picard available or so..

using subprocess.call

Is there an insistence on subprocess.call and not subprocess.Popen? Python describes call as an older-level API.

[JOJ0]

yeah, goot catch. then use .Popen. sounds good.

I've always used subprocess.run in the past. not sure if that makes sense here.

[JOJ0]

Sorry I accidantly edited your post above, I hope I restored it correctly!

[doronbehar]

It'd be nice if someone else will give this PR a test drive on a machine without picard installed...

[JOJ0]

It'd be nice if someone else will give this PR a test drive on a machine without picard installed...

could be easily done by yourself, by just providing a non-existent path in the config:

confuse's .path() .as_filename() wont fail, it just checks if "it looks like" a proper path/filename.

when you try running with subprocess.Popen, your error should be catched and handled by the except clause.

So just to clarify and prevent further misunderstandings, what I think would be the most straight forward way and will work on all platforms and can be tested easily by yourself:

• check if the user has a path configured by using the config handler's (confuse) own sanity check (you do that already)
• now that we know we have a proper path configured we can instantly run the command within a try block.
• if it doesn'T work out we print a nicely formatted error message (in the except block)

please doublecheck my assumption about .path() .as_filename() in the confuse docs: https://confuse.readthedocs.io/en/latest/examples.html#path

edit: But I can certainly test it out on my machine were currently is no Picard installed, once you are finished with implementing.

[doronbehar]

It'd be nice if someone else will give this PR a test drive on a machine without picard installed...

could be easily done by yourself, by just providing a non-existent path in the config:

You are right :) I wasn't thinking about it. Here's what I added to my config for testing:

mbsubmit:
    picard-path: picard-missing

confuse's .path() .as_filename() wont fail, it just checks if "it looks like" a proper path/filename.

when you try running with subprocess.Popen, your error should be catched and handled by the except clause.

So just to clarify and prevent further misunderstandings, what I think would be the most straight forward way and will work on all platforms and can be tested easily by yourself:

• check if the user has a path configured by using the config handler's (confuse) own sanity check (you do that already)
• now that we know we have a proper path configured we can instantly run the command within a try block.
• if it doesn'T work out we print a nicely formatted error message (in the except block)

please doublecheck my assumption about .path() .as_filename() in the confuse docs: confuse.readthedocs.io/en/latest/examples.html#path

And I got the following experience:

➜ Apply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, Print tracks, Open files with Picard,
eDit, edit Candidates? O
error: Could not open picard due to error:
[Errno 2] No such file or directory: '/home/doron/.config/beets/picard-missing'

And the beet exits completely. I'm not satisfied with this behavior. Firstly, I don't like how confuse' behavior is to always get a path relative to the config. Even if I comment out the mbsubmit: config section, using .as_path() or .as_filename() makes it look for picard in the directory I'm at, and not in $PATH. So again, what is the reason not to use .as_str()?

Here is the behavior I get with an implementation similar to the previous - using shutil.which(), and .as_str():

➜ Apply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates, Print tracks,
Open files with Picard? O
mbsubmit: couldn't find picard executable 'picard-missing'

Also note, that the import process doesn't stop completely when failing to run picard-missing, because I don't raise ui.UserError, as opposed to the previous behavior.

edit: But I can certainly test it out on my machine were currently is no Picard installed, once you are finished with implementing.

Sure :) now that I learned to test it, I'll notify you when I'm satisfied with the behavior. I pushed (without --force this time) a certain behavior more similar to the previous behavior, but this time I tested it with the following options:

1. mbsubmit.picard-path is defined as picard-missing, causes an error message to come out, without exiting the process alltogether.
2. Using an absolute and a relative (to $PATH) path in mbsubmit.picard-path is both supported, thanks to shutil.which.

[JOJ0]

Hi, you are right using as_filename() might not be the best choice here. My impression was that as_filename() checks for an absolute path and errors if that's not the case. The magic it does with relative paths is not what we want here. Sorry I should have read docs more closely.

My idea actually was that it should be required for a user to always configure an absolute path since I've not seen automatic retrieval of paths for external programs in other places in beets. I might be wrong. I still think it's sufficient and most safe if we don't put too much magic here since we can't test on all platforms. which() works on Windows too as far as I read in the docs, but the executable will probably be could picard.exe or something. So the default of picard won't work anyway. But yeah if you want we do it with which, no problem. Default will work on Linux and Macs probably.

Because uf exiting: You don't have to raise another exception, you could just print the error using self._log.error() - it will be seen by the user and beets will stay alive.

Your current code looks much better now. My main problem with it was probably the cascaded if conditions and especially the check for `....startswith("/") - it was just hard to read what's going on here. That's why I suggeste a simple try/except.

What do you think of something like that:

    def picard(self, session, task):
        picard_path = self.config["picard_path"].as_str()
        paths = []
        for p in task.paths:
            paths.append(p.decode("utf-8"))
        try:
            subprocess.Popen([picard_path] + paths)
        except OSError as exc:
            self._log.error(f"Could not open picard.\n{exc}")

(or something similar including the usage of which())

Error looks like that and beets stays alive:

mbsubmit: Could not open picard.
[Errno 2] No such file or directory: 'tmp/picard_exec'

One more thing: Please rename the config option to not include dashes: picard_path

[JOJ0]

For decoding paths there is a utility in beets.util you could use:

from beets.util import displayable_path
...
...
            paths.append(displayable_path(p))

[doronbehar]

Thanks for all the suggestions! I liked even more the behavior with the try-except you demonstrated. I hope the usage with the displayable_path won't cause issues in obscure encoding environments. I pushed both of your suggestions and squashed everything to the 1st commit.

I still think it's sufficient and most safe if we don't put too much magic here since we can't test on all platforms. which() works on Windows too as far as I read in the docs, but the executable will probably be could picard.exe or something. So the default of picard won't work anyway. But yeah if you want we do it with which, no problem. Default will work on Linux and Macs probably.

I agree. Additionally, you should remember that even if a user installs picard and get a picard.exe executable somewhere in their C:\Program Files or whatever, it is likely this executable's directory doesn't get added to the system's $PATH. I also presume that when installing Picard via an installer, the user gets to choose the install location. Hence it is natural to ask all windows users to specify that path in their config.

[JOJ0]

Awesome, code looking perfect now. Some nitpicking on the docs: Maybe state that picard path needs to be configured.

[doronbehar]

Great :) I just installed Picard on a Windows platform and amended a sentence to the picard_path config option. Mainly meant for windows.

[JOJ0]

That help now reads very complete and I think it tells everything one needs to know on how to use these new features. Thanks a lot.

I'm happily merging this now. At this point: Thanks a lot for baring with us with those loads of iterations and refinements. Often it is not so easy for all participants to see the most straight-forward way for a feature/plugin. After all I personally think it was worth it that we didn't rush a merge and thanks again for your patients and the good work! :-)

[jackwilsdon]

Sorry for replying so late.

@JOJ0

So at first I thought that hiding the Picard part of the prompt (if Picard not installed) could be useful to unclutter the prompt but now after giving it some thought, I think that way: Showing the prompt might even encourage users to install Picard and help submitting stuff to MusicBrainz in the future. So why not let them try it, they get a friendly error message, and might even install Picard because of that :-)

I (and I imagine a lot of other users) use beets on headless machines (over SSH), which don't really have the option for using picard. I'm still in favor of hiding the prompt when Picard isn't installed.

It feels to me like the behaviour should be:

• If picard_path is empty and we don't find picard in PATH, don't show the prompt
• If picard_path is empty and we find picard in PATH, show the prompt
• If picard_path is non-empty, always show the prompt (even if it's invalid)

[JOJ0]

Hi @jackwilsdon, I just saw your comment about headless machines. We didn't take this interesting thought into account yet, I might have missed this perspective in our previous conversations. Please could you open a feature request issue for that? I think it won't be hard to implement. Then let's move all the discussion on how it should work over there. Sorry again I missed this part and thanks in advance!

[doronbehar]

I'm also a bit surprised that headless machines were a strong reason in favor of hiding the Picard prompt choice... we talked about it long ago and I got the impression that we settled down upon not hiding it. I am also a bit surprised though that users of headless machines even use the mbsubmit plugin - how did the plugin help you submit tracks up until now? If the plugin is not enabled, the picard prompt choice is disabled.

The logic @jackwilsdon suggested looks not bad, and I haven't thought about it myself back when we discussed this detail. However, I still think that having this PromptChoice enabled anyway encourages users to use Picard to quickly submit releases to MusicBrainz. I also hardly believe that there are so many users of headless machines, and that they will consider it a significant inconvenience.

[jackwilsdon]

how did the plugin help you submit tracks up until now?

The p prompt prints out the track listing in a format MusicBrainz can understand, which I copy from my terminal and paste into the MusicBrainz website which is open on my local machine.

I also hardly believe that there are so many users of headless machines, and that they will consider it a significant inconvenience.

From how many people use the LinuxServer.io Docker image (over 10M pulls, although this seems quite inflated), I'd say there's probably a decent number of people using beets without a desktop environment. It's not that it's really an inconvenience, it just seems less than ideal to be showing an option that we know will throw an error.

I'm not overly bothered by it at the end of the day (not enough to open a feature request), it just seemed like it'd be nice if we didn't show an extra prompt choice if we didn't need to (as there's already a good number of choices).