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

Request: Rename classes to allow import of multiple namespaces without ambiguous symbols #110

Open
daiplusplus opened this issue Jun 13, 2018 · 2 comments

Comments

@daiplusplus
Copy link

daiplusplus commented Jun 13, 2018

(I searched previous Issues in GitHub to see if anyone raised this point, I couldn't see anything)

(I'm aware the docs say the v2.0 API is stable, so this is a proposal for version 3)

I used TagLib-Sharp to write my utility program TeslaTags - thank you for this library!, but while using the library I experienced a number of ergonomic issues with TagLib, such as how class names are reused in child namespaces when they're used to represent subclasses, which shouldn't be done because it means we need to use C# in a non-idiomatic way - such as aliasing types (using MpegFile = TagLib.Mpeg.File) or using Go-style package/namespace prefixes (e.g. using mpeg = TagLib.Mpeg; .. mpeg.AudioFile mp3 = mpeg.AudioFile.Create(...).

Another issue is a conflict with System.IO that frequently occurrs because TagLib is invariably used to work with files and some of the type-names conflict (namely all of the File classes in TagLib, I can't use it "naked" in the same context as System.IO.File) so I end up needing to alias or prefix everything - the result is that my code isn't as readable as it should be.

I'd like to propose the following changes:

  • Rename TagLib.File to TaggedFile, to avoid conflicting with System.IO.File.
  • In each codec/format-specific namespace (e.g. TagLib.Aac, TagLib.Mpeg prefix their File subclass with the codec/format name. So TagLib.Tiff.File becomes TagLib.Tiff.TiffFile.
  • The inheritance heirarchy should be exposed through the class' name where it makes sense. Right now it's looking bad using the TIFF code as an example, as we have TagLib.Tiff.File < TagLib.Tiff.BaseTiffFile < TagLib.Image.File < TagLib.File - that's 3 types all named File!
    • I'd rename them TiffImageFile, BaseTiffImageFile, ImageFile and TaggedFile respectively. Using these names makes their relationship much easier to understand.
  • If a namespace only has a few types in it combine them in a type-specific namespace.
    • For example, TagLib.Tiff's types could be put into the TagLib.Image namespace.
  • Less important: but I wonder if file format namespaces should be grouped under a new namespace TagLib.Formats.* as a parent because it's unclear by sight if a namespace refers to a specific format or something else, for example I initially thought that TagLib.Audible.File was a superclass for all "audible" (audio) formats and not Audible.com's own format.
  • Similar to File, the specialised subclasses of TagLib.Tag (e.g. TagLib.Id3v2.Tag) should also be prefixed named, e.g. Id3v2Tag instead).

I'm happy to do this myself if you're willing to accept a PR for it.

I'm sure there's other convention/idiom issues that FxCop/Code-Analysis would pick up that that I'd throw into my changes too.

@daiplusplus
Copy link
Author

daiplusplus commented Jun 13, 2018

On a related note (for v3 ideas) I have some questions about the existing design. Though I see that a lot of the code dates from 2002-2005 which predates .NET 2.0 and C# 2.0 and many things we take for granted now like Generics and Extension methods. If we adopted them into the library then we can make it more .NET-idiomatic and future-proof with new versions of .NET Standard.

  • Remove ListBase and use System.Collections.Generic.List<T> directly with extension methods for the added functionality like InsertSort and Split.
    • As well as the typed ListBase subclasses like StringCollection and ByteVectorCollection. They can be replaced with List<T> for internal use and Collection<T> for certain public APIs (though the latest convention seems to be to use List<T> but expose it as IReadOnlyList<T>).
  • Remove the helper members from TagLib.Tag and reduce Tag to the minimum common functionality needed for all Tag subclasses, then have more specific subclasses, e.g. AudioTag (which would have Performers, Album, etc) so we don't end-up with non-usable members for Image files, for example.
    • The helper members, like JoinedPerformers and MusicBrainz should be moved to a decorator-pattern class (as an example reason, it was unclear at first if AlbumArtistsSort would simply return a sorted representation of AlbumArtists or if it was specifically the "Album Artist sort order" tag field that iTunes uses. It's also unclear which properties refer to actual fields and which are abstractions or helpers - and also when TagLib was performing concatenation/split as an abstraction or if the underlying tag file type actually supported multiple album artists natively (e.g. ID3v2 "supports" multiple artists by separating them with slashes, as opposed to having a native tag list struct).
    • The idea being that the actual members of the Tag subclass represent the functionality supported by that implementation, and the abstraction is now done by the decorator.
    • I feel this is needed because, for example, ID3v1 only supports a small subset of data members whereas ID3v2 is far more comprehensive, and I felt uncomfortable using this interface without knowing exactly what would happen if I were to set the AlbumArtist on an ID3v1 Tag object (would it silently fail? throw an exception? write to a side-channel file?). I'd rather be working directly with an actual Id3v1Tag class instance which wouldn't have that member at all.

@Starwer
Copy link
Contributor

Starwer commented Jul 13, 2018

For the namespace/class naming, it is indeed an unusual way to do it, and you are not the first user to complain about this. I was myself a bit surprised by this approach when I first tried this library. However, if you do not use the using TagLib; statement, the code is in fact quite readable. In the end, I prefer to have TagLib.Tiff.File in my code instead of a TiffFile. This is then pretty obvious what class you're dealing with, and not so much more verbose than the TiffFile writing. I wouldn't bother then to break the all backward compatibility of TagLib# for such a reason.

Remove ListBase and use System.Collections.Generic.List directly with extension methods for the added functionality like InsertSort and Split.

I do agree that using the standard types and generics would be an improvement.

Remove the helper members from TagLib.Tag and reduce Tag to the minimum common functionality needed for all Tag subclasses, then have more specific subclasses, e.g. AudioTag (which would have Performers, Album, etc) so we don't end-up with non-usable members for Image files, for example.

To be fair, I advocate the opposite approach, that we could develop code for a Tag instance regardless of its type, and have then a strong type-abstraction. In that aspect, the current architecture is quite convenient. I'd also like to promote the approach of, if possible, having a field apparently specific to a type also mapped to other types. When you think of it, having Performers and Album for an image is far from being total nonsense. I like to tag my photo with the persons that appear on it (Performers) and sort it per Album (like: "Summer Vacation 2017"). I must admit that the naming of the Tag-property becomes a bit hard to follow in that case (what EXIF will it map to ? What is the Album really representing for Video ?), but IMHO the gain worth this inconvenience. I've proposed a mapping for these audio-related tags to Video Tags in #69, and also explained why I think that type-abstraction is important.

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

No branches or pull requests

2 participants