NCrusher74 / SwiftTaggerID3

A Swift library for reading and editing ID3 tags
Apache License 2.0
5 stars 2 forks source link

Restructuring frame handling #2

Closed NCrusher74 closed 4 years ago

NCrusher74 commented 4 years ago

Moving the restructuring to its own branch, since this isn't about parsing at the moment.

NCrusher74 commented 4 years ago

Okay, I'm leaving that other comment unresolved because it has a lot of code and discussion I need to refer back to.

I'm running into a thing I encounter occasionally where trying to track down all the errors in all the places gets too complicated and I start to get lost. Usually I try to take things in small chunks. I make a change, work on it until it builds, and move onto the next thing. I think case, however, with so many interlinked pieces, the changes are being made across multiple places and some errors are happening because I haven't gotten it right in one place so that's breaking another place and...yeah. Anyway. Specifics in the code.

NCrusher74 commented 4 years ago

I'm up to working on parsing the Genre frame, and the example for that from ID3TagEditor is so convoluted that I'm not sure I should use it as a model.

Here is the spec for versions 2.2 and 2.3:

TCON The 'Content type', which previously was stored as a one byte numeric value only, is now a numeric string. You may use one or several of the types as ID3v1.1 did or, since the category list would be impossible to maintain with accurate and up to date categories, define your own.

References to the ID3v1 genres can be made by, as first byte, enter "(" followed by a number from the genres list (appendix A.) and ended with a ")" character. This is optionally followed by a refinement, e.g. "(21)" or "(4)Eurodisco". Several references can be made in the same frame, e.g. "(51)(39)". If the refinement should begin with a "(" character it should be replaced with "((", e.g. "((I can figure out any genre)" or "(55)((I think...)". The following new content types is defined in ID3v2 and is implemented in the same way as the numerig content types, e.g. "(RX)".

 RX  Remix
 CR  Cover

And here is the spec for version 2.4:

TCON The 'Content type', which ID3v1 was stored as a one byte numeric value only, is now a string. You may use one or several of the ID3v1 types as numerical strings, or, since the category list would be impossible to maintain with accurate and up to date categories, define your own. Example: "21" $00 "Eurodisco" $00

You may also use any of the following keywords:

 RX  Remix
 CR  Cover

I think I'm close, but I'd like to run it by you to see if I did it right.

    init(decodingContents contents: Data.SubSequence,
         version: Version,
         layout: KnownFrameLayoutIdentifier,
         flags: Data) throws {
        self.flags = GenreFrame.defaultFlags(version: version)
        self.layout = layout
        var parsing = contents
        let encoding = GenreFrame.extractEncoding(data: &parsing, version: version)
        if version == .v2_2 || version == .v2_3 {
            let unparsedString = GenreFrame.extractTerminatedString(data: &parsing, encoding: encoding)
            self.genreType = extractAndInterpretParentheticalString(
                unparsedString: unparsedString)
            self.descriptionString = unparsedString
        } else {
            var strings: [String] = []

            while !parsing.isEmpty,
                let next = parsing.extractPrefixAsStringUntilNullTermination(encoding) {
                    strings.append(next)
            }
            for string in strings {
                self.genreType = extractAndInterpretParentheticalString(unparsedString: string)
                self.descriptionString = string
            }
        }
    }

    private func extractAndInterpretParentheticalString(unparsedString: String) -> GenreType? {
        if let range = unparsedString.range(of: #"(\\()\\w*\\d*(\\))"#, options: .regularExpression) {
            let genreWithParens = String(unparsedString[range])
            let genreWithoutParens = genreWithParens.dropLast(1).dropFirst(1)
            var genre: GenreType = .None
            if genreWithoutParens == "CR" {
                genre = .Cover
            } else if genreWithoutParens == "RX" {
                genre = .Remix
            } else if let genreInt = Int(genreWithoutParens),
                let validGenre = GenreType(rawValue: genreInt) {
                genre = validGenre
            }
            return genre
        }; return nil
    }

(I borrowed the regex from ID3TagEditor. I'm not sure if there's a better way.)

SDGGiesbrecht commented 4 years ago

You are still telling it to interpret parentheses in version 2.4. Isn’t the list of distinct strings instead of separating the pieces with parentheses?

My (only rough) understanding is the frame holds a list of genres. Older versions express the list with each entry in parentheses. New versions express it with each entry in a separate string.

That would mean your frame ought to have an array of genres: [GenreType]. And I don’t think there is any reason to hold onto the intermediate string (descriptionString); you only need it momentarily while you are parsing it or writing it out.

NCrusher74 commented 4 years ago

The genre frame is a complex one. There are a lot of ways it can come back.

The first is that it can come back as a one or more of a curated list of genres, which ID3v1 started with a barebones list and WinAmp and others extended. That's my GenreType enum. It has a rawValue of the interger code for the extended list of commonly recognized genres, plus static vars for RX (remix) and CR (cover).

Each one of those integer codes would be in parenetheses.

Then there's what the spec refers to as a "refinement" (what I'm calling the descriptionString) which is a freeform text offering more information. That would immediately follow the parens.

So, the frame may hold genreType: XX by itself (so genreDescription would be nil), or it may hold genreDescription: "I have this to say" (with genreType as nil) or it may hold genreType: XX, genreDescription "I have this to say about genre type XX".

Or, heaven help me, it may be:

[
genreType: XX, 
genreType: YY, genreDescription: "I have this to say about genre type YY", 
genreDescription: "hah! fooled you into thinking there was gonna be a ZZ!"
]

So, it's sort of like my role: String, person: String tupleArray, except that both role and person are optional, and in version 2.2 and 2.3, role is delimited by parens instead of a null character.

Ugh.

I think I can just refine my role: String, person: String model.

Did I at least handle the regex for the parens correctly?

SDGGiesbrecht commented 4 years ago

It’s still a list of entries though. This is a valid frame isn’t it?

(1)(2)refined(3)((refined in parentheses)(RX)

If so, you could think about it as this:

  1. Split the string by each occurrence of (. (components(separatedBy:)) `,1),2)refined,3), ,refined in parentheses),RX)`
  2. For every component which is empty,
    1. Remove it. 1), 2)refined, 3), refined in parentheses), RX)
    2. If it wasn’t the first, add a ( to the following component. 1), 2)refined, 3), (refined in parentheses), RX)
    3. If it wasn’t the first, merge the components before and after. 1), 2)refined, 3)(refined in parentheses), RX)
  3. For every component, find the first ) and split the string there. 1 2, refined, 3, (refined in parentheses), RX,
  4. Clean out an new empty components. 1, 2, refined, 3, (refined in parentheses), RX

After step 4, you have the same list of strings that version 2.4 expresses directly as a list. From there, simply interpret any that are special and leave the rest as “define‐your‐own” genres in the wording of the version 2.4 specification.

Did I at least handle the regex for the parens correctly?

That kind of regular expression tends to be as nuanced as it is illegible. I stopped using them altogether.

The one you have seems to find the first pair of parentheses well enough, but then the method returns without handling the trailing refinement or any of the following pairs.

NCrusher74 commented 4 years ago

lol figures. And here I thought I'd done something right.

While I was waiting to hear back from you on that question, I started work on the ImageFrame and...yeahhh.

So, here's how the spec lays them out:

(version 2.2)

Text encoding $xx Image format $xx xx xx Picture type $xx Description $00 (00) Picture data

(version 2.3-2.4)

Text encoding $xx MIME type $00 Picture type $xx Description $00 (00) Picture data

Most of this is pretty basic. I have I can get the text encoding, no problem. I can parse the picture type byte, and I've got an enum of the recognized values for that. Same with the description.

But the MIME type part for versions 2.3 and 2.4 is throwing me, mostly because the way ID3TagEditor handles it isn't making sense to me.

I wrote this, based on how upstream handles it in ID3AttachedPictureFrameContentParsingOperation:

    private let jpegMagicNumber: Data = Data([0xFF, 0xD8, 0xFF, 0xE0])
    private let pngMagicNumber: Data =  Data([0x89, 0x50, 0x4E, 0x47])

    private func extractAndInterpretMagicNumber(data: inout Data.SubSequence) -> String? {
        let magicNumberBytes = data.extractFirst(4)
        if magicNumberBytes == jpegMagicNumber { return "jpg" }
        else if magicNumberBytes == pngMagicNumber { return "png" }
        else { return nil }
    }

(I double checked the magic number stuff to make sure I understood what I was dealing with.)

But then I looked at the spec and noticed, for version 2.2, the picture format should only be 3 bytes, and the magic number is 4. And also, now having looked at the spec, I'm not sure that magic number comes from the first four bytes (after the encoding byte) after all.

In ID3AttachedPictureFrameConfiguration, upstream has this:

    private let commonFrameHeaderMimeTypeHeader: [ID3PictureFormat : [UInt8]] = [
        .Png : [0x00] + [UInt8]("image/png".utf8) + [0x00, 0x03, 0x00],
        .Jpeg : [0x00] + [UInt8]("image/jpeg".utf8) + [0x00, 0x03, 0x00]
    ]
    private var frameHeaderMimeTypeHeader: [ID3Version : [ID3PictureFormat : [UInt8]]] = [
        .version2 : [
            .Png : [0x00] + [UInt8]("PNG".utf8) + [0x03, 0x00],
            .Jpeg : [0x00] + [UInt8]("JPG".utf8) + [0x03, 0x00]
        ]
    ]

I can use the magic number to decide which of those [UInt8] to go with, but version 2 doesn't even have a MIME string? I'm a little lost with what I'm supposed to be doing with this.

SDGGiesbrecht commented 4 years ago

I’m not sure I follow it either. I suggest getting the basic StringFrame and the UnknownFrame working first, and then working on the global scaffolding that reads and writes entire tags. That way you can direct everything else into UnknownFrame to start with, so that the rest of the tag loads fine (but meaninglessly). Then as you try to figure out each new frame type (starting with StringFrame), you can actually test and experiment with it as you go. You’ll be able to do that by creating the frame in another application and then loading it with your own code, printing it along the way to see what’s actually there and what’s happening as you process it.

NCrusher74 commented 4 years ago

At this point, I have everything except the most complex frames worked out. I'll have GenreFrame done soon, which leaves just ImageFrame, DateFrame (because of course the data handling between versions is weird depending on which version and which frame), ChapterFrame and TableOfContents frame. Everything else is done.

And honestly...writing these is a lot easier than parsing them, for the most part, so I'm considering taking a short cut for at least one of the complex types. As far as I can tell, AVFoundation has an AVMetadataKey.id3MetadataKey for every frame type except the Chapter and TableOfContents frames. The only reason I haven't gone anywhere near it is because AVFoundation won't build or write a tag for an mp3 file at pass-through settings. I'd have to re-encoded an that's not good.

However, in the process of looking something else up the other day, I noticed this example in Finding Metadata Values:

// Collection of "common" metadata
let metadata = asset.commonMetadata

// Filter metadata to find the asset's artwork
let artworkItems = AVMetadataItem.metadataItems(from: metadata, filteredByIdentifier: AVMetadataCommonIdentifierArtwork)

if let artworkItem = artworkItems.first {
    // Coerce the value to an NSData using its dataValue property
    if let imageData = artworkItem.dataValue {
        let image = UIImage(data: imageData)
        // process image
    } else {
        // No image data found
    }
}

So I suspect I could probably at last get the image as Data without having to parse everything out? idk.

There is also an ObjC library for handling Chapter metadata in AVFoundation, but I'm not sure it would do me much good, seeing as how, again, AVFoundation won't write an mp3 file losslessly.

NCrusher74 commented 4 years ago

Okay, it took me way, way longer to get the parsing of the GenreFrame stuff so that it works. I worked on it for ten hours last evening, and another hour this morning.

I think my problem with handling iterating over arrays is, I don't do it often enough to retain much of what I learned the last time, so each time I have to figure it out from scratch and I end up struggling with type mismatches and stuff.

Anyway, the only way I could work out to do what you were recommending was to re-insert open and closed parens where appropriate after using them to separate the components. But parseParentheticalString(unparsedString: String) -> [String] works with the example you provided above in a playground.

I'm sure there's a better way, but this is the best I could manage.

    init(decodingContents contents: Data.SubSequence,
         version: Version,
         layout: KnownFrameLayoutIdentifier,
         flags: Data) throws {
        self.flags = GenreFrame.defaultFlags(version: version)
        self.layout = layout
        var parsing = contents
        let encoding = GenreFrame.extractEncoding(data: &parsing, version: version)
        var parsedArray: [String] = []
        if version == .v2_2 || version == .v2_3 {
            let unparsedString = GenreFrame.extractTerminatedString(data: &parsing, encoding: encoding)
            parsedArray = parseParentheticalString(unparsedString: unparsedString)
        } else {
            while !parsing.isEmpty,
                let next = parsing.extractPrefixAsStringUntilNullTermination(encoding) {
                    parsedArray.append(next)
            }
            for component in parsedArray {
                var genre: GenreType = .None
                if component == "CR" {
                    genre = .Cover
                } else if component == "RX" {
                    genre = .Remix
                } else if let genreInt = Int(component),
                    let validGenre = GenreType(rawValue: genreInt) {
                    genre = validGenre
                } else {
                    self.descriptionString = component
                }
                self.genreType = genre
            }
        }
    }

    func parseParentheticalString(unparsedString: String) -> [String] {
        var stringComponents = unparsedString.components(separatedBy: "(")
        for (index, value) in stringComponents.enumerated() {
            if index != 0 && value == "" {
                let rangeToReplace = (index - 1)...(index + 1)
                stringComponents[index + 1].insert("(", at: stringComponents[index + 1].startIndex)
                let componentsToJoin = [stringComponents[index - 1], stringComponents[index + 1]]
                let joinedComponents = [componentsToJoin.joined()]
                stringComponents.replaceSubrange(rangeToReplace, with: joinedComponents)
                stringComponents.removeAll(where: {$0 == ""})
            }
        }
        var refinedComponents: [String] = []
        for component in stringComponents {
            if component.contains(")") {
                var separatedComponents = component.components(separatedBy: ")")
                separatedComponents.removeAll(where: {$0 == ""})
                for (index, value) in separatedComponents.enumerated() {
                    if value.contains("(") {
                        var valueToChange = value
                        valueToChange.append(")")
                        separatedComponents.remove(at: index)
                        separatedComponents.insert(valueToChange, at: index)
                    }
                }
                refinedComponents.append(contentsOf: separatedComponents)
            }
        }
        return refinedComponents
    }
NCrusher74 commented 4 years ago

I’m not sure I follow it either. I suggest getting the basic StringFrame and the UnknownFrame working first, and then working on the global scaffolding that reads and writes entire tags. That way you can direct everything else into UnknownFrame to start with, so that the rest of the tag loads fine (but meaninglessly).

I admit, I've sort of been avoiding UnknownFrame up to this point because I have no idea how to go about creating it or what it should do.

I imaging there will be two different situations when encountering an unknown frame. Either it will be a valid frame from the ID3 spec that we're not handling, or it will be a non-spec frame that will probably need to be handled as a UserTextFrame.

In the first case, the data pretty much needs to be passed through and not touched, right? And in the second, we take a stab at parsing it like a UserTextFrame?

Or am I overthinking this?

SDGGiesbrecht commented 4 years ago

I'm sure there's a better way, but this is the best I could manage.

Actually that is very good over all. There might be one or two ways of abbreviating or optimizing, but nothing sufficiently better to actually be worth the effort of changing.

But there are two things that might be typos, though they have nothing to do with the iteration.

self.flags = GenreFrame.defaultFlags(version: version)

Shouldn’t that be set to the flags just passed as in a parameter?

if component.contains(")") {

That if statement has no else branch. I think such a situation would indicate corruption. But what do you want to do then? Right now it would be dropped. i.e. a tag with "(6" would be read in as [] and then written back as "".


In the first case, the data pretty much needs to be passed through and not touched, right?

That is exactly what it will do in all cases. It should really be the easiest frame to make, since its content parsing is just a matter of storing the data it was given, and its content writing is just a matter of passing back that same data.

NCrusher74 commented 4 years ago

I'm sure there's a better way, but this is the best I could manage.

Actually that is very good over all. There might be one or two ways of abbreviating or optimizing, but nothing sufficiently better to actually be worth the effort of changing.

YAY!

But there are two things that might be typos, though they have nothing to do with the iteration.

self.flags = GenreFrame.defaultFlags(version: version)

Shouldn’t that be set to the flags just passed as in a parameter?

I... don't know? I'm so lost on the flags thing. Right now I have this is FrameProtocol

    internal static func defaultFlags(version: Version) -> Data {
        var flagBytes: [UInt8] = []
        switch version {
            case .v2_2: flagBytes = []
            case .v2_3, .v2_4: flagBytes = [0x00, 0x00]
        }
        return Data(flagBytes)
    }

and I've been using it in the various frame types, but that was just me guessing at how to handle it. I know we will probably be writing them as those zero/empty flags, but for parsing, we probably just want to pass through what is already there? I guess?

That is exactly what it will do in all cases. It should really be the easiest frame to make, since its content parsing is just a matter of storing the data it was given, and its content writing is just a matter of passing back that same data.

Okay I will work on it. I've definitely been slow on the uptake this week. It took me until today to figure out how to access KnownFrameLayoutIdentifier by way of FrameLayoutIdentifier.

NCrusher74 commented 4 years ago

I think my issue is less not knowing what to do about the flags and more not knowing how to write an initializer where the variable being initialized isn't either part of the parameter or calculated using part of the parameter.

For instance, my initializer here:

    public init(contentString: String) {
        self.contentString = contentString
    }

The compiler wants me to initialize flags and layout in this initializer. But I don't want to add any parameters besides those the user needs to satisfy in order for the frame to write its content properly.

So how do I initialize flags and layout here? self.flags = flags obviously won't work, because it's initializing a variable to itself.

SDGGiesbrecht commented 4 years ago

So how do I initialize flags and layout here?

self.flags = Wherever.defaultFlags()

For the layout one, you have two options:

  1. Add a parameter, clients create them like this StringFrame(identifier: .author, content: "Someone") or StringFrame(identifier: .title, content: "Something"). This strategy is the least effort on your part, but risks them trying to create something invalid: StringFrame(identifier: .movement, content: "Something") (which should have been an IntegerFrame).

  2. Make the initializer from option 1 private, then for every valid identifier, create a dedicated initializer that passes through to it:

    init(author: String) {
      self.init(identifier: .author, content: author)
    }
NCrusher74 commented 4 years ago

Gah! I just messed up and accidentally edited your comment instead of replying to it. GitHub seriously shouldn't allow that.

NCrusher74 commented 4 years ago

I think this is the one I'll actually end up going with, because part of the point of this is to try to make writing the frames a lot more intuitive. So yeah, the fewer parameters the user ends up having to satisfy to create a tag, the better.

I was thinking a moment ago that part of my problem is that I keep getting a little turned around in my head as to what flows from what. Especially since there are a few points at which we've been using identifier to describe both the 3-4 byte string code for a particular frame and to describe the FrameLayoutIdentifier type. I've been trying to differentiate them by referring to FrameLayoutIdentifier as layout but I messed up in a couple places.

This is mostly me writing this all down to get is clear in my head so I can see how it's going to flow, but I'm also running it by you to make sure I'm not messing anything up.

Reading:

1) Mp3File is initialized with the URL of a file and its read function passes the data on to Tag 2) Tag reads the data from the file and calls TagValidator to confirm that the file is valid and has valid tag data. If not, it throws an error 3) Tag then begins to use extract/dropFirst(_:) methods to pull out pieces of the file data and handle them.) 4) Tag calls TagProperties to extract the tag size and version data 5) Tag extracts the identifier string and passes the data off to Frame 6) Frame uses the identifier string to decide which layout the data should be passed on to. 6a) If the identifier string is not contained within KnownFrameLayoutIdentifier the frame gets passed off to UnknownFrame which will pass-through the frame data unchanged. 6b) If the identifier string is one of the specialized frames, the data gets passed off to the appropriate frame type. Otherwise, it gets passed off to StringFrame 7) The frame types parse the data in conformance with the FrameProtocol requirements and...???

Question: Where does the [frameKey: frame] part of the Tag initializer come into this? FrameKey is a convenience thing to enable writing multiple versions of the same Frame where allowed. It doesn't really have anything to do with reading a frame?

Writing: 1) User selects a FrameKey, which assigns a specific kind of Frame and requires the user to satisfy its particular parameters. When the tag is built, each FrameKey must be unique, but the uniqueness may be determined by factors such a language or description parameters, rather than layout. 2) Something (FrameProtocol?) adds header data to the frame and passes it to Tag 3) Tag compiles the [FrameKey: Frame] dictionary into an array and adds appropriate header data. 3a) Any passed-through unknown tags, and any non-empty existing tags which haven't been edited, get added to the [FrameKey: Frame] dictionary as-is. 4) The tag is written when Mp3File.write is called.

SDGGiesbrecht commented 4 years ago

Where does the [frameKey: frame] part of the Tag initializer come into this? FrameKey is a convenience thing to enable writing multiple versions of the same Frame where allowed. It doesn't really have anything to do with reading a frame?

The layout thing is used in order to figure out what type of frame to turn the data into. Once the frame is constructed, the frame gets to decide what it’s own FrameKey is. The FrameKey is how the frames are indexed for easy retrieval, and to know whether a new frame is an addition or an overwrite. The dictionary [FrameKey: Frame] is how the Tag stores the frames while the client is manipulating them in code. It should be a property of the Tag. (It should be read‐only. Writing should be done with a separate method, func insert(frame: Frame) in which you ask the frame for its key and insert it into the dictionary. This is so that the user cannot put a frame in the dictionary with a mismatched key.)

  1. User selects a FrameKey, which assigns a specific kind of Frame and requires the user to satisfy its particular parameters. When the tag is built, each FrameKey must be unique, but the uniqueness may be determined by factors such a language or description parameters, rather than layout.

Kind of. The user assembles a frame, and the frame decides what its key is. Then the user adds it to the Tag.

Other than that you’ve gotten the hang of it.

NCrusher74 commented 4 years ago
2. Make the initializer from option 1 private, then for every valid identifier, create a dedicated initializer that passes through to it:
   ```swift
   init(author: String) {
     self.init(identifier: .author, content: author)
   }
   ```

Should I make one of these for each KnownFrameLayoutIdentifier, or for each FrameKey? (meaning there might be one for remixer as well as one for arranger)

NCrusher74 commented 4 years ago

Kind of. The user assembles a frame, and the frame decides what its key is. Then the user adds it to the Tag.

Hmm, okay, I had imagined it would be the other way around. I don't think most users will actually care what frame they're using, they just want the frame that is appropriate space for their metadata.

For instance, with audiobooks, author usually maps to the artist frame, and narrator goes to the composer frame (neither of which would be true if I'd been involved in deciding how audiobook metadata would be written, FYI. It offends my sense of order in the world that author doesn't go to composer or even lyricist--the ID3 code for which is actually TEXT-- and narrator doesn't get mapped to a performer frame.)

But I digress...

If I'm an audiobook listener, I'm going to be looking for an option for author rather than an option for artist, so I'd choose a FrameKey called author (which I may or may not realize is mapped to the artist frame) before I'd chose the artist frame. Which is why most music tagging apps--and even TagLib--refer to the publisher frame as label and so forth.

NCrusher74 commented 4 years ago

Is this what I should be doing for the encodeContents function for each particular frame type?

    internal func encodeContents(version: Version) throws -> Data {
        let contents = self.contentString
        return contents.encoded(withNullTermination: false)
    }
SDGGiesbrecht commented 4 years ago

Should I make one of these for each KnownFrameLayoutIdentifier, or for each FrameKey?

Good idea. FrameKey might be better. (Although when it involves something like a layout + an arbitrary language, you probably just want the language as a parameter, instead of hundreds of initializers.)

If I'm an audiobook listener, I'm going to be looking for an option for author rather than an option for artist, so I'd choose a FrameKey called author (which I may or may not realize is mapped to the artist frame) before I'd chose the artist frame.

I’m not sure we’re talking about the same thing. I mean the client library does this:

let newFrame = StringFrame(author: "Someone")
let tag = Tag()
tag.add(newFrame)
assert(tag.frames[.author] == newFrame)

If you want to help them find the right frame with autocomplete, you can create a master list hanging off a certain namespace. This pattern is often called using “factory methods”.

public enum FrameFactory {
  static func makeAuthorFrame(_ author: String) -> StringFrame {
    return StringFrame(author: author)
  }
  static func makeMovementFrame(_ movement: Int) -> IntegerFrame {
    return IntegerFrame(movement: movement)
  }
}

That way the user doesn’t have to predict the right layout type to look for the initializer they want. They can simply type FrameFactory.make and see the list of what’s available. It looks like this when they use it:

let newFrame = StringFactory.makeAuthorFrame("Someone")
let tag = Tag()
tag.add(newFrame)
assert(tag.frames[.author] == newFrame)

Either way, the important thing is that under the hood, your add method is doing this:

mutating func add(_ frame: Frame) {
  // This ensures the right key is used for the frame,
  // by letting the frame itself tell us its key:
  self.frames[frame.frameKey] = frame
}

What you don’t want is for frames to be settable by library clients. Then users would be able to do this:

let newFrame = StringFactory.makeAuthorFrame("Someone")
let tag = Tag()
tag.frames[.movement] = newFrame // ← Bad! Wrong key!
assert(tag.frames[.movement] == newFrame)
SDGGiesbrecht commented 4 years ago

Is this what I should be doing for the encodeContents function for each particular frame type?

Yes. That looks like the correct implementation for the StringFrame. It will be the only one that does that though; the others have different things to encode.

NCrusher74 commented 4 years ago

What is a good default value for optionals being converted to Data? For instance, in the LocalizedFrame, the description string is optional. It may be empty. What would I put after the ?? here?

        let encodedDescriptionString = descriptionString?.encoded(withNullTermination: true) ??

Would I first convert it to a null byte and then change it to data? or just do an "if

Is this what I should be doing for the encodeContents function for each particular frame type?

Yes. That looks like the correct implementation for the StringFrame. It will be the only one that does that though; the others have different things to encode.

Yep, I'm working on that as we speak

Since sometimes one of the content pieces are optional, and usually the spec calls for that field just to be a null byte if it's empty, I'm using this sort of thing:

        guard let encodedDescriptionString = descriptionString?.encoded(withNullTermination: true) else {
            let emptyStringBytes: UInt8 = 0x00
            return emptyStringBytes.data
        }

In other news, I discovered last night that the MediaType frame actually needs to be handled like the Genre frame, which a bunch of preset media types (either in parentheses or null-terminated, depending on version) and a freeform refinement option after.

There's an added layer of complexity, however.

The media type presets look like this:

    CD     CD
      /A    Analogue transfer from media
      /DD   DDD
      /AD   ADD
      /AA   AAD

    TT     Turntable records
      /33    33.33 rpm
      /45    45 rpm
      /71    71.29 rpm
      /76    76.59 rpm
      /78    78.26 rpm
      /80    80 rpm

Where all the options preceeded by a / can be tacked onto the thing they refer to. So (CD/A) would be "Analog transfer from CD media".

Making them into enums is proving to be clumsy, because something like "71.29 rpm" makes for a really awkward case declaration, and I'm still working on how to code the preset-refinements so that the preset refinement choices differ based on the preset media type.

It's actually sort of like the way we handled Genre -> Subgenre in AudiobookLiberator, minus the need for a combo box. But the code I wrote for parsing preset and/or refined genre strings is based on working with enums rather than arrays, and I know I can get an array from the enum using CaseIterable but I'm still working on the exact syntax to make it all work.

NCrusher74 commented 4 years ago
let newFrame = StringFrame(author: "Someone")
let tag = Tag()
tag.add(newFrame)
assert(tag.frames[.author] == newFrame)

Ah ok, in my head I'd had the idea that the client end would look more like this:

let authorFrame = framesKeys.author(author: "Someone")
let tag = Tag()
tag.add(authorFrame)

And then behind the scenes, the code would know that frameKeys.author actually means KnownFrameLayoutIdentifier.artist and KnownFrameLayoutIdentifier.artist is a StringFrame, so it would do something vaguely like this:

framesKeys.author(author: "Someone") = StringFrame(layout: .known(KnownFrameLayoutIdentifier.artist), contentString: "Someone")

And that way the client would never have to be bothered with whether or not they were using a StringFrame or IntegerFrame or whatever (and they couldn't try to add the wrong content to the wrong frame, which is something I've done a few times using both ID3TagEditor and MP42Foundation because I was cutting and pasting and forgot to change the frame or data time.) They would just use whatever frameKey was appropriate to their needs and the code would take it from there.

SDGGiesbrecht commented 4 years ago

[...] usually the spec calls for that field just to be a null byte if it's empty, I'm using this sort of thing:

[...].encoded(withNullTermination: true)[...] [...]let emptyStringBytes: UInt8 = 0x00[...]

That sounds like it’s not really optional in the specification, but rather that there is always a null‐terminated string there, and it’s just allowed to be a zero‐length null terminated string. If so, then the property ought not to be optional in the first place, and it needs no special handling.

NCrusher74 commented 4 years ago

[...] usually the spec calls for that field just to be a null byte if it's empty, I'm using this sort of thing: [...].encoded(withNullTermination: true)[...] [...]let emptyStringBytes: UInt8 = 0x00[...]

That sounds like it’s not really optional in the specification, but rather that there is always a null‐terminated string there, and it’s just allowed to be a zero‐length null terminated string. If so, then the property ought not to be optional in the first place, and it needs no special handling.

Yeah, the spec is sort of weird on that. It doesn't require there to be content, per se. So it's optional on the user end, but not on the spec end.

The 'Content descriptor' is a terminated string. If no descriptor is entered, 'Content descriptor' is $00 (00) only.

Maybe give it a default value of an empty string rather than making it optional? But an empty string ("") isn't [0x00] it's [0x22, 0x22] right?

SDGGiesbrecht commented 4 years ago

The quotation marks aren’t part of the string.

"abc".encoded(withNullTermination: true) // [0x61, 0x62, 0x63, 0x00]
"".encoded(withNullTermination: true) // [0x00]
SDGGiesbrecht commented 4 years ago

Also, when you parsed it in, the method would have found the first 0x00, and returned up to that point as a string, giving you "". So I don’t think you should ever really have nil to begin with. (Your initializer that the client will use might have its parameter be optional though: description: String = "" That would allow the client to not bother specifying it when they don’t care.)

NCrusher74 commented 4 years ago

Okay, will do.

Does the same apply to integers? I know the byte for 0 is 0x30.

On the PartOfTotal frame (track and disc position) the total part is truly optional. The spec says:

The value MAY be extended with a "/" character and a numeric string containing the total number of parts in the set. E.g. "1/2".

What I have so far is:

        let partData = self.part.data
        guard let totalData = self.total?.data else {
            let emptyValue = 0
            return emptyValue.data
        }
        return partData + totalData

And I just realized I'm missing the potential forward-slash.

Maybe I need to just convert it all to a String and then encode it?

SDGGiesbrecht commented 4 years ago

Maybe I need to just convert it all to a String and then encode it?

Sounds like a good idea.

NCrusher74 commented 4 years ago

Yeah, that's much easier:

        if self.total == nil {
            let partOfTotalString = String(self.part)
            return partOfTotalString.encoded(withNullTermination: false)
        } else {
            let partOfTotalString = "\(self.part)/\(self.total ?? 0)"
            return partOfTotalString.encoded(withNullTermination: false)
        }

Now things get really fun! I'm up to the CreditListFrame with it's entries: [(role: String, person: String)] content!

So far I've got:

        for entry in self.entries {
            let entry = (entry.role.encoded(withNullTermination: true), entry.person.encoded(withNullTermination: true))

        }

I'm trying to figure out how to get from that to the entire thing as Data.

SDGGiesbrecht commented 4 years ago

append(contentsOf:)

NCrusher74 commented 4 years ago

like this?

    func encodeContents(version: Version) throws -> Data {
        var entriesAsData: [Data.Element] = []
        for entry in self.entries {
            entriesAsData.append(contentsOf: entry.role.encoded(withNullTermination: true))
            entriesAsData.append(contentsOf: entry.person.encoded(withNullTermination: true))
        }
        return Data(entriesAsData)
    }
SDGGiesbrecht commented 4 years ago

Almost. You can just use Data directly instead of [Data.Element].

SDGGiesbrecht commented 4 years ago

...but what you have does work.

NCrusher74 commented 4 years ago

Got it. Thank you!

NCrusher74 commented 4 years ago

Question:

If I have this:

enum TVRefinement: String {
    case PAL = "PAL"
    case NTSC = "NTSC"
    case SECAM = "SECAM"

    static var code: String {
        switch self {
            case .PAL: return "/PAL"
            case .NTSC: return "/NTSC"
            case .SECAM:  return "/SECAM"
        }
    }
}

Why would I be getting these errors on each of the cases in the code variable? "Cannot infer contextual base in reference to member 'PAL'" "Pattern cannot match values of type 'TVRefinement.Type'"

I don't understand what those errors mean.

NCrusher74 commented 4 years ago

In order to convert the GenreType enum to a String to convert it to Data I have to make it conform to LosslessStringConvertible.

Did I do this right?

public enum GenreType: Int, CaseIterable, LosslessStringConvertible  {
    public init?(_ description: String) {
        self.init(description)
    }

    public var description: String {
        return String(self.rawValue)
    }

Edit: Hmm actually I'm wrong. It's saying Int? should conform, not GenreType.

For context:

    func encodeContents(version: Version) throws -> Data {
        switch version {
            case .v2_2, .v2_3:
                let genreTypeAsString = String(self.genreType?.rawValue) ?? ""
                let genreTypeStringAsData = genreTypeAsString.encoded(withNullTermination: false)
                let refinementStringAsData = refinementString?.encoded(withNullTermination: false)
            case .v2_4:
                let genreTypeAsString = String(self.genreType?.rawValue) ?? ""
                let genreTypeStringAsData = genreTypeAsString.encoded(withNullTermination: true)
                let refinementStringAsData = refinementString?.encoded(withNullTermination: true)

        }
    }
SDGGiesbrecht commented 4 years ago

Why would I be getting these errors on each of the cases in the code variable?

You declared the property as static. The static archetype’s self is the thing that makes instances, not an instance itself. When you see the compiler talking about .Type, it’s referring to that abstract archetype.

In order to convert the GenreType enum to a String to convert it to Data I have to make it conform to LosslessStringConvertible.

Did I do this right?

I’m too lazy to look up init(_:) or rawValue to see precisely what their implementations are, but as long as they are doing what their names imply, then what you have is correct.

NCrusher74 commented 4 years ago

Is there a way to access rawValue on a CaseIterable enum? Like, if I wanted an array of the rawValue for AllCases, is there a way to get that? Because I can't find the right syntax for it, I always end up with type mismatch errors.

SDGGiesbrecht commented 4 years ago

If the enumeration is both CaseIterable and RawRepresentable (either explicitly or implicitly by giving it Int or String underlying values), then you can get the list of raw values with map(_:):

let allRawValues: [MyEnumeration.RawValue]
  = MyEnumeration.allCases.map { $0.rawValue }
NCrusher74 commented 4 years ago

I think I'm gonna make that an extension of enum (edit: or CaseIterable, I guess?) because I've been banging my head against trying to get the raw value arrays for enums for weeks.

Thank you.

NCrusher74 commented 4 years ago

formatting this MediaType stuff is giving me a headache. I've tried it about a dozen different ways, using enums, arrays, dictionaries, whatever, and I can't quite get it to work.

Parsing the stuff will be pretty basic. I use the same method I created for parsing the genretype, and then once I have my array of preset media type codes, preset media type refinement codes, and freeform refinements, all I need to do is look for a / at the beginning, and if I find one, it will be the preset refinement option. If it doesn't have /, then I see if it matches the array of media type codes, and if it does, then it's a preset media type code. If neither of those is true, it's a freeform refinement.

So that's not a problem.

The problem is formatting the codes and their translations so that they're usable. I've tried every collection type I can think of--arrays, dictionaries, tuples, enums with various types of rawValues and computed properties--but I can't quite get it right.

For each MediaType, I have two strings. One is the actual media type as a human-readable string, and the other is the 2-3 letter code.

Then, for MOST of those media types, I have one more more possible preset refinements, again both with a human-readable string describing it, and a code (beginning with /)

Sometimes the refinement (particularly "Analog transfer from media" aka "/A") is used by multiple different media types. And sometimes the code for the refinement means something different depending on the Media Type (for instance, "/76" may mean "76.59 rpm" for "Turn Table record" or it may mean "76 cm/s" for "Reel (tape)".

So, this would be the information for Turntable records:

    TT     Turntable records
      /33    33.33 rpm
      /45    45 rpm
      /71    71.29 rpm
      /76    76.59 rpm
      /78    78.26 rpm
      /80    80 rpm

When parsing a frame, I want to look for the part code (either "TT" or one of the "/" refinements) .

But when presenting a user with the options they can select, I probably want to give them the full string description for readability, and then I want to customize the list of refinements available based on the Media Type.

I've formatted this data so many different ways, I've memorized all the obscure refinements. I've tried creating a MediaType enum with a refinement computed property, but sometimes that refinement computed property is an array (of either the name for the refinements, or the code) and sometimes it's not. I've tried creating [name: String, code: String] dictionaries for all the types and refinements. Each time, I end up bumping up against an error I can't resolve.

For example, I tried:

extension CaseIterable where Self: RawRepresentable {

    static var rawValues: [RawValue] {
        return allCases.map { $0.rawValue }
    }
}

enum MediaTypeRefinement: String, CaseIterable {
    case analogTransfer = "Analog transfer from media"
    case thirtyThreeRPM = "33.33 rpm"
    case fortyFiveRPM = "45 rpm"

    static var code: String {
        switch self {
            case .analogTransfer:
            return "/A"
            case .thirtyThreeRPM:
            return "/33"
            case .fortyFiveRPM:
            return "/45"
        }
    }
}

enum MediaType: String, CaseIterable {

    case otherDigitalMedia = "Other digital media"
    case turntableRecords = "Turntable records"
    // etc

    var code: String {
        switch self {
            case .otherDigitalMedia:
            return "DIG"
            case .turntableRecords:
            return "TT"
        }
    }

    var refinementNames: [String] {
        switch self {
            case .otherDigitalMedia:
                return [MediaTypeRefinement.analogTransfer.rawValue]
            case .turntableRecords:
                return [MediaTypeRefinement.thirtyThreeRPM.rawValue,
                        MediaTypeRefinement.fortyFiveRPM.rawValue]
        }
    }

    var refinementCodes: [String] {
        switch self {
            case .otherDigitalMedia:
                return [MediaTypeRefinement.code]
            case .turntableRecords:
            return [MediaTypeRefinement.code]
        }
    }
}

But this gives me the error I was asking about earlier because of the static var declaration. If I don't make those static, however, the var refinementCodes: [String] won't work.

This is my latest attempt, but I can't quite figure out how to make it work either:

struct MediaTypeInfo {
    public let mediaTypeName: String
    public let mediaTypeCode: String
    public let refinementNames: [String]
    public let refinementCodes: [String]
}

struct MediaType {
    static let types: [MediaTypeInfo] = [
        MediaTypeInfo(mediaTypeName: "Other Digital Media",
                      mediaTypeCode: "DIG",
                      refinementNames: ["Analog transfer from media"],
                      refinementCodes: ["/A"]),
        MediaTypeInfo(mediaTypeName: "Other Analog Media",
                      mediaTypeCode: "ANA",
                      refinementNames: ["Wax cylinder", "8track tape cassette"],
                      refinementCodes: ["/WC", "/8CA"]),
// etc etc 

I feel stupid because this is pretty basic stuff that I should be able to do at this point.

SDGGiesbrecht commented 4 years ago

But this gives me the error I was asking about earlier because of the static var declaration. If I don't make those static, however, the var refinementCodes: [String] won't work.

It doesn’t make any sense as static (to me at least). I think the real error is in refinementCodes, but I’m not really sure what that property is supposed to be. I’m just stabbing in the dark, but is this what you are trying to accomplish:

var refinements: [MediaTypeRefinement] {
  switch self {
  case .otherDigitalMedia:
    return [.analogTransfer]
  case .turntableRecords:
    return [.thirtyThreeRPM, .fortyFiveRPM]
  }
}

var refinementNames: [String] {
  return refinements.map { $0.rawValue }
}

var refinementCodes: [String] {
  return refinements.map { $0.code }
}
NCrusher74 commented 4 years ago

Yes, except that refinements.map { $0.rawValue } would return ALL the rawValues (or codes right?) but case turntable has a completely different set of refinements than case dat or case radio or whatever.

That's what keeps tripping me up. I can come up with a half-dozen different ways to write the code to make the return of the refinement properties differ depending on the MediaType case, but by the time I'm done, I can't seem to query it in a way that I can use it either as a comparison basis for parsing an incoming frame (which is why I need the refinementCode property), or a source of selection options for autocomplete for a user to choose from when writing a frame (which is why I'd need the refinementName property.)

So put it in partially-correct pseudocode, what I want to accomplish is this:

// after parsing everything the same way I do with the `GenreTypes` for an incoming frame
            for component in parsedArray {
                var media: MediaType
                if component.first == "/" {
                   self.mediaTypeRefinement = component 
                } else if mediaTypeNames.rawValues.contains(component) {
                    self.mediaType = component
                } else self.refinementString = component
            }

That's the easy part, and I've got that, like, 95% in place. I just need to figure out how to tweak the code so that I can make GenreFrame and MediaTypeFrame into a PresetOptionsFrame instead, which would require making the GenreType enum and the MediaType enum... conform to a protocol, maybe? or make them both options under a PresetOptions enum sort of the way KnownFrameLayoutIdentifier is the case known option in FrameLayoutIdentifier.

Where it gets trickier is writing the frame.

struct PresetOptionsFrame: FrameProtocol {
     private var presetType: String // GenreType Int rawValue or MediaType code string  that gets written to file
     private var presetRefinement: String // Media Type Refinement code string that gets written to file

     public var: genreOrMediaType: String // genre or media NAME that the user can select using autocomplete (likely this will be GenreType.rawValue or MediaType.rawValue)     
     public var: mediaTypeRefinement: String // the NAME of the media type, the autocomplete options for which will change depending on what Media Type was chosen in the previous variable
     var refinementString: String? // reform description/refinement

// ... code to take the NAME that the user inputs and derive the CODE that will get written to the file

I think I can write that code, but I just need to have my MediaType and MediaTypeRefinements formatted so that I can access them to accomplish it, and I'm not figuring out the right way to go about it.

NCrusher74 commented 4 years ago

Actually, what is tripping me up is getting all my types to agree. Because I've got MediaType and GenreType one of which is RawRepresentable as a String and the other of which is RawRepresentable as an Int. And I'm accessing both of those enums by way of a PresetOptions enum which has cases mediaType and genreType and then I'm trying to convert what I'm actually getting from all those types to a String and it's sort of a mess.

I have a playground I'm using to model it, but apparently you can't make a Git repo from a playground?

extension CaseIterable where Self: RawRepresentable {

    static var rawValues: [RawValue] {
        return allCases.map { $0.rawValue }
    }
}

enum PresetOptions {
    case mediaType(MediaType)
    case genreType(GenreType)
}

public enum GenreType: Int, CaseIterable  {

    case Blues = 0
    case ClassicRock = 1
    case Country = 2
    case Dance = 3
// etc etc
    case None = 255
    case Remix = 256
    case Cover = 257

    static var RX: GenreType { return .Remix }
    static var CR: GenreType { return .Cover }

    var stringValue: String {
        return String(describing: self.rawValue)
    }
}

enum MediaType: String, CaseIterable {
    case otherDigital = "Other digital media"
    case otherAnalog = "Other analogue media"
    case compactDisc = "Compact Disc"
    case laserdisc = "Laserdisc"
    case minidisc = "Minidisc"
    case dcc = "DCC"
    case dat = "DAT"
    case turntableRecord = "Turntable records"
    case dvd = "DVD"
    case television = "Television"
    case video = "Video"
    case radio = "Radio"
    case telephone = "Telephone"
    case normalCassette = "Normal Cassette"
    case reel = "Reel"
    case none = ""

    var code: String {
        switch self {
            case .otherDigital: return "DIG"
            case .otherAnalog: return "ANA"
            case .compactDisc: return "CD"
            case .laserdisc: return "LD"
            case .minidisc: return "MD"
            case .dcc: return "DCC"
            case .dat: return "DAT"
            case .turntableRecord: return "TT"
            case .dvd: return "DVD"
            case .television: return "TV"
            case .video: return "VID"
            case .radio: return "RAD"
            case .telephone: return "TEL"
            case .normalCassette: return "MC"
            case .reel: return "REE"
            case .none: return ""
        }
    }

    var refinementNames: [String] {
        switch self {
            case .otherDigital: return [MediaTypeRefinements.analogTransfer.rawValue]
            case .otherAnalog: return [MediaTypeRefinements.waxCylinder.rawValue,
                                       MediaTypeRefinements.eightTrack.rawValue]
            case .compactDisc: return [MediaTypeRefinements.analogTransfer.rawValue,
                                       MediaTypeRefinements.ddd.rawValue,
                                       MediaTypeRefinements.add.rawValue,
                                       MediaTypeRefinements.aad.rawValue]
            case .laserdisc: return []
            case .minidisc: return [MediaTypeRefinements.analogTransfer.rawValue]
            case .dcc: return [MediaTypeRefinements.analogTransfer.rawValue]
            case .dat: return [MediaTypeRefinements.analogTransfer.rawValue,
                               MediaTypeRefinements.standardDAT.rawValue,
                               MediaTypeRefinements.modeTwoDAT.rawValue,
                               MediaTypeRefinements.modeThreeDAT.rawValue,
                               MediaTypeRefinements.modeFourDAT.rawValue,
                               MediaTypeRefinements.modeFiveDAT.rawValue,
                               MediaTypeRefinements.modeSixDAT.rawValue]
            case .turntableRecord: return [MediaTypeRefinements.thirtyThreeRPM.rawValue,
                                           MediaTypeRefinements.fortyFiveRPM.rawValue,
                                           MediaTypeRefinements.seventyOneRPM.rawValue,
                                           MediaTypeRefinements.seventySixRPM.rawValue,
                                           MediaTypeRefinements.seventyEightRPM.rawValue,
                                           MediaTypeRefinements.eightyRPM.rawValue]
            case .dvd: return [MediaTypeRefinements.analogTransfer.rawValue]
            case .television: return [MediaTypeRefinements.pal.rawValue,
                                      MediaTypeRefinements.ntsc.rawValue,
                                      MediaTypeRefinements.secam.rawValue]
            case .video: return [MediaTypeRefinements.pal.rawValue,
                                 MediaTypeRefinements.ntsc.rawValue,
                                 MediaTypeRefinements.secam.rawValue,
                                 MediaTypeRefinements.vhs.rawValue,
                                 MediaTypeRefinements.svhs.rawValue,
                                 MediaTypeRefinements.betamax.rawValue]
            case .radio: return [MediaTypeRefinements.am.rawValue,
                                 MediaTypeRefinements.fm.rawValue,
                                 MediaTypeRefinements.lw.rawValue,
                                 MediaTypeRefinements.mw.rawValue]
            case .telephone: return [MediaTypeRefinements.isdn.rawValue]
            case .normalCassette: return [MediaTypeRefinements.normalSpeed.rawValue,
                                          MediaTypeRefinements.nineCMS.rawValue,
                                          MediaTypeRefinements.typeI.rawValue,
                                          MediaTypeRefinements.typeII.rawValue,
                                          MediaTypeRefinements.typeIII.rawValue,
                                          MediaTypeRefinements.typeIV.rawValue]
            case .reel: return [MediaTypeRefinements.nineCMS.rawValue,
                                MediaTypeRefinements.nineteenCMS.rawValue,
                                MediaTypeRefinements.thirtyEightCMS.rawValue,
                                MediaTypeRefinements.seventySixCMS.rawValue,
                                MediaTypeRefinements.typeI.rawValue,
                                MediaTypeRefinements.typeII.rawValue,
                                MediaTypeRefinements.typeIII.rawValue,
                                MediaTypeRefinements.typeIV.rawValue]
            case .none: return [MediaTypeRefinements.none.rawValue]
        }
    }

    var refinementCodes: [String] {
        switch self {
            case .otherDigital: return [MediaTypeRefinements.analogTransfer.code]
            case .otherAnalog: return [MediaTypeRefinements.waxCylinder.code,
                                       MediaTypeRefinements.eightTrack.code]
            case .compactDisc: return [MediaTypeRefinements.analogTransfer.code,
                                       MediaTypeRefinements.ddd.code,
                                       MediaTypeRefinements.add.code,
                                       MediaTypeRefinements.aad.code]
            case .laserdisc: return []
            case .minidisc: return [MediaTypeRefinements.analogTransfer.code]
            case .dcc: return [MediaTypeRefinements.analogTransfer.code]
            case .dat: return [MediaTypeRefinements.analogTransfer.code,
                               MediaTypeRefinements.standardDAT.code,
                               MediaTypeRefinements.modeTwoDAT.code,
                               MediaTypeRefinements.modeThreeDAT.code,
                               MediaTypeRefinements.modeFourDAT.code,
                               MediaTypeRefinements.modeFiveDAT.code,
                               MediaTypeRefinements.modeSixDAT.code]
            case .turntableRecord: return [MediaTypeRefinements.thirtyThreeRPM.code,
                                           MediaTypeRefinements.fortyFiveRPM.code,
                                           MediaTypeRefinements.seventyOneRPM.code,
                                           MediaTypeRefinements.seventySixRPM.code,
                                           MediaTypeRefinements.seventyEightRPM.code,
                                           MediaTypeRefinements.eightyRPM.code]
            case .dvd: return [MediaTypeRefinements.analogTransfer.code]
            case .television: return [MediaTypeRefinements.pal.code,
                                      MediaTypeRefinements.ntsc.code,
                                      MediaTypeRefinements.secam.code]
            case .video: return [MediaTypeRefinements.pal.code,
                                 MediaTypeRefinements.ntsc.code,
                                 MediaTypeRefinements.secam.code,
                                 MediaTypeRefinements.vhs.code,
                                 MediaTypeRefinements.svhs.code,
                                 MediaTypeRefinements.betamax.code]
            case .radio: return [MediaTypeRefinements.am.code,
                                 MediaTypeRefinements.fm.code,
                                 MediaTypeRefinements.lw.code,
                                 MediaTypeRefinements.mw.code]
            case .telephone: return [MediaTypeRefinements.isdn.code]
            case .normalCassette: return [MediaTypeRefinements.normalSpeed.code,
                                          MediaTypeRefinements.nineCMS.code,
                                          MediaTypeRefinements.typeI.code,
                                          MediaTypeRefinements.typeII.code,
                                          MediaTypeRefinements.typeIII.code,
                                          MediaTypeRefinements.typeIV.code]
            case .reel: return [MediaTypeRefinements.nineCMS.code,
                                MediaTypeRefinements.nineteenCMS.code,
                                MediaTypeRefinements.thirtyEightCMS.code,
                                MediaTypeRefinements.seventySixCMS.code,
                                MediaTypeRefinements.typeI.code,
                                MediaTypeRefinements.typeII.code,
                                MediaTypeRefinements.typeIII.code,
                                MediaTypeRefinements.typeIV.code]
            case .none: return [MediaTypeRefinements.none.code]
        }
    }
}

enum MediaTypeRefinements: String {
    case analogTransfer = "Analog transfer from media"
    case waxCylinder = "Wax cylinder"
    case eightTrack = "8-track tape cassette"
    case ddd = "DDD"
    case add = "ADD"
    case aad = "AAD"
    case thirtyThreeRPM = "33.33 rpm"
    case fortyFiveRPM = "45 rpm"
    case seventyOneRPM = "71.29 rpm"
    case seventySixRPM = "76.59 rpm"
    case seventyEightRPM = "78.26 rpm"
    case eightyRPM = "80 RPM"
    case standardDAT = "standard, 48 kHz/16 bits, linear"
    case modeTwoDAT = "mode 2, 32 kHz/16 bits, linear"
    case modeThreeDAT = "mode 3, 32 kHz/12 bits, non-linear, low speed"
    case modeFourDAT = "mode 4, 32 kHz/12 bits, 4 channels"
    case modeFiveDAT = "mode 5, 44.1 kHz/16 bits, linear"
    case modeSixDAT = "mode 6, 44.1 kHz/16 bits, 'wide track' play"
    case pal = "PAL"
    case ntsc = "NTSC"
    case secam = "SECAM"
    case vhs = "VHS"
    case svhs = "S-VHS"
    case betamax = "Betamax"
    case am = "AM"
    case fm = "FM"
    case lw = "LW"
    case mw = "MW"
    case isdn = "ISDN"
    case normalSpeed = "4.75 cm/s (normal speed for a two sided cassette)"
    case nineCMS = "9.5 cm/s"
    case nineteenCMS = "19 cm/s"
    case thirtyEightCMS = "38 cm/s"
    case seventySixCMS = "76 cm/s"
    case typeI = "Type I cassette (ferric/normal)"
    case typeII = "Type II cassette (chrome)"
    case typeIII = "Type III cassette (ferric chrome)"
    case typeIV = "Type IV cassette (metal)"
    case none = ""

    var code: String {
        switch self {
            case .analogTransfer: return "/A"
            case .waxCylinder: return "/WC"
            case .eightTrack: return "/8CA"
            case .ddd: return "/DD"
            case .add: return "/AD"
            case .aad: return "/AA"
            case .thirtyThreeRPM: return "/33"
            case .fortyFiveRPM: return "/45"
            case .seventyOneRPM: return "/71"
            case .seventySixRPM: return "/76"
            case .seventyEightRPM: return "/78"
            case .eightyRPM: return "/80"
            case .standardDAT: return "/1"
            case .modeTwoDAT: return "/2"
            case .modeThreeDAT: return "/3"
            case .modeFourDAT: return "/4"
            case .modeFiveDAT: return "/5"
            case .modeSixDAT: return "/6"
            case .pal: return "/PAL"
            case .ntsc: return "/NTSC"
            case .secam: return "/SECAM"
            case .vhs: return "/VHS"
            case .svhs: return "/SVHS"
            case .betamax: return "/BETA"
            case .am: return "/AM"
            case .fm: return "/FM"
            case .lw: return "/LW"
            case .mw: return "/MW"
            case .isdn: return "/ISDN"
            case .normalSpeed: return "/4"
            case .nineCMS: return "/9"
            case .nineteenCMS: return "/19"
            case .thirtyEightCMS: return "/38"
            case .seventySixCMS: return "/76"
            case .typeI: return "/I"
            case .typeII: return "/II"
            case .typeIII: return "/III"
            case .typeIV: return "/IV"
            case .none: return ""
        }
    }
}

struct PresetOptionsFrame {
    private var presetType: PresetOptions?
    private var presetRefinement: PresetOptions?
    private var refinementDescription: String?

    private init(presetType: PresetOptions?,
                 presetRefinement: PresetOptions?,
                 refinementDescription: String?) {
        self.presetType = presetType
        self.presetRefinement = presetRefinement
        self.refinementDescription = refinementDescription
    }

    public init(genreType: GenreType?,
                genreDescription: String?) {
        self.init(presetType: PresetOptions.genreType(
            GenreType(rawValue: genreType?.rawValue ?? 255) ?? .None),
                  presetRefinement: nil,
                  refinementDescription: genreDescription)
    }

    public init(mediaType: MediaType?,
                mediaRefinement: MediaTypeRefinements?,
                mediaTypeDescription: String?) {
        self.init(presetType: PresetOptions.mediaType(
            MediaType(rawValue: mediaType?.rawValue ?? "") ?? .none),
                  presetRefinement: PresetOptions.mediaType(
                    MediaType(rawValue: mediaType?.rawValue ?? "")?.refinementNames ?? .none),
                  refinementDescription: mediaTypeDescription)
    }
}

it's this very last bit, the public init for the two variations on the frame, that is giving me a hard time, because Swift seems to be leading me in circles with the recommended fixes.

Also, I'm doing this wrong. What this will do is write an array to the file, not present the user with an array of options to select.

NCrusher74 commented 4 years ago

I think I may be overthinking something.

If I have this:

enum PresetOption {
    case mediaType(MediaType)
    case genreType(GenreType)

    init(presetName: String) {
        if let mediaType = MediaType(rawValue: presetType) {
            self = .mediaType(mediaType)
        }
        if let genreType = GenreType(rawValue: presetType) {
            self = .genreType(genreType)
        }
    }

    var rawValue: String {
        switch self {
            case .mediaType(let mediaType): return mediaType.rawValue
            case .genreType(let genreType): return genreType.rawValue
        }
    }

    var code: String {
        switch self {
            case .mediaType(let mediaType): return mediaType.code
            case .genreType(let genreType): return genreType.code
        }
    }
}

And I'm trying to backtrack to the code from the rawValue, then all I need to do is this, right?

                if let presetName = presetType?.rawValue {
                    let presetCode = presetType?.code
            }
SDGGiesbrecht commented 4 years ago

Your giant example compiles if I simply remove ?.refinementNames from the fourth‐last line. Is that what it was supposed to do?


Your latest example would simply be this:

let presetCode = presetType?.code

Unless you meant the initializer to be optional (init?(...)) and were looking for this:

let presetCode = PresetOption(presetName: whatever)?.code
NCrusher74 commented 4 years ago

Is that what it was supposed to do?

Um... will I seem like an idiot if I say I honestly don't know? Since I posted that comment, I've done hours of work on trying to simplify the PresetOption enum so that it's not so convoluted. Of course, but I'm still just sort of guessing because I don't really understand how enum initializers work, much less how an initializer for an enum that is sort of masquerading as a protocol to unify two other enums is supposed to work. I'm using our initializer for FrameLayoutIdentifier (and the way it acts as an intermediary for KnownFrameLayoutIdentifier) as my example.

Unless you meant the initializer to be optional

I don't think I did?

Basically, all I need to be able to go from rawValue to code and back again, for GenreType, MediaType.

The PresetOptionsFrame needs to be usable by both .mediaType and .genreType frames, so I need presetType.rawValue to stand in for mediaType.rawValue OR genreType.rawValue and same with code. To simplify things, I've even made the GenreType code an integer string instead of an Int, and I'll handle that conversion when appropriate.

But I worry that I'm sort of coding myself in circles. Which I why I thought I'd check if I was overthinking it.

I've got this sort of function on each enum to handle the code-to-name conversion:

    private static let codeToNameMapping: [String: String] = {
        var mapping: [String: String] = [:]
        for genre in GenreType.allCases {
            let id = genre.code
            mapping[id] = genre.rawValue
        }
        return mapping
    }()

So basically, if the user is creating a .genre type frame, the string they supply will be the rawValue of the genreType and from that, I'll derive the code and write it to the file.

On the flip side, when I'm parsing, the file will contain the code, and I'll convert that to the rawValue for readability.

Actually, what they supply should really be an array of genre.rawValue and they can select as many as they want, but I haven't gotten to that part yet. Nor have workout out how I'm going to handle the possibility of presetType, refinementDescription pairs when sometimes what they're putting in may be a pair, and sometimes it may just be one or the other.

I'm thinking... maybe let them create the frame as though they're creating one frame for each pair (or single value) and then when constructing the tag, combine all those into a single frame with multiple pairs and single values?

NCrusher74 commented 4 years ago

Okay, I think I worked out the PresetOptionsFrame, now I'm trying to work on the LanguageFrame and it reminds me that I'm not sure I did this part of the PresetOptionsFrame properly either.

So, I've got my languages enum, with the ISO-639-T code for the cases, with the IsoName as the rawValue, and then computed properties for nativeName and ISO-639-B there and mapped to the cases and each other just in case someone wants them someday, I guess.

public enum ISO639Codes: String, CaseIterable {
    case aar = "Afar"
    case abk = "Abkhazian"
    case ace = "Achinese"
    case ach = "Acoli"
    case ada = "Adangme"
// etc etc
}

    public var nativeName: String {
        switch self {
            case .aar: return "Qafaraf; ’Afar Af; Afaraf; Qafar af"
            case .abk: return "Аҧсуа бызшәа; Аҧсшәа"
            case .ace: return "بهسا اچيه"
            case .ach: return "Lwo"
            case .ada: return "Dangme"
// etc etc
    }

    public var iso639Bibliographic: String {
        switch self {
            case .aar: return "aar"
            case .abk: return "abk"
            case .ace: return "ace"
            case .ach: return "ach"
            case .ada: return "ada"
// etc etc
    }

    public static let isoNameToNativeNameMapping: [String: String] = {
        var mapping: [String: String] = [:]
        for name in ISO639Codes.allCases {
            let isoName = name.rawValue
            mapping[isoName] = name.nativeName
        }
        return mapping
    }()
}

But if I'm not mistaken, what the user will see on autocomplete when entering the languageString for the frame is the actual 3-letter code, right?

What I'd like them to see is either the rawValue, and/or the nativeName. And I'm not sure how to accomplish that. Is it even possible?