[ID3v2] Split Frame Factory#139
Open
benrr101 wants to merge 43 commits into
Open
Conversation
…ender methods to the frame header class.
There was a problem hiding this comment.
Pull request overview
This PR refactors ID3v2 frame construction/parsing by splitting the frame factory into “from file” vs “from raw/tag bytes” paths and standardizing frame parsing on fromFieldBytes(header, fieldBytes, version). It also moves extended-header handling into header types, introduces new dedicated frame classes for user text/URL frames, and updates unit/integration tests to match the new parsing/rendering behavior.
Changes:
- Refactored ID3v2 frame parsing/rendering to use
fromFieldBytes(...), and updated the frame factory to construct frames from file or tag byte blocks. - Moved/expanded ID3v2 frame/tag header responsibilities (extended header sizing/reading, extended per-frame header bytes, unsync handling).
- Added/updated extensive unit + integration tests for the new parsing/rendering paths, plus updated public exports.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test-unit/utilities/testers.ts | Adjusts ByteVector equality helper behavior for falsy expectations. |
| test-unit/id3v2/userUrlLinkFrameTests.ts | Updates tests to UserUrlLinkFrame.fromFieldBytes(...) and adds more cases. |
| test-unit/id3v2/userTextInformationFrameTests.ts | Migrates tests to new UserTextInformationFrame type and parsing entrypoint. |
| test-unit/id3v2/urlLinkFrameTests.ts | Migrates tests to UrlLinkFrame.fromFieldBytes(...) and adds helpers. |
| test-unit/id3v2/unsynchronizedLyricsFrameTests.ts | Updates USLT tests for new field-bytes parsing and expanded render coverage. |
| test-unit/id3v2/unknownFrameTests.ts | Updates unknown frame tests to new factory/parsing flow. |
| test-unit/id3v2/uniqueFileIdentifierFrameTests.ts | Updates UFID tests to new parsing flow and new invalid-data expectations. |
| test-unit/id3v2/termsOfUseFrameTests.ts | Updates USER tests to new parsing flow and adds version/encoding render cases. |
| test-unit/id3v2/tagExtendedHeaderTests.ts | Updates expectations for extended header size decoding. |
| test-unit/id3v2/relativeVolumeFrameTests.ts | Updates RVA2 tests to new parsing flow and expanded render/parse cases. |
| test-unit/id3v2/privateFrameTests.ts | Updates PRIV tests to new parsing flow and expanded render cases. |
| test-unit/id3v2/playCountFrameTests.ts | Updates PCNT tests to new parsing flow and tighter size validation. |
| test-unit/id3v2/musicCdIdentifierFrameTests.ts | Updates MCDI tests to new parsing flow and adds fromData coverage. |
| test-unit/id3v2/id3v2TagTests.ts | Updates imports to new frame module structure (default exports / split classes). |
| test-unit/id3v2/genreFrameTests.ts | Updates genre tests to use fromFieldBytes(...) parsing entrypoint. |
| test-unit/id3v2/frameTests.ts | Removes tests tied to deleted frame raw parsing helpers. |
| test-unit/id3v2/frameConstructorTests.ts | Updates base constructor tests to validate fromFieldBytes(...) contract. |
| test-unit/id3v2/eventTimeCodeFrameTests.ts | Updates ETCO tests to new parsing flow and adds new invalid cases. |
| test-unit/id3v2/commentsFrameTests.ts | Updates COMM tests to new parsing flow and expanded render coverage. |
| test-integration/mp3_id3v24_fileTests.ts | Switches URL writes to setUrlFrame(...) for URL frames. |
| test-integration/mp3_id3v2_fileTests.ts | Switches URL writes to setUrlFrame(...) for URL frames. |
| test-integration/mp3_id3v1_fileTests.ts | Adds TODO notes clarifying test intent/coverage. |
| src/index.ts | Updates public exports for split frame classes and default exports. |
| src/id3v2/id3v2Tag.ts | Splits parsing into parseFromData / parseFromFile paths and updates URL setting API. |
| src/id3v2/id3v2ExtendedHeader.ts | Adds explicit extended-header size parsing and file-based reading. |
| src/id3v2/frames/userUrlLinkFrame.ts | New dedicated WXXX implementation with fromFieldBytes(...) + rendering. |
| src/id3v2/frames/userTextInformationFrame.ts | New dedicated TXXX implementation with fromFieldBytes(...) + rendering. |
| src/id3v2/frames/urlLinkFrame.ts | Refactors URL link frames to default export and fromFieldBytes(...). |
| src/id3v2/frames/unsynchronizedLyricsFrame.ts | Refactors USLT parsing to fromFieldBytes(...). |
| src/id3v2/frames/unknownFrame.ts | Adds fromFieldBytes(...) constructor and removes raw parsing method. |
| src/id3v2/frames/uniqueFileIdentifierFrame.ts | Refactors UFID parsing to fromFieldBytes(...) with stricter validation. |
| src/id3v2/frames/textInformationFrame.ts | Refactors text frames to default export + fromFieldBytes(...). |
| src/id3v2/frames/termsOfUseFrame.ts | Refactors USER parsing to fromFieldBytes(...). |
| src/id3v2/frames/synchronizedLyricsFrame.ts | Refactors SYLT parsing to fromFieldBytes(...) and tweaks render ordering/filtering. |
| src/id3v2/frames/relativeVolumeFrame.ts | Refactors RVA2 parsing to fromFieldBytes(...) and simplifies render pipeline. |
| src/id3v2/frames/privateFrame.ts | Refactors PRIV parsing to fromFieldBytes(...). |
| src/id3v2/frames/popularimeterFrame.ts | Refactors POPM parsing to fromFieldBytes(...) with size constraints. |
| src/id3v2/frames/playCountFrame.ts | Refactors PCNT parsing to fromFieldBytes(...) with size constraints. |
| src/id3v2/frames/musicCdIdentifierFrame.ts | Refactors MCDI parsing and adds fromData(...). |
| src/id3v2/frames/genreFrame.ts | Refactors TCON parsing to fromFieldBytes(...). |
| src/id3v2/frames/frameHeader.ts | Adds extended per-frame header support (group id, DLI, etc.) and base-size helpers. |
| src/id3v2/frames/frameFactory.ts | Splits factory into file/tag-bytes entrypoints and centralizes per-frame constructors. |
| src/id3v2/frames/frame.ts | Updates rendering pipeline to include extended header bytes and removes old raw parsing helpers. |
| src/id3v2/frames/eventTimeCodeFrame.ts | Refactors ETCO parsing to fromFieldBytes(...) and tightens malformed detection. |
| src/id3v2/frames/commentsFrame.ts | Refactors COMM parsing to fromFieldBytes(...). |
| src/id3v2/frames/attachmentFrame.ts | Refactors APIC/GEOB setup to fromFieldBytes(...) and updates frameId switching logic. |
| package.json | Downgrades Mocha dev dependency version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1453
to
+1460
| // Only add the frame if its size is > 0 | ||
| // @TODO: How is this ever possible? | ||
| if (result.frame.size === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| this.addFrame(result.frame); | ||
| position += result.totalSize; |
Comment on lines
+1498
to
+1505
| // Only add the frame if its size is > 0 | ||
| // @TODO: How is this ever possible? | ||
| if (result.frame.size === 0) { | ||
| continue; | ||
| } | ||
| this.addFrame(frame); | ||
|
|
||
| this.addFrame(result.frame); | ||
| position += result.totalSize; |
Comment on lines
+39
to
+41
| if (fieldBytes.length < 4) { | ||
| throw new CorruptFileError("Genre frame must contain at least 4 bytes."); | ||
| } |
Comment on lines
128
to
+140
| /** | ||
| * Gets the encryption ID applied to the current instance. | ||
| * @returns | ||
| * Value containing the encryption identifier for the current instance or | ||
| * `undefined` if not set. | ||
| */ | ||
| public get encryptionId(): number | undefined { | ||
| return NumberUtils.hasFlag(this.flags, Id3v2FrameFlags.Encryption) | ||
| ? this._encryptionId | ||
| : undefined; | ||
| } | ||
| public get encryptionId(): number { return this._header.encryptionId; } | ||
| /** | ||
| * Sets the encryption ID applied to the current instance. | ||
| * @param value Value containing the encryption identifier for the current instance. Must be an | ||
| * 8-bit unsigned integer. Setting to `undefined` will remove the encryption header and ID | ||
| */ | ||
| public set encryptionId(value: number | undefined) { | ||
| Guards.optionalByte(value, "value"); | ||
| this._encryptionId = value; | ||
| if (value !== undefined) { | ||
| this.flags |= Id3v2FrameFlags.Encryption; | ||
| } else { | ||
| this.flags &= ~Id3v2FrameFlags.Encryption; | ||
| } | ||
| } | ||
| public set encryptionId(value: number) { this._header.encryptionId = value; } |
Comment on lines
166
to
+178
| /** | ||
| * Gets the grouping ID applied to the current instance. | ||
| * @returns | ||
| * Value containing the grouping identifier for the current instance, or | ||
| * `undefined` if not set. | ||
| */ | ||
| public get groupId(): number | undefined { | ||
| return NumberUtils.hasFlag(this.flags, Id3v2FrameFlags.GroupingIdentity) | ||
| ? this._groupId | ||
| : undefined; | ||
| } | ||
| public get groupId(): number { return this._header.groupId; } | ||
| /** | ||
| * Sets the grouping ID applied to the current instance. | ||
| * @param value Grouping identifier for the current instance. Must be an 8-bit unsigned integer. | ||
| * Setting to `undefined` will remove the grouping identity header and ID | ||
| */ | ||
| public set groupId(value: number | undefined) { | ||
| Guards.optionalByte(value, "value"); | ||
| this._groupId = value; | ||
| if (value !== undefined) { | ||
| this.flags |= Id3v2FrameFlags.GroupingIdentity; | ||
| } else { | ||
| this.flags &= ~Id3v2FrameFlags.GroupingIdentity; | ||
| } | ||
| } | ||
| public set groupId(value: number) { this._header.groupId = value; } |
Comment on lines
+113
to
+115
| @params(2, "v2") | ||
| @params(3, "v3") | ||
| @params(3, "v3") |
Comment on lines
+125
to
+127
| @params(2, "v2") | ||
| @params(3, "v3") | ||
| @params(3, "v3") |
Comment on lines
+140
to
+142
| @params(2, "v2") | ||
| @params(3, "v3") | ||
| @params(3, "v3") |
Comment on lines
+165
to
+167
| @params(2, "v2") | ||
| @params(3, "v3") | ||
| @params(3, "v3") |
Comment on lines
+91
to
96
| private assertFrame(frame: UrlLinkFrame, text: string) { | ||
| assert.ok(frame); | ||
| assert.strictEqual(frame.frameClassType, FrameClassType.UrlLinkFrame); | ||
| assert.strictEqual(frame.frameId, FrameIdentifiers.WCOM); | ||
| assert.strictEqual(frame.text, "foo"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR splits the ID3v2 frame factory code into fromFile and fromRawData (more or less). This includes a large refactoring of the frame construction code. All frame classes now have a
fromFieldBytes(header, fieldBytes, version)method that replacesfromOffsetRawData. The offset wasn't ever very effective, nor was multiple places where headers were being read. Frame extended header fields were moved to the header class as was the extended header reading logic. The tests were completely updated to be waaaaay better.