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 Disc Number support #139

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 21, 2023

Although ID3 and a number of tagging schemes do not support disc numbers - Disc numbers are can be an integral part of tagging for multi-disc albums, especially Classical music.

This PR adds Disc number support to audacious-plugins - there is a complementary PR to audacious adding support there. (audacious-media-player/audacious#63)

This implements Feature #603

Although ID3 and a number of tagging schemes do not support disc numbers - Disc numbers are can be an integral part of tagging for multi-disc albums, especially Classical music.

This PR adds Disc number support to audacious-plugins - there is a
complementary PR to audacious adding support there.

This implements Feature #603

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

@jlindgren90
Copy link
Member

As in audacious-media-player/audacious#63, the translation changes should be omitted.

I noticed just one trivial whitespace error. Other than that, the changes looks good to me.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Sorry for the delayed response. I've removed the po changes.

I noticed just one trivial whitespace error. Other than that, the changes looks good to me.

Oh? I can't see it - do you want to write a comment on the line that has the error so I can fix it?

Is there anything else I need to do to get this in?

@radioactiveman
Copy link
Member

I haven't found the time to test it but want to do so this weekend. The code looks good to me.

The whitespace issue mentioned by John is probably the if-statement in vorbis.cc (wrong indentation).
The CI errors are because we need to merge audacious-media-player/audacious#63 first.

Thanks @zeripath for the contributions. 👍

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

The whitespace issue mentioned by John is probably the if-statement in vorbis.cc (wrong indentation).

aha! The final bracket appeared to have gained a preceding whitespace - not sure how I did that. I've removed that.

I haven't found the time to test it but want to do so this weekend. The code looks good to me.

Cools. Hope it works for you.

@@ -151,6 +151,8 @@ static void read_comment (vorbis_comment * comment, Tuple & tuple)
tuple.set_int (Tuple::Track, atoi (tmps));
if ((tmps = vorbis_comment_query (comment, "DATE", 0)))
tuple.set_int (Tuple::Year, atoi (tmps));
if ((tmps = vorbis_comment_query (comment, "DISCNUMBER", 0)))
tuple.set_int (Tuple::Disc, atoi (tmps));
Copy link
Member

Choose a reason for hiding this comment

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

@zeripath: The if still has a wrong indentation.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@radioactiveman radioactiveman merged commit 397b4fd into audacious-media-player:master Jul 2, 2023
@zeripath zeripath deleted the disc-number-support branch September 15, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants