Reworking tests

[NCrusher74]

I've gotten about as far as I can with this. It'd be one thing if the tests that are failing were consistent, but they're not.

Case in point. In my V24 "Problem Children" test, the first test for the MusicianCreditsList frame (Musician/Musician Name)was passing, but the second test (Singer/Singer Name) wasn't.

I made some changes on an entirely different frame that has nothing to do with that frame or those tests, and when I came back, it was the other way around. The second test was passing, but the first test wasn't. I hadn't touched the frame at all.

So at this point, I'm not sure what's my code, what's XCode being weird on me, and what's the result of Yate not writing frames the way I think they should be written. (example: the test of the MovementNumber and MovementCount frames fails. Not because the data isn't there, but because Yate writes both of those values to the Movement Number frame and ignores Movement Count. Which I have to think is a deliberate choice to go off-spec--not that there really is a spec for those iTunes specific frames--and handle the frames the same way disc and track numbers are handled.)

Anyway, I've been banging my head against the PresetOptionsFrame and CreditsListFrame for days now, and I'm not getting anywhere. I even went through and added documentation to the whole thing hoping it would help me look at it with new eyes and clarify what was going wrong, but instead all it did was lead me astray with a bunch of stuff that was working but I felt needed tweaking. I can't help it. I'm an inveterate tinkerer who can't stop fiddling with things.

Anyway, questions in the code when your workload lightens and you have time for them. Thanks!

[SDGGiesbrecht]

Okay. I have some time again. Many of the yet unanswered questions are in pull requests that have already been merged. I’ll take a look at the comments in this open pull request. But for the others, if any of the questions are still relevant, you’ll have to point me at which ones they are.

[NCrusher74]

I'm so turned around with how to handle the getter-setters for the PresetOptionsFrame. I'm going to talk it out here in the hopes of finding some clarity. (Seriously, you don't know how many comments I don't actually end up sending you because I stumble upon the answer, or at least an idea of where to look for it, while typing my question out.)

So, irrespective of the (version-dependent) presence of parentheses or null-terminators, what we're ultimately dealing with here is an array of strings. That's what the ID3 spec calls for.

When I initialize the PresetOptionsFrame, my frame-specific property is [String?]. That's what I parse out of a file being read, and what I encode into a file being written.

I've got my parser working now and I'd really rather not touch it if I don't have to, but that limits my options as to how I deal with getting and setting things.

Why bother with the preset enums (is a question I keep asking myself)? And the answer is because some or all of the strings in the array are supposed to be very specific strings as laid out in the spec, if the user chooses to use them. So I have to make those available. Well, maybe I don't have to, but it's good form I would imagine?

However, from a user perspective, what we're dealing with is are tuples, in which every element of the tuple is optional. For a Genre frame, the tuple is (GenreType?, String?) where GenreType is one of the selection of options from the GenreType enum and String is a freeform refinement or custom genre. Maybe they choose the preset option, maybe they choose the custom option, maybe they choose the preset and refine it with the string...or maybe they choose more than one of each.

For MediaType and FileType, the tuple is comprised of three types: (MediaType?, MediaTypeRefinement?, String?) but the same rules apply. They can choose all of one, some of one and some of another, none of one and multiple of others, or whatever. Usually there's not going to be a MediaTypeRefinement unless there's also a MediaType, and on the chance that the String is a refinement and not something else entirely, we need to keep these three grouped together, even though there can be multiples of each one.

So what we're actually dealing with is on the back end an array of tuples. Which I would think I'd be familiar with because that is how I was handling the CreditsListFrame initially, but the fact that the options are, well, optional, means that it's just different enough that there's no clean "pairs" structure to rely upon.

While I can usually count on there being a MediaType if there's a MediaTypeRefinement, it's possible neither one of those will be there and it will just be a String, or they'll both be there but there won't be a String. Or MediaType will be there without the refinements, or with just the String as a refinement.

I've considered adding properties for GenreType?, etc but since there can be more than one in the same instance of the frame, more accurately, it would be [GenreType?]. But we don't want to separate GenreType from whatever String immediate follows it, in case that string is a refinement, so it has to be [(GenreType?, String?)] and that's the point at which we start bumping up against messing with the parsing after I just got it working, which I'd rather not do if I can avoid it.

It wouldn't hurt anything if I had a (GenreType, String) pair where the string was not actually a refinement. One way or the other, that String is following that GenreType and I don't think it matters if it's handled as part of the tuple or not. But If I'm parsing out what is ultimately a stringArray, I'm parsing one component at a time, so I'd have to work out the logic for saying "if aString component follows GenreType component, treat them as a (GenreType, String) pair, but if String component follows another String component, handle it as though GenreType were nil so it's just (String) in the tuple. And also, make sure the String following a GenreType is a String and not another GenreType, in which case the first GenreType is actually (GenreType) where String is nil for that tuple."

...And that is the point at which my brain explodes.

So, yeah. I've got the parsing worked out. But I've tried multiple ways to deal with the user side and the interface between the two and I'm just not seeing a solution that both satisfies the requirements of the spec and keeps things as clean as possible on the user end.

[SDGGiesbrecht]

Would it help to turn the tuples into a small types?

struct PresetEntry {
  var type: MediaType?
  var refinement: MediaTypeRefinement?
  var whateverTheLastThingIs: String?
}

It doesn’t really enable anything new, but having named properties can make things easier to understand when you are reading the code that uses it. That can in turn make it easier to figure out how to put it together.

For this reason it’s rare to put a tuple in user‐facing API. (Well, that and the type can serve as a place to put documentation.)

[NCrusher74]

Would it help to turn the tuples into a small types?

struct PresetEntry {
  var type: MediaType?
  var refinement: MediaTypeRefinement?
  var whateverTheLastThingIs: String?
}

It doesn’t really enable anything new, but having named properties can make things easier to understand when you are reading the code that uses it. That can in turn make it easier to figure out how to put it together.

For this reason it’s rare to put a tuple in user‐facing API. (Well, that and the type can serve as a place to put documentation.)

I think that may have been what I was trying to do with the PresetOptions enum except there I was trying to emulate what you did with FrameLayoutIdentifier/KnownFrameLayoutIdentifier using an enum as a bridge to another enum, but I wasn't doing it well, and when I restructured the frame it turned out I hadn't needed it. Or something.

At any rate, it hadn't occured to me to try it as a struct instead of an enum. Maybe that will give me the flexibility I need to simplify things.

[NCrusher74]

Maybe I'm being particularly dense on this point, but how would I simplify this for a user to query? (or to write a test?)

Optional([SwiftTaggerID3.GenreEntry(presetType: nil, customGenre: Optional("Test Genre"))])

[NCrusher74]

Eh, nevermind. This is absurd. I'm wasting way too much time on an obscure frame that I personally really don't need all that much.

I'm just going to have to suck it up and accept that I don't have the skills to make this particular frame fully spec-compliant. If I allow the user to enter one value each of preset/refinement/freeformString that should be sufficient for almost any user, and way more than I'll ever personally use (and since I started this whole thing because I needed something I could use, anything above and beyond that is just me being...discarding a number of less flattering descriptors...fastidious.)

If anyone is nitpicky enough to need more than that, they have the option of forking and finding a way to implement it themselves, or finding another tool better suited to their needs.

[NCrusher74]

Okay, final question and then I'm shutting the book on the PresetOptionsFrame and never touching it again.

I've been having a lot of trouble with the final else never happening because the compiler wants either a nil-coalesce or a forced unwrapping, and the nil-coalesce is preventing things from ever reaching the final else.

I know we generally try not to force-unwrap, but if I make sure the array isn't empty first, it should be safe, right?

        get {
            var presetType: MediaType? = Optional.none
            var presetRefinement: MediaTypeRefinements? = Optional.none
            var refinementString: String? = ""
            // if the array exists...
            if let frameArray = presetOptionData(for: .mediaType),
                // ... and it isn't empty... (should be safe to force-unwrap, right?)
                !frameArray.isEmpty {
                for element in frameArray {
                    if MediaType(rawValue: element!) != nil {
                        presetType = MediaType(rawValue: element ?? "")
                    } else if MediaTypeRefinements(code: element!) != nil {
                        presetRefinement = MediaTypeRefinements(code: element ?? "")
                    } else {
                        refinementString = element!
                    }
                }
            }
            return (presetType, presetRefinement, refinementString)
        }

[SDGGiesbrecht]

they have the option of forking and finding a way to implement it themselves

Or sending you a pull request.

[NCrusher74]

they have the option of forking and finding a way to implement it themselves

Or sending you a pull request.

True!

[SDGGiesbrecht]

// ... and it isn't empty... (should be safe to force-unwrap, right?)
!frameArray.isEmpty {
for element in frameArray {
  if MediaType(rawValue: element!) != nil {

If frameArray is empty, then for will do its thing 0 times. So if !frameArray.isEmpty does nothing here.

But the array contains a list, where each entry is an optional string(?). That is the thing you need to somehow unwrap. One option is this:

if element.map({ MediaType(rawValue: $0) }) != nil {
  // There is an element and it can create a valid media type.
} else {
  // There is no element, or there is but it cannot create a valid media type.
}

There map method for optionals is basically the same as the one for collections. For collections it does the closure for each element and returns a new collection with the transformed elements. For optionals it does the closure for the contained element if it’s there and returns the transformed optional value.

[NCrusher74]

This is proving impossible to troubleshoot.

In the hopes that maybe seeing precisely what frames aren't coming out right when writing might lead me to more information, I went through the writing test function and added XCTAssertEqual blah blah blah tests for every item being written.

When I ran the test, I encountered a "Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)" error for what looked to be the "Copyright" frame.

I'm not sure why I encountered that error only upon writing and then trying to read what I had written, but not when reading or writing just by themselves, but okay, I went with it.

I wrote a test just for writing and reading the copyright frame:

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.copyright = "2020 Copyright"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.copyright, "2020 Copyright")
    }

I ran the test and it passed. No error.

So I went back to the test where I was writing and then trying to read ALL the frames and I got the "Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)" error again...this time for the CONDUCTOR frame, not the copyright frame.

I went to my single-frame test and changed it to check the conductor frame...and it passed. No errors.

I went back to the all-frames test and ran it again, and this time I got the error on the MediaType frame, not copyright or conductor.

Note: at no point did I touch anything in the all-frames test. I didn't change a thing, merely wrote another test isolating a single frame.

How do I even begin to troubleshoot something like that?

[NCrusher74]

Is the problem related to needing to...thread or something?

This test passed with no issues:

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.album = "Album"
        tag.albumArtist = "Album Artist"
        tag.albumArtistSort = "Album Artist Sort"
        tag.albumSort = "Album Sort"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.album, "Album")
        XCTAssertEqual(tagWritten.albumArtist, "Album Artist")
        XCTAssertEqual(tagWritten.albumArtistSort, "Album Artist Sort")
        XCTAssertEqual(tagWritten.albumSort, "Album Sort")
    }

This test, however, fails on every single item because it's reading all five frames as being empty strings:

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.album = "Album"
        tag.albumArtist = "Album Artist"
        tag.albumArtistSort = "Album Artist Sort"
        tag.albumSort = "Album Sort"
        tag.artist = "Artist"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.album, "Album")
        XCTAssertEqual(tagWritten.albumArtist, "Album Artist")
        XCTAssertEqual(tagWritten.albumArtistSort, "Album Artist Sort")
        XCTAssertEqual(tagWritten.albumSort, "Album Sort")
        XCTAssertEqual(tagWritten.artist, "Artist")
    }

It doesn't matter if the fifth item is Artist or Arranger or what. The fact that there is a fifth item causes none of the items to write.

At this point, the only thing I can think of is that it can't handle more than a few frames at a time?

Edit to add:

Sometimes, instead of causing none of the frames to write, the fifth frame causes the same error I mentioned above, such as here:

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.album = "Album"
        tag.albumArtist = "Album Artist"
        tag.albumArtistSort = "Album Artist Sort"
        tag.albumSort = "Album Sort"
        tag.audioFileWebpage = "http://audiofile.url"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.album, "Album")
        XCTAssertEqual(tagWritten.albumArtist, "Album Artist")
        XCTAssertEqual(tagWritten.albumArtistSort, "Album Artist Sort")
        XCTAssertEqual(tagWritten.albumSort, "Album Sort")
        XCTAssertEqual(tagWritten.audioFileWebpage, "http://audiofile.url")
    }

Everything else I have tried as the fifth item so far just caused the frames not to write. That one causes the bad instruction error on the "Album" frame. Which wrote fine when there were only four items.

ETA2: This causes the same error, but for "Album Sort" instead of "Album."

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.album = "Album"
        tag.albumArtist = "Album Artist"
        tag.albumArtistSort = "Album Artist Sort"
        tag.albumSort = "Album Sort"
        tag.audioSourceWebpage = "http://audiosource.url"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.album, "Album")
        XCTAssertEqual(tagWritten.albumArtist, "Album Artist")
        XCTAssertEqual(tagWritten.albumArtistSort, "Album Artist Sort")
        XCTAssertEqual(tagWritten.albumSort, "Album Sort")
        XCTAssertEqual(tagWritten.audioSourceWebpage, "http://audiosource.url")
    }

ETA3: bpm and compilation frames (both variations on StringFrame) worked when added to the list as the fifth item, but when I tried to add them both, nothing wrote and every test failed:

    func testProblems() throws {
        let mp3Url = Bundle.v23NoMeta
        let mp3File = try Mp3File(location: mp3Url)
        var tag = try Tag(readFrom: mp3File)

        tag.album = "Album"
        tag.albumArtist = "Album Artist"
        tag.albumArtistSort = "Album Artist Sort"
        tag.albumSort = "Album Sort"
        tag.bpm = 98
        tag.compilation = true
        //        tag.composer = "Composer"

        let outputUrl = URL(fileURLWithPath: "/Users/nolainecrusher/Desktop/test output/problemsV24.mp3")
        XCTAssertNoThrow(try mp3File.write(tagVersion: .v2_4, using: tag, writingTo: outputUrl))

        // MARK: Confirm accuracy
        let mp3UrlWritten = outputUrl
        let mp3FileWritten = try Mp3File(location: mp3UrlWritten)
        let tagWritten = try Tag(readFrom: mp3FileWritten)

        XCTAssertEqual(tagWritten.album, "Album")
        XCTAssertEqual(tagWritten.albumArtist, "Album Artist")
        XCTAssertEqual(tagWritten.albumArtistSort, "Album Artist Sort")
        XCTAssertEqual(tagWritten.albumSort, "Album Sort")
        XCTAssertEqual(tagWritten.bpm, 98)
        XCTAssertEqual(tagWritten.compilation, true)
//        XCTAssertEqual(tagWritten.composer, "Composer")

[SDGGiesbrecht]

None of this makes any sense to me from reading your descriptions. I’ll have to tinker with it myself. Maybe I’ll be able to get to it tomorrow?

[NCrusher74]

None of this makes any sense to me from reading your descriptions. I’ll have to tinker with it myself. Maybe I’ll be able to get to it tomorrow?

Okay. I did find one issue with the URL frames ending up with an extra 0 on the end somehow (the URLs were coming out "http://artist.url/0" which I managed to track down to the fact that they were getting the preferred encoding (which I've still got as isoLatin1 for now until I have a chance to figure out why the various tag readers were giving me gibberish when I used utf16WithBoM even after fixing the encoded identifier issue) when the urls should have just been getting the stringASCII encoding. But I don't think that's the cause of the issues I was encountering, because I was encountering them even without any URL frames in the mix.

I'm going through right now adding on one frame at a time and trying all the other frames one by one to see if it works with the frame or frames that they're being added to, and hopefully I'll have some more results by the time you get around to it.