Adding chapter and toc handler structure

[NCrusher74]

So, it took most of the day of me turning this around in my head before I finally started to see how it would come together.

This isn't complete, but am I on the right track here? I'm not entirely sure about using the chapter frame as the initializer for the Chapter substructure, but I couldn't figure out any other way to make it come together.

As it is, I've decided to make my life easier by declaring that SwiftTaggerID3 will only support a single, top-level TOC frame with ordered child elements. That gets rid of three of the properties for the TableOfContentsFrame type, which makes handling it a lot simpler. Anyone who needs a more complex TOC structure than that can submit a pull request, because I personally don't have any use for more than that, and I doubt many others will either.

Since I have the chapterFrame elementIDs available as an array property of the TOC frame, I created a second frameKey for the chapter frame so that I could initialize a chapter frame using the elementID, without giving up the ability to initialize a chapter frame using the startTime.

/// a public-facing type for handling the TableOfContents frame in a more intuitive manner
public struct TableOfContents {

    /// a dictionary of chapter frames within the tag.
    /// `Int`: the chapter start time
    public var chapters: [Int: Chapter]

    /// a public-facing type for handling the Chapter frame in a more intuitive manner
    public struct Chapter {
        /// the start time in miliseconds
        public var startTime: Int

        init(chapterFrame: ChapterFrame) {
            self.startTime = chapterFrame.startTime
        }        
    }

    /// The chapters in chronological order.
    public func sortedChapters() -> [(startTime: Int, chapter: Chapter)] {
        return chapters.keys.sorted().map { ($0, chapters[$0]!) }
    }
}

extension Tag {
    var tableOfContents: TableOfContents {
        get {
            var chapters: [Int: TableOfContents.Chapter] = [:]
            if let frame = self.frames[.tableOfContents],
                case .tocFrame(let tocFrame) = frame {
                for id in tocFrame.childElementIDs {
                    if let chapter = self.frames[.chapter(byElementID: id)],
                        case .chapterFrame(let chapterFrame) = chapter {
                        chapters[chapterFrame.startTime] = TableOfContents.Chapter(chapterFrame: chapterFrame)
                    }
                }
            }
            return TableOfContents(chapters: chapters)
        }
        set {
            // Assemble a frame based on the new value of your type and overwrite the old frame.
            // You’ll create arbitrary identifiers of your own here whenever needed.
        }
    }
}

[SDGGiesbrecht]

Yes, that’s the idea. Just some minor details seem out of place.

[SDGGiesbrecht]

if index < newValue.sortedChapters().count {
  let nextChapter = newValue.sortedChapters()[index + 1]
  endTime = nextChapter.startTime
} else {
  endTime = self.mp3Duration ?? 0
}

1. I would call sortedChapters() just once and save it in a local variable, otherwise you are re‐sorting it every each time you ask for it.
2. < sorted.endIndex, not < count.
3. The endIndex (here the same as count), is the past‐the‐end fencepost, and does not reference a valid element. So while index < endIndex guarantees it’s validity, it doesn’t guarantee the validity of index + 1, which will be the endIndex on the last iteration.
4. Isn’t what you’re saving to endTime actually the duration? Shouldn’t you add that to the startTime to get the real endTime?

I've also managed to break my chapter removal function.

Shouldn’t it now simply be self.tableOfContents[startTime] = nil?

[NCrusher74]

4. Isn’t what you’re saving to `endTime` actually the duration? Shouldn’t you add that to the `startTime` to get the real `endTime`?

No, it's just the start time of the next chapter, which should be the end time of the current chapter, which is what you recommended, right?

            let sortedChapters = newValue.sortedChapters()
            for (index, _) in sortedChapters {
                // index for current chapter
                if index < sortedChapters.endIndex {
                    // next chapter is the chapter at currentChapterIndex + 1
                    let nextChapter = sortedChapters[index + 1]
                    // current chapter endTime is equal to nextChapter startTime
                    endTime = nextChapter.startTime
                } else {
                    // unless it's the last chapter, in which case the end time is equal to the end of the file
                    endTime = self.mp3Duration ?? 0
                }
            }

Unless I'm doing something wrong?

I did notice when switching to endIndex, I couldn't use it on enumerated, which makes me a little nervous that it's looking at the startTime (as the first item in the tuple) instead of index but I haven't tested it yet to see.

I've also managed to break my chapter removal function.

Shouldn’t it now simply be self.tableOfContents[startTime] = nil?

With so many things named similar names, I want to be clear about what you're suggesting here.

It's the ChapterFrame I'm having trouble removing. I have two getter-setter subscripts for the ChapterFrame and the removal function, as follows:

    subscript(chapterFrom elementID: String) -> ChapterFrame? {
        get {
            if let frame = self.frames[.chapter(byElementID: elementID)],
                case .chapterFrame(let chapterFrame) = frame {
                return chapterFrame
            } else {
                return nil
            }
        }
        set {
            let frame = ChapterFrame(.known(.chapter),
                                     elementID: elementID,
                                     startTime: newValue?.startTime ?? 0,
                                     endTime: newValue?.endTime ?? 0,
                                     embeddedSubframesTag: newValue?.embeddedSubframesTag)
            self.frames[.chapter(byElementID: elementID)] = .chapterFrame(frame)
        }
    }

    subscript(chapterFrom startTime: Int) -> ChapterFrame? {
        get {
            if let frame = self.frames[.chapter(byStartTimeString: String(startTime))],
                case .chapterFrame(let chapterFrame) = frame {
                return chapterFrame
            } else {
                return .init()
            }
        }
        set {
            let frame = ChapterFrame(.known(.chapter),
                                     elementID: newValue?.elementID ?? UUID().uuidString,
                                     startTime: startTime,
                                     endTime: newValue?.endTime ?? 0,
                                     embeddedSubframesTag: newValue?.embeddedSubframesTag)
            self.frames[.chapter(byStartTimeString: String(startTime))] = .chapterFrame(frame)
        }
    }

    public mutating func removeChapter(at startTime: Int) {
        self.frames[.chapter(byStartTimeString: String(startTime))] = nil
    }

I've tried both the removal function as shown above, and also:

    public mutating func removeChapter(at startTime: Int) {
        self[chapterFrom: startTime] = nil
    }

(which I think is what you were suggesting?)

In the case of the latter, this is what I see afterward when looking at Yate:

The first two are the chapters that were already in the file, which I was trying to remove:

The second two are NEW chapters it created in the file instead of removing the ones already there:

Which is weird, because the subscript accessor is the startTime, and the new chapters should have the same startTime as the old chapters, and since the startTime is used to create the frameKey, it shouldn't allow duplicates?

Unless for some reason, it's using the startTime to access the frame, but then somehow generating the new frames using the elementID subscript as an accessor?

At any rate, what I see when I do it this way is slightly different:

    public mutating func removeChapter(at startTime: Int) {
        self.frames[.chapter(byStartTimeString: String(startTime))] = nil
    }

Using this approach, it doesn't add the new frames, but neither does it remove the old frames.

So, yeah, I'm a little perplexed by that right now.

[NCrusher74]

Okay, I think I see what I'm doing wrong.

            let newChapters = newValue.sortedChapters()
            for (index, _) in newChapters {
                if index < newChapters.endIndex {
                    let nextChapter = newChapters[index + 1]
                    endTime = nextChapter.startTime
                } else {
                    endTime = self.mp3Duration ?? 0
                }
            }

Only really works if newValue.sortedChapters has something in it. If I'm starting with an empty array, then index + 1 is going to be a problem, right?

So if I'm trying to build a chapter list from scratch, which is what I'm trying to do in this test, then I need to somehow prevent that. I'm drawing a blank on how right now, but a lot of that is because it's my (now) 13yo's birthday and it's been a busy day already and I haven't been able to cogitate on it much today.

[SDGGiesbrecht]

No, it's just the start time of the next chapter, which should be the end time of the current chapter, which is what you recommended, right?

Yeah, sorry. Not sure what I was thinking.

for (index, _) in newChapters {
  if index < newChapters.endIndex {
    let nextChapter = newChapters[index + 1]
    endTime = nextChapter.startTime
  } else {
    endTime = self.mp3Duration ?? 0
  }
}

That loop doesn’t do anything except calculate a single endTime.

To have one for each frame, you’d need to combine the to loops into one, and move let endTime and let elementID inside it.

You also need to erase all pre‐existing chapter frames in the setter first, because you’re assigning a fresh sequence of identifiers. Right?

[NCrusher74]

Okay, that's another reason why I need to get my removeChapter function working.

I'm a little confused about how to handle this loop. If I use newChapters = newValue.sortedChapters().enumerated() then I don't have access to endIndex. If I don't use enumerated() then I have the following errors:

            let newChapters = newValue.sortedChapters()
            for chapter in newChapters {
                let endTime: Int
                if index < newChapters.endIndex { 
// error at <
// Type '(UnsafePointer<Int8>?, Int32) -> 
// UnsafeMutablePointer<Int8>?' cannot conform to 'BinaryInteger'; 
// only struct/enum/class types can conform to protocols
                    let nextChapter = newChapters[index + 1]
// error at [index + 1]
// Cannot convert value of type '(UnsafePointer<Int8>?, Int32) -> 
// UnsafeMutablePointer<Int8>?' to expected argument type 'Int'
                  endTime = nextChapter.startTime
                } else {
                    endTime = self.mp3Duration ?? 0
                }

[SDGGiesbrecht]

newChapters.index(after: index) instead of index + 1.

[NCrusher74]

That doesn't change the error. Why is index (UnsafePointer<Int8>?, Int32) -> UnsafeMutablePointer<Int8>? rather than Int?

[SDGGiesbrecht]

Sorry. Both that and the following:

for index in newChapters.indices {
  let chapter = newChapters[index]
  // ...

[NCrusher74]

Okay, thank you. I had to adjust the first one a bit, because let nextChapter = newChapters.index(after: index) was giving me an Int for nextChapter when I needed the actual chapter, but I think I've got it now? I just need to make sure index + 1 is less than endIndex, as opposed to index, right?

            let newChapters = newValue.sortedChapters()
            for index in newChapters.indices {
                let chapter = newChapters[index]
                let endTime: Int
                let nextIndex = newChapters.index(after: index)
                // this should make sure it never goes out of range when getting the next chapter, right?
                if nextIndex < newChapters.endIndex {
                    let nextChapter = newChapters[nextIndex]
                    endTime = nextChapter.startTime
                } else {
                    endTime = self.mp3Duration ?? 0
                }

It's still not writing, because now I'm getting an InvalidTOCData error, which is the error thrown when the entryCount (the count of the entries in the childElementIDs array, which is a spec requirement of theTableOfContentsFramethat I generate automatically when encoding the frame contents) is zero. So my adding thechildElementIDshere isn't making it all the way to theTableOfContentsFrame.encodeContents` function and I need to figure out why, but at least I'm not out of bounds now.

But first, I need to figure out why the removal of the chapter frames isn't working, because as you said, that needs to be done before I start building the new chapter frames and TOC.

[NCrusher74]

Okay, I bet I know what's going wrong.

I've got two FrameKeys for ChapterFrame, because I have two times when I need to access the frame.

When I'm doing it here, using the childElementIDs array from the TableOfContentsFrame, when all I have is an elementID for the chapter frame, so I use that as the FrameKey:

        get {
            // initialize a chapter instance by the start time
            var chapters: [Int: TableOfContents.Chapter] = [:]
            // get the TOC frame
            if let tocFrame = self.toc {
                let childElementIDs = tocFrame.childElementIDs
                for elementID in childElementIDs {
                    let frameKey: FrameKey = .chapter(byElementID: elementID)
                    if let frame = self.frames[frameKey],
                        case .chapterFrame(let chapterFrame) = frame {
                        let startTime = chapterFrame.startTime
                        let chapter = TableOfContents.Chapter(from: chapterFrame)
                        chapters[startTime] = chapter
                    }
                }
            }
            return TableOfContents(chapters: chapters)
        }

But later, when I'm rebuilding all the chapter frames, what I've got to go on is the startTime in our TableOfContents type (rather than the TableOfContentsFrame.) So I use that for the FrameKey.

// from the FrameKey enum
    case chapter(byStartTime: Int)
    case chapter(byElementID: String)

In terms of how this works in the ChapterFrame structure, I initialize the frameKey property for the frame by the elementID when decoding the frame (since that's what I've got to work with in the get portion of the tableOfContents getter/setter) but then when I'm encoding the frame, I initialize the frameKey by the startTime.

I suspect that is why my attempt to remove a frame (using a startTime frame key) isn't removing frames that were read from file using an elementID frame key.)

Which...is sort of a sticky problem. If I don't initialize frameKey to the elementID version when decoding the frame, then I can't read a chapter frame. So I can't get rid of that.

But our TableOfContents structure is supposed to be blind to the elementIDs, so I don't store them anywhere and thus I don't have them later when rebuilding all my chapter frames. All I've got to go on is the startTime. So I can't get rid of thestartTime`-derived frameKey either.

If I'm dealing with files written by SwiftTagger, I could always make the elementID a string of the startTime and sort of split the difference that way, so that the frame key is always the same either way. But sometimes I might be dealing with files written by other tools and they might not handle it that way.

What I need may be...an intermediate step? Like, if I say "get me a ChapterFrame with elementID: chX it says "okay, that is actually the chapterFrame with SooperSekritElementID: I-could-tell-you-but-id-have-to-kill-you, which you can also get to using startTime: 123456.

Hmmm....

[NCrusher74]

I know this won't actually work, because it's still two different frame keys, but I need something along these lines:

enum FrameKey {
    case chapter(byStartTime: Int?, orElementID: String?)

// snip

    static func chapterByStartTime(startTime: Int) -> FrameKey {
        return .chapter(byStartTime: startTime, orElementID: nil)
    }

    static func chapterByElementID(elementID: String) -> FrameKey {
        return .chapter(byStartTime: nil, orElementID: elementID)
    }

}

Unless...

        self.frameKey = .chapter(byStartTime: self.startTime, orElementID: self.elementID)

if the frameKey is initialized in the frame like this, is there a way I can utilize it with only one of the two values, and the other set to nil?

[SDGGiesbrecht]

You shouldn’t need the identifier subscripts anymore (unless they are somehow useful in the implementation of the following):

• The table of contents getter should simply load everything that is a chapter. (It already does, doesn’t it?)
• The table of contents setter should obliterate all existing chapter frames (by iterating the frames and removing anything that is a chapter), and then rebuild the set of chapter frames from scratch.

When users want to remove a specific chapter, they do this:

tag.tableOfContents.chapters[startTime] = nil

If they want to erase all of them, they do this:

tag.tableOfContents.chapters = [:]

At least that is what I envisioned. It might need a little adjustment if chapters are supposed to be able to exist outside of a table of contents, or if you are supposed to be able to have multiple tables of contents.

[NCrusher74]

The TableOfContents getter DOES load everything that is a chapter, but the way it does that is by loading the TableOfContentsFrame and using it's childElementIDs array property to get the elementIDs for the chapters.

            // get the TOC frame
            if let tocFrame = self.toc {
                // the TOC frame has an array of elementIDs for the chapters
                let childElementIDs = tocFrame.childElementIDs

Then it uses each elementID in that array as the frameKey to access the ChapterFrame for each chapter, and reads the startTime from the chapter frame's data.

                // use those to initialize and decode a chapter frame
                for elementID in childElementIDs {
                    let frameKey: FrameKey = .chapter(byElementID: elementID)
                    if let frame = self.frames[frameKey],
                        case .chapterFrame(let chapterFrame) = frame {
                        let startTime = chapterFrame.startTime
                        let chapter = TableOfContents.Chapter(from: chapterFrame)
                        chapters[startTime] = chapter
                    }
                }

It then adds the startTime to the chapters dictionary and created a TableOfContents.Chapter instance from the ChapterFrame.

I'm...drawing a blank at the moment as to how I would get the startTime property from an unknown number of chapter frames without a subscript accessor, and the only way to get something that would allow me to target a specific ChapterFrame and read its data without already knowing its data would be to get the elementID from the TableOfContentsFrame.

I guess I could try to find a way to iterate over the entire Tag instance and isolate all the ChapterFrames, but even that will be rough because the Tag instance is designed to look up frames using the frameKey, and when there are multiple frames of the same type, that frameKey needs a secondary identifier.

I can play around with the code and see I can find a way

tag.tableOfContents.chapters[startTime] = nil

This...would remove the TableOfContents.Chapter instance, but it wouldn't remove the ChapterFrame instance. That's what we need to do, right? Otherwise, those frames are just lingering there in the tag data waiting to mess up someone's day, somehow.

It might need a little adjustment if chapters are supposed to be able to exist outside of a table of contents, or if you are supposed to be able to have multiple tables of contents.

Technically, both of these are true. The spec allows for it.

However, I've made the executive decision NOT to support more than one TableOfContentsFrame to keep things simple (and that frame won't have any embedded subframes, so the only parameter that I can't create using arbitrary values is the childElementIDs array.)

I've also noticed that, while the spec does allow for chapter frames that aren't accounted for in a CTOC frame's array of childElementIDs, even apps that will recognize and work with ID3 chapters won't recognize them. Admittedly, my sample size is one app (called Fission) but in that app, if there are chapter frames that aren't included in the CTOC frame's childElementIDs array, none of the chapter frames will be recognized. Which is why we need to not just be able to wipe the TableOfContents.Chapter instance, but to actually remove the chapter frames when we build new ones.

[NCrusher74]

Yeah, I don't see any way to leave out the subscript identifier for the chapter frames. From the ground up, we've been handling frames as a [FrameKey: Frame] for specifically this reason: there will be multiple ChapterFrame instances, so they need to be made unique by a secondary identifier.

If I read in a file with multiple ChapterFrames, each successive one would overwrite the last if they all had the same FrameKey, right? And I wouldn't be able to write more than one ChapterFrame to a file. At least not without radically changing how our Tag instance is initialized.

[NCrusher74]

I think I must be using the structure incorrectly.

I'm running this test:

    func testFrameWriting() throws {
        var tag = try TestFile.noMeta.tag()

        var toc = tag?.tableOfContents.chapters
        toc?[0]?.subframes?.title = "Chapter 001"
        toc?[1680]?.subframes?.title = "Chapter 002"
        toc?[3360]?.subframes?.title = "Chapter 003"

        tag?.tableOfContents.chapters = toc ?? [:]

        let outputUrl = try localDirectory(fileName: "newtoctest", fileExtension: "mp3")
        XCTAssertNoThrow(try TestFile.noMeta.mp3File()?.write(
            tagVersion: .v2_4,
            using: tag ?? Tag(readFrom: Mp3File(location: TestFile.noMeta.url)),
            writingTo: outputUrl)) // XCTAssertNoThrow failed: threw error "InvalidTOCFrame"

InvalidTOCFrame is thrown in the encodeContents function of the TableOfContentsFrame:

        // encode and append the entry count
        let entryCount = self.childElementIDs.count
        // a valid TOC frame needs at least 1 child element
        guard entryCount > 0 else {
            throw Mp3File.Error.InvalidTOCFrame
        }

(incidentally, I need a way to sidestep this in cases where the TOC frame or chapters are being removed, but that's another issue.)

I started putting print commands in the TableOfContents to setter to see where the elementIDs were being lost, and it turns out, they were never created in the first place, because the TableOfContents.chapters dictionary is empty from the get-go.

            // initialize an empty `childElementIDs` array
            var childElementIDs: [String] = []
            // store the new chapters array to avoid resorting every time we call it
            let newChapters = newValue.sortedChapters()
            print(newChapters) // empty

So, for some reason, this:

        var toc = tag?.tableOfContents.chapters
// create chapters at startTime 0, 1680, and 3360, with title subframes
        toc?[0]?.subframes?.title = "Chapter 001"
        toc?[1680]?.subframes?.title = "Chapter 002"
        toc?[3360]?.subframes?.title = "Chapter 003"

        tag?.tableOfContents.chapters = toc ?? [:]

isn't making it into TableOfContents.chapters.

[SDGGiesbrecht]

The TableOfContents getter DOES load everything that is a chapter, but the way it does that is by loading the TableOfContentsFrame and using it's childElementIDs array property to get the elementIDs for the chapters.

The setter still has access to the existing value until you actually change it. You can call the getter to pull the existing table of contents and use its list to know what to wipe out.

I guess I could try to find a way to iterate over the entire Tag instance and isolate all the ChapterFrames, but even that will be rough because the Tag instance is designed to look up frames using the frameKey, and when there are multiple frames of the same type, that frameKey needs a secondary identifier.

for frame in whatever {
  if case .chapter(let identifier) = frame.identifier {
    // ...
  }
}

[NCrusher74]

The TableOfContents getter DOES load everything that is a chapter, but the way it does that is by loading the TableOfContentsFrame and using it's childElementIDs array property to get the elementIDs for the chapters.

The setter still has access to the existing value until you actually change it. You can call the getter to pull the existing table of contents and use its list to know what to wipe out.

I think I figured that out toward the end of the night last night. Here is what I have at the start of the setter now:

            // wipe the existing chapter frames and TOC so they can be replaced
            if let tocFrame = self.toc {
                let oldElementIDs = tocFrame.childElementIDs
                for id in oldElementIDs {
                    self.frames.removeValue(forKey: .chapter(byElementID: id))
                }
            }
            self.toc = nil

I'll probably sound like a complete idiot saying this, but I honestly don't remember why I was having such trouble with the frameKey issue yesterday and convinced I needed two different frameKeys for the chapter frame. I blame the constant interruptions throughout the day. I'm lucky I can remember my own name.

At any rate, I was able to get rid of the startTime-based frameKey and everything was simplified by that. Reading is now working, frame removal is working, and all that's left to do is figure out why the chapters dictionary isn't being populated.

What I'm trying to write is this:

        var toc = tag?.tableOfContents.chapters
        toc?[0]?.chapterTitle = "Chapter 001"
        toc?[1680]?.chapterTitle = "Chapter 002"
        toc?[3360]?.chapterTitle = "Chapter 003"

        tag?.tableOfContents.chapters = toc ?? [:]

        let outputUrl = try localDirectory(fileName: "newtoctest", fileExtension: "mp3")
        XCTAssertNoThrow(try TestFile.noMeta.mp3File()?.write(
            tagVersion: .v2_4,
            using: tag ?? Tag(readFrom: Mp3File(location: TestFile.noMeta.url)),
            writingTo: outputUrl))

Which, if I understand the way it's supposed to work properly, should create three entries in the chapters dictionary.

The first entry should have a startTime of 0ms, and a Chapter instance initialized by a chapterTitle string of "Chapter 001".

The second entry should have a startTime of 1680ms, and a Chapter instance initialized by a chapterTitle string of "Chapter 002".

The third entry should have a startTime of 0ms, and a Chapter instance initialized by a chapterTitle string of "Chapter 003".

But the write operation isn't happening because the childElementIDs array in the TableOfContentsFrame is empty and thus I'm encountering an error that I intentionally included because a valid TOC frame needs to have at least one child element.

So, I start putting print commands in my setter to see where those childElementIDs are being lost. The print command at the bottom is the first, and I work my way upwards.

        set {
            // wipe the existing chapter frames and TOC so they can be replaced
            if let tocFrame = self.toc {
                let oldElementIDs = tocFrame.childElementIDs
                for id in oldElementIDs {
                    self.frames.removeValue(forKey: .chapter(byElementID: id))
                }
            }
            self.toc = nil

            // initialize an empty `childElementIDs` array
            var childElementIDs: [String] = []
            // store the new chapters array to avoid resorting every time we call it
            let newChapters = newValue.sortedChapters()
//            print(newChapters) // [] prints
            // for each index in the chapters array...
            for index in newChapters.indices {
                // get the current chapter
                let chapter = newChapters[index]
                // get the endTime for the current chapter from the startTime of the next chapter
                let endTime: Int
                // get the index of the next chapter
                let nextIndex = newChapters.index(after: index)
                if nextIndex < newChapters.endIndex {
                    let nextChapter = newChapters[nextIndex]
                    // get the start time of the next chapter for the end time of the current chapter
                    endTime = nextChapter.startTime
                } else {
                    // unless it's the last chapter, in which case the end time is the end of the file
                    endTime = self.mp3Duration ?? 0
                }
                // assign an arbitary elementID to the chapter
                let elementID = "ch\(index)"
//                print(elementID) // nothing prints
                // build a title subframe
                let titleFrame = StringFrame(.known(.title), contentString: chapter.chapter.chapterTitle ?? "Chapter Title")
                let titleFrameKey: FrameKey = .title
                let subframe = Frame.stringFrame(titleFrame)
                let subframesTag = Tag(subframes: [titleFrameKey : subframe])

                // initialize a ChapterFrame instance for the chapter
                // using all the elements we just constructed
                let frame = ChapterFrame(.known(.chapter),
                                         elementID: elementID,
                                         startTime: chapter.startTime,
                                         endTime: endTime,
                                         embeddedSubframesTag: subframesTag)
                self.frames[.chapter(byElementID: elementID)] = .chapterFrame(frame)
                // add the elementID to the childElementIDs array for the TOC frame
                childElementIDs.append(elementID)
//                print(childElementIDs) // nothing prints
            }
            // initialize a CTOC frame and populate it with the child element IDs array
            let frame = TableOfContentsFrame(.known(.tableOfContents),
                                             childElementIDs: childElementIDs,
                                             embeddedSubframesTag: nil)
            self.frames[.tableOfContents] = .tocFrame(frame)
        }
    }

So, newChapters is an empty array, which means there is nothing in newValue.sortedChapters(). Which is just a rearranged version of the chapters dictionary, so I move there for my next print command:

    public func sortedChapters() -> [(startTime: Int, chapter: Chapter)] {
//        print(chapters) // [:] prints
        return chapters.keys.sorted().map { ($0, chapters[$0]!) }
    }

So chapters is empty. Which means that what I'm doing to create those entries in the chapters dictionary isn't working.

Since I know there is more than one way to add values to a dictionary, I change the way I'm writing things:

        toc?.updateValue(TableOfContents.Chapter(title: "Chapter 001"), forKey: 0)

And I get an error saying that the initializer for Chapter is unavailable because it's not public. Since the initializers for both TableOfContents and TableOfContents.Chapter are implicit (and probably thus internal by default) I add explicit public initializers:

/// a public-facing type for handling the TableOfContents frame in a more intuitive manner
public struct TableOfContents {

    /// a dictionary of chapter frames within the tag.
    /// `Int`: the chapter start time
    public var chapters: [Int: Chapter]
    public init(chapters: [Int : Chapter]) {
        self.chapters = chapters
    }

    /// a public-facing type for handling the Chapter frame in a more intuitive manner
    public struct Chapter {
        public var chapterTitle: String?
        public init(title: String?) {
            self.chapterTitle = title
        }
    }
// snip
}

And...it works! But only if I update the chapters dictionary the second way:

// doesn't work
        toc?[0]?.chapterTitle = "Chapter 001"
        toc?[1680]?.chapterTitle = "Chapter 002"
        toc?[3360]?.chapterTitle = "Chapter 003"

// works
        toc?.updateValue(TableOfContents.Chapter(title: "Chapter 001"), forKey: 0)
        toc?.updateValue(TableOfContents.Chapter(title: "Chapter 002"), forKey: 1680)
        toc?.updateValue(TableOfContents.Chapter(title: "Chapter 003"), forKey: 3360)

Since the second way is pretty clumsy, I'm hoping there's a more streamlined way to do this.

[NCrusher74]

Found the easier way.

    public mutating func addChapter(at startTime: Int, title: String) {
        var toc = self.tableOfContents.chapters
        toc.updateValue(TableOfContents.Chapter(title: title), forKey: startTime)
        self.tableOfContents.chapters = toc
    }

Which allows me to do this:

        tag?.addChapter(at: 0, title: "Chapter 001")
        tag?.addChapter(at: 1680, title: "Chapter 002")
        tag?.addChapter(at: 3360, title: "Chapter 003")

[SDGGiesbrecht]

toc?[0]?.chapterTitle = "Chapter 001"

toc?.updateValue(TableOfContents.Chapter(title: "Chapter 001"), forKey: 0)

The first means, “Look for a chapter at 0, and if there is one, change its title.” (It will abort at the second ? if there was no chapter at 0.)

The second simply puts a brand‐new chapter at 0, overwriting whatever may or may not have been there before. It could be written more concisely like this:

toc?[0] = TableOfContents.Chapter(title: "Chapter 001")

Both are “correct” for their different purposes.

[NCrusher74]

Okay, I've almost got everything working, but I've hit a hinky issue with overwriting existing chapters. I know what's happening, and why, but I'm not sure if I should try to change it, and if I should, how.

In other words, I'm not sure if it's a bug or a feature.

This is my test:

    func testOverwriting() throws {
        var tag = try TestFile.chapterized.tag()

        tag?.addChapter(at: 0, title: "Chapter 001")
        tag?.addChapter(at: 1680, title: "Chapter 002")
        tag?.addChapter(at: 3360, title: "Chapter 003")

        let outputUrl = try localDirectory(fileName: "newtoctest", fileExtension: "mp3")
        XCTAssertNoThrow(try TestFile.chapterized.mp3File()?.write(
            tagVersion: .v2_4,
            using: tag ?? Tag(readFrom: Mp3File(location: TestFile.chapterized.url)),
            writingTo: outputUrl))

        let writtenFile = try Mp3File(location: outputUrl)
        let writtenTag = try Tag(readFrom: writtenFile)

        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[0].startTime, 0)
        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[0].chapter.chapterTitle, "Chapter 001")
        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[1].startTime, 1680)
        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[1].chapter.chapterTitle, "Chapter 002")
        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[2].startTime, 3360)
        XCTAssertEqual(writtenTag.tableOfContents.sortedChapters()[2].chapter.chapterTitle, "Chapter 003")
    }

The file I'm reading from, and writing to, already has two chapters:

startTime: 0ms, chapterTitle: "Chapter 01" startTime: 2795ms, chapterTitle: "Chapter 02"

The chapter at 0ms gets overwritten, as it should, but the one at 2795 doesn't. So the last two checks in my test fail, because the chapter at writtenTag.tableOfContents.sortedChapters()[2] has a startTime of 2795 and a chapterTitleof "Chapter 02", and the third chapter I added is actually the fourth chapter in the final result.

Which makes sense. The chapter at 2795 is being added to the chapters dictionary in the getter, and never removed in the setter, so the new chapters are just being added to it and a new TableOfContentsFrame created to include them all. I checked the TOC frame, and looked at the file in Fission. All the chapters are being recognized, even the leftover one. And when I view the raw frame data in Yate, the chapter (which previously had an elementID of "ch1") now has an elementID of "ch2" because it's the third chapter when sorted. So can confirm that part is working, so it's great news for when we have old chapters we want to keep instead of overwriting.

So...I should call it a day with that, right? If someone wants to remove a chapter that isn't being overwritten by another chapter at the same startTime, they should be required to do that on a case-by-case basis using our (now functional) removal commands, right?

At this point, beyond adding another getter/setter to possibly simplify things still further so that instead of writtenTag.tableOfContents.sortedChapters()[0].startTime and writtenTag.tableOfContents.sortedChapters()[0].chapter.chapterTitle it would be writtenTag.chapters[0].startTime, I'm not sure if I should fuss with it anymore? I don't know.

[SDGGiesbrecht]

Yes. That is how it is supposed to work.

To erase all existing chapters, a user could iterate the chapter list to get all the start times, then for each one, set the chapter there to nil. If you want to put that in a convenient function, go ahead.

[NCrusher74]

Okay.

For some reason I was just all flummoxed by that yesterday.

I've already got these functions, so it should be all good:

    /// Removes the chapter at the specified start time.
    public mutating func removeChapter(at startTime: Int) {
        self.tableOfContents.chapters[startTime] = nil
    }

    /// Removes all chapters.
    public mutating func removeAllChapters() {
        self.tableOfContents.chapters = [:]
    }

[SDGGiesbrecht]

✅