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

Add support for pregap tracks #1493

Merged
merged 1 commit into from Jun 3, 2015
Merged

Conversation

ruippeixotog
Copy link
Contributor

This pull request adds support for pregap tracks in imports from Musicbrainz. This will only work after Musicbrainzngs releases a new version and beets starts using it, but everything should work as normal until then.

Two things to note:

  • I opted to leave track_info.index starting at 1 even in the presence of pregap tracks, which means that users with per_disc_numbering deactivated will see the pregap track as track 1 and every other track as its "original" album track number plus one. In albums with a single disc that felt strange to me, but then I thought about albums with multiple discs and multiple pregap tracks - I could set the index of the pregap in the first disc as 0, but then I would have to go sequential from there, including other pregap tracks in the same release. As I thought that would be rather inconsistent I implemented as seen in this pull request, but suggestions would be welcome;
  • I'm not that fluent in Python, so please tell me if there is a more efficient/pythonic way to do this :)

Fixes #1104.

item.track = track_info.medium_index or track_info.index
item.track = track_info.medium_index
if item.track is None:
item.track = track_info.index
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a little tricky, an inline condition, like this:

item.track = track_info.medium_index if track_info.medium_index is not None else track_info.index

or a comment to the effect of "we want to let the track number be zero, but if the medium index is not provided we need to fall back to the overall index" would be helpful here.

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 did not inline the conditional because of its size; I'll add that comment there instead.

@sampsyo
Copy link
Member

sampsyo commented Jun 2, 2015

This looks great, and I agree with your reasoning about the per_disc_numbering distinction. It makes sense to make those all just indexed serially from 1. Would you mind adding to the documentation some discussion of that behavior?

Other than that (and pending a changelog entry), this looks great! Let me know when you're happy and we'll merge it.

@ruippeixotog
Copy link
Contributor Author

No problem, I'll describe the numbering behavior in the documentation and add a changelog entry if you want. Where in the documentation do you suggest I add that?

@sampsyo
Copy link
Member

sampsyo commented Jun 2, 2015

Good question. Certainly on the config page under per_disc_numbering, but perhaps we'll come up with somewhere else too.

@ruippeixotog
Copy link
Contributor Author

Sorry, I messed up with the local branches here. I added some text to the per_disc_numbering, please check if it looks good to you, as it's mostly a question of text style.

@ruippeixotog ruippeixotog reopened this Jun 3, 2015
@sampsyo sampsyo merged commit 93c8f83 into beetbox:master Jun 3, 2015
sampsyo added a commit that referenced this pull request Jun 3, 2015
Add support for pregap tracks
sampsyo added a commit that referenced this pull request Jun 3, 2015
@sampsyo
Copy link
Member

sampsyo commented Jun 3, 2015

Awesome; looks great! Thank you for addressing this feature, even though it spans two projects. ✨

I expanded the docs a little; let me know if I messed anything up.

sampsyo added a commit that referenced this pull request Jun 3, 2015
@pdf
Copy link
Contributor

pdf commented Jun 3, 2015

I'm not sure I agree with the decision to number always renumber all tracks if there's a pre-gap track (if I understand correctly what you've implemented here). Ripping tools will generally only produce a single file for the pregap, but I'm not entirely certain how to handle metadata that includes multiple pre-gap entities.

Ignore my rambling, misunderstood.

LordSputnik pushed a commit to LordSputnik/beets that referenced this pull request Jul 6, 2015
LordSputnik pushed a commit to LordSputnik/beets that referenced this pull request Jul 6, 2015
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.

HTOA/pregap tracks (AKA track 0)
3 participants