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.

Sign up for GitHub

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

Jump to bottom

Fix Media Headers on Import #5134

Merged
merged 4 commits into from Mar 16, 2024
Merged

Fix Media Headers on Import #5134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11932eb
Fix Media Headers on Import
InvisibleFunction Mar 3, 2024
94e2945
Update Changelog
InvisibleFunction Mar 6, 2024
33c8ae7
Update in response to comments
InvisibleFunction Mar 7, 2024
b9ab0c9
Run black
InvisibleFunction Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions beets/autotag/mb.py
View file
Open in desktop
Expand Up @@ -595,8 +595,12 @@ def album_info(release: Dict) -> beets.autotag.hooks.AlbumInfo:

# Media (format).
if release["medium-list"]:
first_medium = release["medium-list"][0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for tagging the whole thing with the media type of the first media?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose this was just a quick solution when the UI was redisigned. If you want to go down that rabbit hole, it would be interesting how mixed media fetching and displaying was handled before our UI redesign. I can't remember if we had that issue or if that even was displayed in the old importer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want to recommend a gitsha I should look at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I found what you meant by the old UI and confirmed that it displayed bad info too. Here's what it looks like at 821e629:

me@compy:/Volumes/Music/ti/hh/Four$ beet-test imp .

/Volumes/Music/ti/hh/Four (14 items)
Tagging:
    Hampton Hawes with Barney Kessel, Shelly Manne & Red Mitchell - Four!
URL:
    https://musicbrainz.org/release/03f118d7-99bb-4daa-ad58-ab86d5325398
Disambiguation string key albumrelease does not exist.
(Similarity: 97.7%) (media, year) (MusicBrainz, 2xHybrid SACD (CD layer), 2023, US, Contemporary Records, CR00501, Contemporary Records Acoustic Sounds Series)
Hybrid SACD (CD layer) 1
 * Yardbird Suite (#2-1)                  -> Yardbird Suite (#1-1)
 * There Will Never Be Another You (#2-2) -> There Will Never Be Another You (#1-2)
 * Bow Jest (#2-3)                        -> Bow Jest (#1-3)
 * Sweet Sue (#2-4)                       -> Sweet Sue (#1-4)
 * Up Blues (#2-5)                        -> Up Blues (#1-5)
 * Like Someone in Love (#2-6)            -> Like Someone in Love (#1-6)
 * Love Is Just Around the Corner (#2-7)  -> Love Is Just Around the Corner (#1-7)
Hybrid SACD (CD layer) 2
 * Yardbird Suite (#1-1)                  -> Yardbird Suite (#2-1)
 * There Will Never Be Another You (#1-2) -> There Will Never Be Another You (#2-2)
 * Bow Jest (#1-3)                        -> Bow Jest (#2-3)
 * Sweet Sue (#1-4)                       -> Sweet Sue (#2-4)
 * Up Blues (#1-5)                        -> Up Blues (#2-5)
 * Like Someone in Love (#1-6)            -> Like Someone in Love (#2-6)
 * Love Is Just Around the Corner (#1-7)  -> Love Is Just Around the Corner (#2-7)
[A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?

Copy link
Member

@JOJ0 JOJ0 Mar 16, 2024 •
edited

Choose a reason for hiding this comment

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

Sorry for being very slow in responding these days. I was about to suggest looking into what was before the "new" UI was introduced in this PR: #3721

Seems like you found what you are looking for already, but I think the sha you posted is missing a character in the end? That was the merge commit: 821e6296.

But wait there is something wrong with GitHub cutting of that last character. The link is off, when pasted in here it looses the 6 character in the end: 821e629, but actually it doesn't matter anymore. The point is: It was broken before and you improved, so I think we are ready to merge here, it's definitely much better than before, if not fixed for good!!! :-)

info.media = first_medium.get("format")
# If all media are the same, use that medium name
if len(set([m.get("format") for m in release["medium-list"]])) == 1:
info.media = release["medium-list"][0].get("format")
# Otherwise, let's just call it "Media"
else:
info.media = "Media"
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a better idea for the default name. "Mixed Media" would maybe be more descriptive, but it is a way too long expression. So I think "Media" was a good choice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I liked Mixed Media but worried about the length. I'm not sure where all this is used so maybe "Mixed Media" would be better but we could shorten it in certain places if needed?

Either way I think this change is an improvement over what's there.


if config["musicbrainz"]["genres"]:
sources = [
Expand Down
10 changes: 6 additions & 4 deletions beets/ui/commands.py
View file
Open in desktop
Expand Up @@ -453,14 +453,16 @@ def show_match_details(self):

def make_medium_info_line(self, track_info):
"""Construct a line with the current medium's info."""
media = self.match.info.media or "Media"
track_media = track_info.get("media", "Media")
# Build output string.
if self.match.info.mediums > 1 and track_info.disctitle:
return f"* {media} {track_info.medium}: {track_info.disctitle}"
return (
f"* {track_media} {track_info.medium}: {track_info.disctitle}"
)
elif self.match.info.mediums > 1:
return f"* {media} {track_info.medium}"
return f"* {track_media} {track_info.medium}"
elif track_info.disctitle:
return f"* {media}: {track_info.disctitle}"
return f"* {track_media}: {track_info.disctitle}"
else:
return ""

Expand Down
3 changes: 3 additions & 0 deletions docs/changelog.rst
View file
Open in desktop
Expand Up @@ -292,6 +292,9 @@ Bug fixes:
and caching in `zsh`.
:bug:`3546`
* Remove unused functions :bug:`5103`
* Fix bug where all media types are reported as the first media type when
importing with MusicBrainz as the data source
:bug:`4947`

For plugin developers:

Expand Down