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.

Sign up for GitHub

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

Jump to bottom

Add support for the OriginalDate tag #29

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Add support for the OriginalDate tag #29

merged 1 commit into from
Apr 21, 2017

Conversation

tremby
Copy link
Contributor

@tremby tremby commented Apr 16, 2017

See https://picard.musicbrainz.org/docs/mappings/

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

@tremby tremby mentioned this pull request Apr 16, 2017
Merged
@tremby
Copy link
Contributor Author

tremby commented Apr 17, 2017 •
edited
Loading

Please ignore this for now; I somehow missed that there is an OriginalDate tag, with an equivalent ID3 mapping, which is more useful than OriginalYear.

@tremby tremby force-pushed the add-original-year-tag branch from 657f926 to 858cc36 Compare April 17, 2017 09:30
@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.

tremby
tremby commented Apr 17, 2017
View reviewed changes
src/tag/Id3Scan.cxx
@@ -55,6 +55,10 @@
#define ID3_FRAME_ALBUM_ARTIST "TPE2"
#endif

#ifndef ID3_FRAME_ORIGINAL_RELEASE_DATE
#define ID3_FRAME_ORIGINAL_RELEASE_DATE "TDOR"
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some ID3 tag identifiers are in definitions up here, others aren't. I wasn't sure which style to mimic. Also, some definitions are indented, others are not; ditto.

@MaxKellermann
Copy link
Member

Fails to build:

src/db/plugins/ProxyDatabasePlugin.cxx:161:23: error: 'MPD_TAG_ORIGINAL_DATE' was not declared in this scope

This needs to check the libmpdclient version (at compile time).

@tremby
Copy link
Contributor Author

tremby commented Apr 18, 2017

Could you please give a further hint at how to do this? Should I check for libmpdclient at one version greater than the currently-released one?

@MaxKellermann
Copy link
Member

No, just greater than the current one.

@tremby
Copy link
Contributor Author

tremby commented Apr 18, 2017

Ah, of course! I'll fix this stuff up after work today. Thanks.

@tremby tremby force-pushed the add-original-year-tag branch from 858cc36 to b365ac9 Compare April 20, 2017 23:27
@tremby
Copy link
Contributor Author

tremby commented Apr 20, 2017

As you see in my libmpdclient patch I've moved the constants so the binary API doesn't suffer. I also updated configure.ac in my local working copy to have a 2.12.0 version number, reconfigured it and rebuilt and reinstalled. I've switched back and forth between stock 2.11 and this version and tested building MPD both ways.

When I was adding the preprocessor version checks I noticed that other such checks use LIBMPDCLIENT_CHECK_VERSION. This macro checks that the version being compiled against is equal to or greater than the specified version, but you were hinting that I should check for strictly greater than, and then specify the current version (2.11.0). I'm wondering if perhaps you misremembered how this macro works, or perhaps you were encouraging me to add a new macro, or perhaps to explicitly use the LIBMPDCLIENT_(MAJOR|MINOR|PATCH)_VERSION constants.

For now I've used the macro, and assumed that 2.12.0 will be the first to support these tags.

Please let me know if you'd prefer it done another way.

tremby
tremby commented Apr 20, 2017
View reviewed changes
src/tag/Type.h
@@ -46,6 +46,7 @@ enum TagType
TAG_NAME,
TAG_GENRE,
TAG_DATE,
TAG_ORIGINAL_DATE,
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 wasn't able to get a preprocessor check here without getting an error. If this line of code needs a check, please help me with it.

Copy link
Member

Choose a reason for hiding this comment

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

Why would this need a preprocessor check? This is MPD's internal definition, not one imported from libmpdclient (which in turn mirrors MPD's internal definitions to use MPD's features).

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 don't know the answer; I wasn't sure where it would be needed and where it would not, and was trying to err on the side of caution. C++ is not my strong suit.

@MaxKellermann
Copy link
Member

MaxKellermann commented Apr 21, 2017 •
edited
Loading

This macro checks that the version being compiled against is equal to or greater than the specified version, but you were hinting that I should check for strictly greater than

You're right; while my suggestion was correct, it is more important to use libmpdclient's version check macro, which is not designed to use not-yet-released features.

MaxKellermann
MaxKellermann requested changes Apr 21, 2017
View reviewed changes
src/output/plugins/HaikuOutputPlugin.cxx Outdated
@@ -27,6 +27,8 @@
#include "system/Error.hxx"
#include "Log.hxx"

#include <mpd/client.h>
Copy link
Member

Choose a reason for hiding this comment

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

This adds a dependency from Haiku on libmpdclient. That's not good!

src/output/plugins/HaikuOutputPlugin.cxx Outdated
@@ -395,6 +397,9 @@ HaikuOutput::SendTag(const Tag &tag)
break;
case TAG_GENRE:
case TAG_DATE:
#if LIBMPDCLIENT_CHECK_VERSION(2,12,0)
Copy link
Member

Choose a reason for hiding this comment

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

... and this condition is unnecessary, because this has nothing to do with libmpdclient.

src/tag/Id3Scan.cxx Outdated
@@ -55,6 +57,12 @@
#define ID3_FRAME_ALBUM_ARTIST "TPE2"
#endif

#if LIBMPDCLIENT_CHECK_VERSION(2,12,0)
Copy link
Member

Choose a reason for hiding this comment

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

Same here. This new tag should always be available, even when not building with libmpdclient.

src/tag/Names.c Outdated
@@ -20,6 +20,8 @@
#include "config.h"
#include "Type.h"

#include <mpd/client.h>
Copy link
Member

Choose a reason for hiding this comment

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

... and so on... you get the picture?

@tremby
Add support for the OriginalDate tag
ccb4f44 
See https://picard.musicbrainz.org/docs/mappings/

This tag is useful when the user would like all releases of the same
album to be sorted next to each other.
@tremby tremby force-pushed the add-original-year-tag branch from b365ac9 to ccb4f44 Compare April 21, 2017 16:48
@tremby
Copy link
Contributor Author

tremby commented Apr 21, 2017

Thanks for explaining. I guess I was overcomplicating things! Hopefully I have it looking right this time.

@MaxKellermann MaxKellermann merged commit ccb4f44 into MusicPlayerDaemon:master Apr 21, 2017
@tremby
Copy link
Contributor Author

tremby commented Apr 21, 2017 via email

@sss123next sss123next mentioned this pull request Aug 9, 2017
Closed
@fajard01 fajard01 mentioned this pull request Sep 6, 2017
Closed
@github1299 github1299 mentioned this pull request Aug 9, 2018
Closed
@avuton avuton mentioned this pull request Oct 28, 2020
Closed
@bishonenknife bishonenknife mentioned this pull request Sep 2, 2022
Closed
@lordoff lordoff mentioned this pull request Oct 3, 2022
Closed
@whatamesss whatamesss mentioned this pull request Jun 19, 2024
Closed
@skidoo23 skidoo23 mentioned this pull request Jul 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxKellermann MaxKellermann MaxKellermann requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tremby @MaxKellermann