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 the OriginalDate tag #1

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Add support for the OriginalDate tag #1

merged 1 commit into from
Apr 21, 2017

Conversation

tremby
Copy link
Contributor

@tremby tremby commented Apr 16, 2017

This tag is useful when the user would like all releases of the same album to be sorted next to each other.

This corresponds to MusicPlayerDaemon/MPD#29

@tremby
Copy link
Contributor Author

tremby commented Apr 17, 2017

Please ignore for now; see the referenced MPD pull request for details.

@tremby tremby changed the title Add support for the OriginalYear tag Add support for the OriginalDate tag Apr 17, 2017
@tremby
Copy link
Contributor Author

tremby commented Apr 17, 2017

Ready for review.

@MaxKellermann
Copy link
Member

New tags must never be added in the middle, because this is an ABI change. Move it to the end.

@tremby
Copy link
Contributor Author

tremby commented Apr 20, 2017

Yikes, I see the comment saying exactly this in tag.h now. Sorry for not noticing that before.

This tag is useful when the user would like all releases of the same
album to be sorted next to each other.
@MaxKellermann MaxKellermann merged commit e5d0e50 into MusicPlayerDaemon:master Apr 21, 2017
@MaxKellermann
Copy link
Member

Sigh. Just to let you know: your commit caused an ABI breakage. Binaries compiled with libmpdclient 2.11 are now incompatible with version 2.12+. To prevent this, there is a big fat warning just a few lines below your change:

/* IMPORTANT: the ordering of tag types above must be
   retained, or else the libmpdclient ABI breaks */

@tremby
Copy link
Contributor Author

tremby commented Dec 21, 2017

Argh. You had already pointed this out above, and then I amended the commit with the version you merged, which I had thought was fixed. But I'd fixed it in the C file but not in the header.

Sorry.

Presumably you have fixed this, or do you want me to add a new patch?

@MaxKellermann
Copy link
Member

No, it's way too late now. More programs have been built with 2.12+, and we'd break them by changing the order again now.

@MaxKellermann
Copy link
Member

btw. the order in the C file doesn't matter. It uses C99 designed initializers, and let compiler will reorder them according to the specified index.

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.

2 participants