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

Use NullPaddedInt for R128 tags #2813

Merged
merged 4 commits into from
May 31, 2018
Merged

Use NullPaddedInt for R128 tags #2813

merged 4 commits into from
May 31, 2018

Conversation

autrimpo
Copy link
Contributor

@autrimpo autrimpo commented Feb 18, 2018

Fixes issue #2757

@sampsyo
Copy link
Member

sampsyo commented Feb 18, 2018

Thank you! Yes, this seems like exactly the right fix. Would you mind adding a quick summary to the changelog?

@autrimpo
Copy link
Contributor Author

Sure, I'll add a commit tomorrow.

@sampsyo
Copy link
Member

sampsyo commented Feb 19, 2018

Perfect; thank you! ✨

@sampsyo sampsyo merged commit 96a421d into beetbox:master May 31, 2018
sampsyo added a commit that referenced this pull request May 31, 2018
Use NullPaddedInt for R128 tags
@djl
Copy link
Member

djl commented Jun 16, 2018

I think this change may have caused beets to write (or rewrite) r128_track_gain and r128_album_gain tags on all of my files. This has made my usual rsync backup a little painful.

I'm going off memory here but I believe beet write -p wanted to change both fields from 0000 -> 0000 which to me looks like nothing should really have changed on the file themselves.

Has anyone else experienced this?

@sampsyo
Copy link
Member

sampsyo commented Jun 16, 2018

Hmm, that's bad news—ideally, the change would only allow the tag to be missing. Perhaps the change is that beets now wants to take the "zero" tags on the files and remove them altogether?

@autrimpo
Copy link
Contributor Author

It does show 0000 -> 0000 when removing the tags, so I think they only got removed, no other changes should happen. Are you able to check what changes actually occured?

@djl
Copy link
Member

djl commented Jun 16, 2018

Ah! It did indeed remove the tags. Not sure how I missed that.

False alarm then. Sorry for the confusion!

@sampsyo
Copy link
Member

sampsyo commented Jun 17, 2018

No problem! For clarity, maybe it would be a good idea for the UI to display something like "none" instead of "0000" when the value is null (missing).

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.

5 participants