-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
@@ -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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
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
After
To Do
Documentation. (If you've added a new command-line flag, for example, find the appropriate page underdocs/
to describe it.)docs/changelog.rst
to the bottom of one of the lists near the top of the document.)