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

Fix Media Headers on Import #5134

Merged
merged 4 commits into from Mar 16, 2024
Merged

Conversation

InvisibleFunction
Copy link
Contributor

@InvisibleFunction InvisibleFunction commented Mar 3, 2024

Description

Fixes when you import a release with mixed media, the importer just shows the first medium for every medium.
#4947

Is this the best fix? Any ideas for improving the format?

Behavior

Before

$ beet imp .

/Volumes/Music/ti/hh/Four (14 items)

  Match (97.7%):
  Hampton Hawes with Barney Kessel, Shelly Manne & Red Mitchell - Four!
  ≠ media, year
  MusicBrainz, 2xHybrid SACD (CD layer), 2023, US, Contemporary Records, CR00501, Contemporary Records Acoustic Sounds Series
  https://musicbrainz.org/release/03f118d7-99bb-4daa-ad58-ab86d5325398
  * Artist: Hampton Hawes with Barney Kessel, Shelly Manne & Red Mitchell
  * Album: Four!
  * Hybrid SACD (CD layer) 1
     ≠ (#2-1) Yardbird Suite (6:43) -> (#1-1) Yardbird Suite (6:40)
     ≠ (#2-2) There Will Never Be Another You (7:02) -> (#1-2) There Will Never Be Another You (6:59)
     ≠ (#2-3) Bow Jest (6:33) -> (#1-3) Bow Jest (6:30)
     ≠ (#2-4) Sweet Sue (5:37) -> (#1-4) Sweet Sue (5:36)
     ≠ (#2-5) Up Blues (5:11) -> (#1-5) Up Blues (5:10)
     ≠ (#2-6) Like Someone in Love (3:23) -> (#1-6) Like Someone in Love (3:21)
     ≠ (#2-7) Love Is Just Around the Corner (5:45) -> (#1-7) Love Is Just Around the Corner (5:43)
  * Hybrid SACD (CD layer) 2
     ≠ (#1-1) Yardbird Suite (6:43) -> (#2-1) Yardbird Suite (6:40)
     ≠ (#1-2) There Will Never Be Another You (7:02) -> (#2-2) There Will Never Be Another You (6:59)
     ≠ (#1-3) Bow Jest (6:33) -> (#2-3) Bow Jest (6:30)
     ≠ (#1-4) Sweet Sue (5:37) -> (#2-4) Sweet Sue (5:36)
     ≠ (#1-5) Up Blues (5:11) -> (#2-5) Up Blues (5:10)
     ≠ (#1-6) Like Someone in Love (3:23) -> (#2-6) Like Someone in Love (3:21)
     ≠ (#1-7) Love Is Just Around the Corner (5:45) -> (#2-7) Love Is Just Around the Corner (5:43)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?

After

$ beet imp .

/Volumes/Music/ti/hh/Four (14 items)

  Match (97.7%):
  Hampton Hawes with Barney Kessel, Shelly Manne & Red Mitchell - Four!
  ≠ media, year
  MusicBrainz, 2xMedia, 2023, US, Contemporary Records, CR00501, Contemporary Records Acoustic Sounds Series
  https://musicbrainz.org/release/03f118d7-99bb-4daa-ad58-ab86d5325398
  * Artist: Hampton Hawes with Barney Kessel, Shelly Manne & Red Mitchell
  * Album: Four!
  * Hybrid SACD (CD layer) 1
     ≠ (#2-1) Yardbird Suite (6:43) -> (#1-1) Yardbird Suite (6:40)
     ≠ (#2-2) There Will Never Be Another You (7:02) -> (#1-2) There Will Never Be Another You (6:59)
     ≠ (#2-3) Bow Jest (6:33) -> (#1-3) Bow Jest (6:30)
     ≠ (#2-4) Sweet Sue (5:37) -> (#1-4) Sweet Sue (5:36)
     ≠ (#2-5) Up Blues (5:11) -> (#1-5) Up Blues (5:10)
     ≠ (#2-6) Like Someone in Love (3:23) -> (#1-6) Like Someone in Love (3:21)
     ≠ (#2-7) Love Is Just Around the Corner (5:45) -> (#1-7) Love Is Just Around the Corner (5:43)
  * Hybrid SACD (SACD layer, 2 channels) 2
     ≠ (#1-1) Yardbird Suite (6:43) -> (#2-1) Yardbird Suite (6:40)
     ≠ (#1-2) There Will Never Be Another You (7:02) -> (#2-2) There Will Never Be Another You (6:59)
     ≠ (#1-3) Bow Jest (6:33) -> (#2-3) Bow Jest (6:30)
     ≠ (#1-4) Sweet Sue (5:37) -> (#2-4) Sweet Sue (5:36)
     ≠ (#1-5) Up Blues (5:11) -> (#2-5) Up Blues (5:10)
     ≠ (#1-6) Like Someone in Love (3:23) -> (#2-6) Like Someone in Love (3:21)
     ≠ (#1-7) Love Is Just Around the Corner (5:45) -> (#2-7) Love Is Just Around the Corner (5:43)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?

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. (Very much encouraged but not strictly required.) (Would love to do this if someone could give me pointers.)

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

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

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

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.

beets/autotag/mb.py Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
@JOJ0 JOJ0 merged commit b09806e into beetbox:master Mar 16, 2024
13 checks passed
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.

None yet

2 participants