Advanced API ?

[Zeugma440]

Continued from #121 (comment) @sandreas

Another thing that would be very handy and is missing: The Properties cannot be loaded or saved by a string / enum / Collection

ATL actually uses a map of all "simple" properties (e.g. non chapters, non pictures, non lyrics) internally, so that shouldn't be a problem to do that. What would be your ideal API ?

So one solution might be to have just one Dictionary to store everything: Track.Properties<string, object> and only LINK the real Properties of Track to this dictionary - Track.AdditionalFields would no longer be necessary and still everything would have defined behaviour. Only Problem would be to support multiple Metadata formats (like id3v2 + ape).

You've said it all... and ATL does intend on supporting multiple formats on the same file, so what you suggest is an actual problem for me too.

The upper level (public API) of ATL is user-oriented and has no clue about which property is mapped to which technical ID. So when you say "Track, please store A into SortAlbum and B into AdditionalFields["TSOA"]", Track has no clue that you're asking for something contradictory. Plus what might be contradictory for ID3v2 is absolutely fine for APEtag 🤦

So what I would do (in the long term): [...]

Part of what you describe seems awfully close to what I've already done on TagData and MetaDataHolder. It would be a kind of waste if you reinvented the wheel beside ATL. Imho, we should converge together towards a unique solution - maybe an "advanced API" to ATL for coders who are knowledgeable about audio metadata ?

[sandreas]

ATL actually uses a map of all "simple" properties (e.g. non chapters, non pictures, non lyrics) internally, so that shouldn't be a problem to do that. What would be your ideal API ?

Well, the problem is not only having an enum for each (simple) property. The main problem that I see here is, that AdditionalFields is kind of undefined. You can stuff everything in here and it is not based on internal names, but on real tag placeholders (e.g. TIT1). That means you can do:

track.AdditionalFields["TIT1"] = "an id3 group";
track.AdditionalFields["©grp"] = "an mp4 group";
track.Save();

What is gonna be saved? You just don't know without the context or the mapping.

Pro:

• Easy to use

Con:

• Untyped
• Mapped fields show undefined behaviour

Removing AdditionalFields would be a problem because this field makes atldotnet so flexible.

So what I would suggest is basically having no AdditionalFields on the main layer but encapsulate this with methods for restricted / normalized access:

class AdvancedTrack {
    // all mapped values are stored here
    public IDictionary<Field, object?> Fields = new();
    
    public int? Bpm
    {
        get => GetField<int?>(Field.BPM);
        set => SetField(Field.BPM, value);
    }

    private T? GetField<T>(Field field) {
        // return casted Fields[field];
    }

    public Field? SetAdditionalField(Format f, string key, object? value) {
         // this should be chained somehow
         // all unmapped values go the the according container, depending on the format and the key
         // you could even validate, if this key is ok for the chosen format.
         if(f.Name = "id3v2" && key == "TBPM") {
             Bpm = value;
             return Field.BPM;
         }
         return null; // means not mapped
    }
    // maybe with Generics?
    public object? GetAdditionalField(string key, Format? f = null) {
         
         if(f != null) {
              return MetadataFormats[f].Get(key);
         }
         foreach(var mf in MetadataFormats) {
            if(mf.ContainsKey(key)) {
                return mf[key];
            }
         }
         return null;
    }
    // ...
}

This way it would be possible to abstract AdditionalFields to a defined behaviour. But I have to think this through, this is the first thing that came to my mind.

[sandreas]

So, after trying some things out, I would like to mention a few things first:

Internal classes

The following classes / values are internal and therefore cannot be used in an Implementation of a custom Track class, although they are used in 'Track` itself:

• AudioFileIO
• Utils
• TrackUtils
• Settings.InternalValueSeparator

Is that intended?

API For AdvancedTrack

And here are some thoughts for an AdvancedTrack API, that stores data a little different. This is only meant as inspiration - not as ready implementation, but I think there are some good ideas in it.

• There is one big IEnumerable to store all tags (below I use a simple double Dictionary approach - but it could also be a simple list of TagField objects - I don't know if you like even one of these approaches)
• This way, GetCustomField and SetCustomField (I used Custom instead of Additional) can also be used to add fields with string keys without breaking or hiding the mapped fields (e.g. AdvancedTrack.Album)
• You can easily access or set values for different formats without breaking the current Track API
• You could implement a Dirty flag on the AdvancedTrack to see if a save is necessary or, if required implement a per field approach with TagField.IsDirty - even an AdvancedTrack.FieldHistory would be possible, where you store all the changes that have been made
• It would be possible to store the generic mapping not in the AdvancedTrack but within the IAudioDataIO, e.g. IAudioDataIO.SupportsCustomField(string field) and Field? f = IAudioDataIO.MapStringToField(string field) - so this would be an improvement, you only have to store the mapping at one place - new fields would be very easy to add
• I did not think about a RemoveField Method, but I think this should be easy to accomplish
• Performance may be a major issue here, I did not try it

// all supported metadata formats / versions as single enum value, which makes mapping very easy (as well as parsing a string format)
enum TagFormat
{
    Id3V23,
    Mp4,
    Ape,
}


enum FieldType
{
    Invalid, // if a string key is not supported by a specific metadata format
    Mapped, // if a string key is mapped to a generic field (e.g. Album => TALB (id3v23))
    Custom // if a string key is NOT mapped to a generic field (e.g. TXXX:CUSTOM id3v23)
}

// currently unused
// it could be used instead of a MetadataFormats dictionary
// Problem would be that every setter has to "search" for a format and key (slow?)
class TagField
{
    public TagFormat format { get; set; }
    public string Key { get; set; }
    public Field? GenericField { get; set; }
    public object? Value { get; set; }
    public object? OriginalValue { get; set; }
    public bool MarkedForDeletion { get; set; }
    public bool IsDirty => EqualityComparer<object?>.Default.Equals(OriginalValue, Value);
}

interface IAdvancedTrack
{
    public object? GetField(Field field);
    public T? GetField<T>(Field field);
    public FieldType SetField(Field field, object? value, TagFormat? format);

    public object? GetCustomField(string field, TagFormat? format);
    public FieldType SetCustomField(string field, object? value, TagFormat? format);
}

class AdvancedAdvancedTrack : IAdvancedTrack
{
    public string? Album
    {
        // todo: better use GetField<string?>(Field.ALBUM)
        get => GetField(Field.ALBUM)?.ToString(); 
        set => SetField(Field.ALBUM, value);
    }
    
    private readonly TagFormat _preferredTagFormat;
    private Dictionary<TagFormat, Dictionary<string, object?>> MetadataFormats = new();

    public AdvancedAdvancedTrack(TagFormat preferredTagFormat)
    {
        // todo: this must be loaded from the file or specified on initialisation
        _preferredTagFormat = preferredTagFormat;
    }

    public object? GetField(Field field) => GetField(field, _preferredTagFormat);
    public object? GetField(Field field, TagFormat format)
    {
        if (!MetadataFormats.ContainsKey(format))
        {
            return null;
        }

        var stringField = MapFieldToString(format, field);
        
        return stringField != null && MetadataFormats[format].ContainsKey(stringField)
            ? MetadataFormats[format][stringField]
            : null;
    }
    

    // todo: this could be stored in IAudioDataIO, so the mapping could be stored and verified at one place
    private static string? MapFieldToString(TagFormat tagFormat, Field field) => tagFormat switch
    {
        TagFormat.Id3V23 => MapId3V23FieldToString(field),
        _ => null
    };

    private static string? MapId3V23FieldToString(Field field) => field switch
    {
        Field.ALBUM => "TALB",
        _ => null
    };

    
    private static Field? MapStringToField(TagFormat tagFormat, string field) => tagFormat switch
    {
        TagFormat.Id3V23 => MapId3V23StringToField(field),
        _ => null
    };

    private static Field? MapId3V23StringToField(string field) => field switch
    {
        "TALB" => Field.ALBUM,
        _ => null
    };


    public FieldType SetField(Field field, object? value, TagFormat? format=null)
    {
        var tagFormat = format ?? _preferredTagFormat;
        var key = MapFieldToString(tagFormat, field);
        if (key == null) return FieldType.Invalid;
        MetadataFormats[tagFormat][key] = value;
        return FieldType.Mapped;
    }


    public object? GetCustomField(string field, TagFormat? format)
    {
        var tagFormat = format ?? _preferredTagFormat;
        var mappedField = MapStringToField(tagFormat, field);
        if (mappedField != null)
        {
            return GetField((Field)mappedField, tagFormat);
        }

        return MetadataFormats.ContainsKey(tagFormat) && MetadataFormats[tagFormat].ContainsKey(field)
            ? MetadataFormats[tagFormat][field]
            : null;
    }

    public FieldType SetCustomField(string field, object? value, TagFormat? format)
    {
        var tagFormat = format ?? _preferredTagFormat;
        var mappedField = MapStringToField(tagFormat, field);
        if (mappedField != null)
        {
            SetField((Field)mappedField, value, tagFormat);
            return FieldType.Mapped;
        }
        // todo: validate, if custom field can be set, otherwise return FieldType.Invalid
        MetadataFormats[tagFormat][field] = value;
        return FieldType.Custom;
    }

    public T? GetField<T>(Field field)
    {
        // todo: return casted field
        return default;
    }
}

What do you think? :-) (Hint: If you make the mentioned internal classes public, I could give this a go and try it in my library)

[sandreas]

@Zeugma440 Hey, did you find the time to look into this idea? Don't feel pushed but since there is a new release I remembered that there was something on my list :-)

[Zeugma440]

@sandreas Yesterday's release was more like a bugfix of some sorts, as the bit depth formula I posted gave wrong results on several formats, and real MIME-types were missing.

I did not forget about our plans, don't worry. I'm just load-balancing a lot of stuff every week (professional, personal, another project of mine with a very large userbase) and didn't find the time to work on evolving the API yet.

[sandreas]

I did not forget about our plans, don't worry. I'm just load-balancing a lot of stuff every week (professional, personal, another project of mine with a very large userbase) and didn't find the time to work on evolving the API yet.

@Zeugma440
I totally understand this because it is the same thing on my side :-)

However, I did not mean the whole API suggestion I made but only the internal classes in Track that cannot be used in class libraries:

• AudioFileIO
• Utils
• TrackUtils
• Settings.InternalValueSeparator

If you would make these public in the next release, I could try to implement something myself, otherwise I have to go for a pull request which would just put even more work on your shoulders or fork the repo with a completely separate build process, which would make me feel uncomfortable in many ways :-)

Maybe there is a good reason to NOT make these classes public, but if you are ok with it, I would definitely appreciate it and could write my own AdvancedTrack :-)

[Zeugma440]

In the first place, some classes have been made internal to minimize the surface of the API, which brings :

• Client-side : A clearer API
• Library-side : Less elements to worry about regarding retro-compatibility

That being said, you're the only person so far who expressed the need for an advanced usage of the library. I need to think this through before making a move. I wouldn't mind a pull request with an alternate implementation of Track, though... maybe it could help us refine the idea together !

[sandreas]

I wouldn't mind a pull request with an alternate implementation of Track, though... maybe it could help us refine the idea together !

Phew, this is a big sign of trust :-) I'm not sure that I'm skilled for this but hey, I'll try to find the time and we'll see what happens ;) Thanks for the quick response.

[sandreas]

@Zeugma440
Ok, today I found the time to put some effort into this. These questions came up after checking out the project:

• Which language version should I use for the project? (Can I use 9.0?)
• Why is Benchmark and UI-Test on .NETFramework 4.8 with C# 7.3, while ATL is on net48;netcoreapp3.1;netstandard2.1?

I ask this, because having nullable for projects would be great for the new features, as well as having optional Generics

interface IAdvancedTrack
{
// ...
    // T? does not work on C# 8.0 or lower
    public T? GetField<T>(Field field);
// ...
}

[Zeugma440]

I made those choices to make ATL usable on as many .NET-compatible platforms as possible.

C#9.0 is not compatible with the library's current minimum requirements (.NET Framework 4.8, .NET Standard 2.1 and .NET CORE 3.1), as it requires .NET 5.

Before greenlighting an upgrade to these requirements, I should study the impacts on the code itself as well as potential compatibility issues with other environments (e.g. Mono). For the time being, I'd prefer you use the current language version (which should be 8.0 ?).

---

On the other hand, Benchmark and UI-Test are not meant for distribution, and are included there for development and testing purposes. You can do whatever you want with these.

[sandreas]

I made those choices to make ATL usable on as many .NET-compatible platforms as possible.

Ok, great.

I'd prefer you use the current language version (which should be 8.0 ?).

Ok, then I have to solve this differently (I'm totally fine with this).

On the other hand, Benchmark and UI-Test are not meant for distribution, and are included there for development and testing purposes. You can do whatever you want with these.

Yeah for the first PR (if it ever happens) I will not touch these and just copy over the unit tests for Track, just to discuss if my solution makes sense at all. I try to create a mostly Track compatible solution, so that AdvancedTrack could replace Track without side effects, but has some additional benefits. Maybe I'll cut of some old braids like PictureTokens...

Thanks, I'll try to push this forward, but it may take a while.