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

Set multiple tags #28

Closed
dgadelha opened this issue Jul 6, 2016 · 11 comments

Comments

@dgadelha
Copy link

commented Jul 6, 2016

How can I set multiple tags, in a way to achieve

ARTIST=artistA
ARTIST=artistB

?

Would setting comment["ARTIST"] to an string[] be compatible with this, or I need to create a new VorbisComment instance for each artist and add to the metadata?

@dgadelha

This comment has been minimized.

Copy link
Author

commented Jul 6, 2016

I think this could not be possible because you save the comments as a Dictionary<string, string>;

A solution can be implemented from here.

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2016

You are correct. I missed the fact that these field names don't have to be unique. This is going to be an issue if you have an existing file with multiple tags too.

Not sure how to fix it without breaking the API though. I'll look into it.

Thanks a lot for the feedback, it means a lot!

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2016

What would you think of something like this? This would leave the original API untouched. It's not as clean though. Given my (probably) small user-based I might be better off breaking the API:

                using (FlacFile flac = new FlacFile(newFile))
                {
                    flac.VorbisComment.SetAllValues("ARTIST", 
                        new VorbisCommentValues
                        {
                            "Aaron", "dgadelha"
                        });

                    // Save flac file
                    flac.Save();
                }
                using (FlacFile flac = new FlacFile(newFile))
                {
                    VorbisCommentValues values = flac.VorbisComment.GetAllValues("ARTIST");
                    Assert.AreEqual(2, values.Count);
                    Assert.AreEqual("Aaron", values[0]);
                    Assert.AreEqual("dgadelha", values[1]);
                }

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2016

@dgadelha would you like to check out these two pull requests: #29 and #30.
Both seem to solve the problem but the first one does it without breaking the original API.

Have a look in the tests to see the usage, especially "AddMultipleVorbisFields" in the write tests.

I would personally go for breaking the API, since adjusting the existing code isn't really that hard (just add .First) and this library (I think) has a small (but non-zero, I'm happy to hear) user-base. Additionally multiple tags is "encouraged" by the VORBIS spec, so making it the default would make sense.

Please let me know what you think, if you have the time.

@dgadelha

This comment has been minimized.

Copy link
Author

commented Jul 11, 2016

Hi @AaronLenoir!

Thanks for the quick implementations.

Both ways looks great for me, just having this feature implemented would be nice. And yes, my request is because Vorbis recommend to use multiple ARTIST tags if there are really multiple artists, and both Windows and Linux support it very well.

Although, from my coding style preference, the one who "breaks the API" also breaks the most common C# coding style.

There's also another complex way, allowing the use to set both

comment["ARTIST"] = "Artist";

and

comment["ARTIST"] = new string[] { "ArtistA", "ArtistB" };

By changing all code to reference them as object instead of string and when saving the tags to the file, check with ReferenceEquals or with is if it is an string or string array. Could also allow to set integers :)

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2016

Do you mean it breaks a common C# coding style because it doesn't allow you to do this anymore?

comment["ARTIST"] = "Artist";

I see what you mean.
However, making it object might open the door to a lot of other string conversion issues. You could put an integer in the tag, but you could never get it back as an integer. Since you don't know if the person who put it there meant "01" or 1, for example.

I guess I could put a safeguard in there if it's not a supported type, but that would defer what are now compile-time errors to be run-time errors.

Would it make more sense if it was like this:

comment["ARTIST"].Value = "Artist";
// OR
comment["ARTIST"] = new VorbisCommentValues("Artist");
// OR
comment["ARTIST"] = new VorbisCommentValues(new string[] { "Artist A", "Artist B" });

So .Value instead of .First. And some overloads in the constructor of VorbisCommentValues.

I'll have to sleep about it :-)

@dgadelha

This comment has been minimized.

Copy link
Author

commented Jul 11, 2016

Hi,

.Value sounds really better than .First.

Thanks.

@AaronLenoir AaronLenoir added this to the Version 2.0 milestone Jul 12, 2016

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 12, 2016

I merged to master, planning to make this version 2.0 and just release it as non-beta to nuget.

Again, thanks for your interest. It's really more fun if other people are using your stuff :-)

Out of curiousity, can you share any info on what you're doing with the library?

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 12, 2016

Additionally, it's pending on nuget and should be available soon: https://www.nuget.org/packages/FlacLibSharp/2.0.0

@AaronLenoir AaronLenoir reopened this Jul 12, 2016

@AaronLenoir AaronLenoir removed the TODO label Jul 12, 2016

@dgadelha

This comment has been minimized.

Copy link
Author

commented Jul 12, 2016

Hi,

Thanks for the quick help!

I'm using it to apply meta tags from FLAC files scrapped from the internet, more from a personal project.

But I'm sure there are many other people using it, it is a great library! :)

@dgadelha dgadelha closed this Jul 12, 2016

@AaronLenoir

This comment has been minimized.

Copy link
Owner

commented Jul 13, 2016

Thanks a lot, glad you find it useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.