Skip to content
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

Implement "Create new release on musicbrainz" feature #5300

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jonaswinkler
Copy link

@jonaswinkler jonaswinkler commented Jun 12, 2024

Description

Closes #1866.

Implement support to create new releases on musicbrainz from unmatched albums directly.

Screencast.from.2024-06-12.23-37-41.webm

See #5299.

  1. When the new option is chosen, the MB release editor is opened in the browser, with known metadata already filled in. The plugin must launch an internal web server to do that.
  2. When the release is saved / created on musicbrainz, musicbrainz redirects to the local web server, submitting the new release ID. beets proceeds to import the album with that ID, similar to what happens when an ID is provided manually. (That's not shown in the screen cast)

See documentation and comments for details.

If no unmatched album is available for testing, set configuration mbsubmit.threshold to 'strong' and run import with --timid to select the new option.

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests.
  • All todo marks still left in the code.
  • Add some more key comments to the code.
  • Add command to CLI
  • Do more testing on Windows and Linux after switch to Flask
  • Selecting Port "0" (let the OS choose) does not work when also specifying a hostname

beetsplug/mbsubmit.py Outdated Show resolved Hide resolved
@jonaswinkler jonaswinkler changed the title Feature/mbsubmit create release Implement "Create new release on musicbrainz" feature Jun 12, 2024
@jonaswinkler jonaswinkler force-pushed the feature/mbsubmit-create-release branch from f499389 to ffcce62 Compare June 15, 2024 15:22
@jonaswinkler jonaswinkler marked this pull request as ready for review June 15, 2024 15:23
@jonaswinkler
Copy link
Author

Hey @doronbehar, If you're around, have a look at this. Maybe I can remove the "Open with Picard" option now that this here is implemented?

@Serene-Arc Serene-Arc self-requested a review June 17, 2024 00:11
Copy link
Contributor

@Serene-Arc Serene-Arc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've made a couple comments but I'll also note that a fair amount of the changes in this PR are fairly boilerplate HTTP webserver stuff. Have you considered using something like simple-http-server or another package to deal with this? It would reduce the number of lines in the PR and make it easier to change in the future.

Comment on lines 92 to 105
formdata: dict
"""
Form data to be submitted to MB.
"""

result_release_mbid: Optional[str] = None
"""
Contains the release ID returned by MusicBrainz after the release was created.
"""

browser_opened: bool = False
"""
True when the user has opened the link for this task in the browser.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring comments go above variables. It's only classes and functions that have them below the definition. Confusing, I know

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they? See https://peps.python.org/pep-0258/#attribute-docstrings... also, PyCharm correctly picks up the docstrings this way.


.. code-block:: console

$ pip install beets[mbsubmit]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the switch to poetry, this is now outdated. Should be changed to the poetry equivalent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beetsplug/mbsubmit.py Outdated Show resolved Hide resolved
@jonaswinkler
Copy link
Author

Thanks for the PR! I've made a couple comments but I'll also note that a fair amount of the changes in this PR are fairly boilerplate HTTP webserver stuff. Have you considered using something like simple-http-server or another package to deal with this? It would reduce the number of lines in the PR and make it easier to change in the future.

I thought about not pulling int too many new dependencies. I can also use flask instead, since that's already used by a couple other plugins...?

@Serene-Arc
Copy link
Contributor

I don't think many dependencies is an issue that is too pressing. If it makes the code easier to read and maintain, that wins out over another dependency in my book. If flask is used by other plugins though, by all means use it!

@jonaswinkler
Copy link
Author

Thank you for the review! It's now much cleaner and shorter with flask.

@doronbehar
Copy link
Contributor

Hey @doronbehar, If you're around, have a look at this. Maybe I can remove the "Open with Picard" option now that this here is implemented?

Hey! Personally I won't mind it getting removed, as I'd definitely use this functionality once it will land! It also makes sense to me from a general point of view to remove it, but that needs an approval from the maintainers of beets.

Thanks for working on this feature!


.. code-block:: console

$ pip install "beets[mbsubmit]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now recommending that people install and manage beets with poetry, so if you wouldn't mind changing this, it should be poetry install beets --extras mbsubmit.

Comment on lines +83 to +96
formdata: Dict[str, str]
"""
Form data to be submitted to MB.
"""

browser_opened: bool = False
"""
True when the user has opened the link for this task in the browser.
"""

result_release_mbid: Optional[str] = None
"""
Contains the release ID returned by MusicBrainz after the release was created.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments go above variables, please.

def _wait_for_condition(self, condition: Callable):
t = threading.current_thread()
while not condition():
time.sleep(0.5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? This is a blocking call on the other thread, so is there any penalty to removing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let mbsubmit plugin open seeded "add release" page
3 participants