Fix numeric genres (#123)

* test suite for single term in a genre frame

* O(n) processing of genre fields!

* (CR) and (RX) support
Tests for multiple terms.

* Tests for rendering.

* Remove old tests that broke as part of fixes

* Address copilot comments

Author: benrr101

src/genres.ts

@@ -231,6 +231,22 @@ export default class Genres {
             ? Genres.AUDIO_GENRES[safeIndex]
             : undefined;
     }
+    
+    public static indexToAudioDirect(index: number|string): string {
+        if (typeof(index) === "string") {
+            if (!(/\d+/).test(index)) {
+                // Non numeric string
+                return undefined;
+            }
+
+            // Numeric string
+            const indexNumber = Number(index);
+            return Genres.AUDIO_GENRES[indexNumber];
+        }
+
+        // Number
+        return Genres.AUDIO_GENRES[index];
+    }
 
     /**
      * Gets the video genre name for a specified index.
@@ -266,7 +282,13 @@ export default class Genres {
             const trimRegex = /^\(+|\)+$/g;
             text = text.replace(trimRegex, "");
         }
-        const index = parseInt(text, 10);
+
+        const normalized = text.trim();
+        if (!/^\d+$/.test(normalized)) {
+            return 255;
+        }
+
+        const index = parseInt(normalized, 10);
         return Number.isNaN(index) ? 255 : index;
     }
 }

src/id3v2/frames/textInformationFrame.ts

@@ -4,7 +4,7 @@ import {ByteVector, StringType} from "../../byteVector";
 import {Frame, FrameClassType} from "./frame";
 import {Id3v2FrameHeader} from "./frameHeader";
 import {FrameIdentifier, FrameIdentifiers} from "../frameIdentifiers";
-import {Guards, StringComparison, StringUtils} from "../../utils";
+import {Guards, StringComparison} from "../../utils";
 
 /**
  * This class provides support for ID3v2 text information frames (section 4.2) covering `T000` to
@@ -419,65 +419,29 @@ export class TextInformationFrame extends Frame {
                 // * (CR) - "Cover"
                 // * (( - used to escape a "(" in a refinement/genre name
 
-                // NOTE: This encoding has an inherent flaw around how multiple genres should be
-                //    encoded. Since multiple genres are already an edge case, I'm just going to
-                //    say yolo to this whole block of code copied over from the .NET implementation
-                while (value.length > 1 && value[0] === "(") {
-                    const closing = value.indexOf(")");
-                    if (closing < 0) {
-                        break;
+                // Treat each term separately
+                const terms = Id3v2Settings.useNonStandardV2V3GenreSeparators
+                    ? value.split(/[;\/]/).filter(t => !!t)
+                    : [value];
+                for (const term of terms) {
+                    // Attempt to process it according to our best understanding of the spec
+                    const numericGenres = this.parseTconAsStandardNumeric(term);
+                    if (numericGenres !== undefined) {
+                        fieldList.push(... numericGenres);
+                        continue;
                     }
 
-                    const number = value.substring(1, closing);
-
-                    let text: string;
-                    if (number === TextInformationFrame.COVER_ABBREV) {
-                        text = TextInformationFrame.COVER_STRING;
-                    } else if (number === TextInformationFrame.REMIX_ABBREV) {
-                        text = TextInformationFrame.REMIX_STRING;
-                    } else {
-                        text = Genres.indexToAudio(number, true);
-                    }
-
-                    if (!text) {
-                        // Number in parentheses was not a numeric genre but part of a larger bit
-                        // of text?
-                        break;
-                    }
-
-                    // Number in parentheses was a numeric genre
-                    fieldList.push(text);
-                    value = StringUtils.trimStart(value.substring(closing + 1), "/ ");
-
-                    // Ignore genre if the same genre appears after the numeric genre
-                    if (value.startsWith(text)) {
-                        value = StringUtils.trimStart(value.substring(text.length), "/ ");
-                    }
-                }
-
-                // Process whatever's left
-                if (value.length > 0) {
-                    // Split the remaining genre value by dividers if the setting is turned on
-                    let splitValue = Id3v2Settings.useNonStandardV2V3GenreSeparators
-                        ? value.split(/[\/;]/).map((v) => v.trim()).filter((v) => !!v)
-                        : [value];
-
-                    splitValue = splitValue.map((v) => {
-                        // Unescape escaped opening parenthesis
-                        let v2 = v.replace(/\(\(/, "(");
-
-                        // If non-standard numeric genres is enabled, parse them
-                        if (Id3v2Settings.useNonStandardV2V3NumericGenres) {
-                            const text = Genres.indexToAudio(v2, false);
-                            if (text) {
-                                v2 = text;
-                            }
+                    if (Id3v2Settings.useNonStandardV2V3NumericGenres) {
+                        // Attempt to process it as a non-standard numeric genre
+                        const numericGenre = Genres.indexToAudioDirect(term);
+                        if (numericGenre !== undefined) {
+                            fieldList.push(numericGenre);
+                            continue;
                         }
+                    }
 
-                        return v2;
-                    });
-
-                    fieldList.push(...splitValue);
+                    // Yeah, we can't do anything smart, just treat it as a string
+                    fieldList.push(term);
                 }
             } else {
                 fieldList.push(value);
@@ -558,24 +522,28 @@ export class TextInformationFrame extends Frame {
             const numericGenres = [];
             const textGenres = [];
             for (const s of text) {
-                switch (s) {
-                    case TextInformationFrame.COVER_STRING:
-                        numericGenres.push(`(${TextInformationFrame.COVER_ABBREV})`);
-                        break;
-                    case TextInformationFrame.REMIX_STRING:
-                        numericGenres.push(`(${TextInformationFrame.REMIX_ABBREV})`);
-                        break;
-                    default:
-                        if (Id3v2Settings.useNumericGenres) {
+                if (Id3v2Settings.useNumericGenres) {
+                    // Try to process it as a numeric genre
+                    switch (s) {
+                        case TextInformationFrame.COVER_STRING:
+                            numericGenres.push(`(${TextInformationFrame.COVER_ABBREV})`);
+                            continue;
+                        case TextInformationFrame.REMIX_STRING:
+                            numericGenres.push(`(${TextInformationFrame.REMIX_ABBREV})`);
+                            continue;
+                        default:
                             const numericGenre = Genres.audioToIndex(s);
                             if (numericGenre !== 255) {
                                 numericGenres.push(`(${numericGenre})`);
-                                break;
+                                continue;
                             }
-                        }
-                        textGenres.push(s.replace(/\(/, "(("));
-                        break;
+                            break;
+                    }
                 }
+
+                // Process it as a text genre
+                const escapedGenre = s.replace(/\(/g, "((");
+                textGenres.push(escapedGenre);
             }
 
             // Put the entire string together
@@ -590,6 +558,101 @@ export class TextInformationFrame extends Frame {
     }
 
     // #endregion
+
+    private parseTconAsStandardNumeric(field: string): string[]|undefined {
+        // Don't even bother setting up the state machine if we aren't starting with an opening
+        // parenthesis.
+        if (field[0] !== "(") {
+            return undefined;
+        }
+
+        const results: string[] = [];
+        let inParentheses = true;
+        let refinementAdded = false;
+        let open = 0;
+        let close = 0;
+
+        const appendToLastResult = (chunk: string): void => {
+            if (!chunk) {
+                return;
+            }
+
+            const lastResult = results[results.length - 1];
+            results[results.length - 1] = refinementAdded
+                ? `${lastResult}${chunk}`
+                : `${lastResult} ${chunk}`;
+        }
+
+        for (let i = 1; i < field.length; i++) {
+            if (inParentheses) {
+                // Inside parentheses ----------------------------------
+                if (field[i] === ")") {
+                    // Closing parenthesis found
+                    close = i;
+
+                    // Attempt to parse the inside as a number
+                    const parenContents = field.substring(open + 1, close);
+                    const numericGenre = Genres.indexToAudioDirect(parenContents);
+                    if (numericGenre !== undefined) {
+                        results.push(numericGenre);
+                    } else if (parenContents === TextInformationFrame.COVER_ABBREV) {
+                        results.push(TextInformationFrame.COVER_STRING);
+                    } else if (parenContents === TextInformationFrame.REMIX_ABBREV) {
+                        results.push(TextInformationFrame.REMIX_STRING);
+                    } else {
+                        // What we expected to be a numeric genre was not. We will assume this
+                        // field is not using standard numeric genres, and dump the remainder.
+                        break;
+                    }
+
+                    // Transition to refinement processing
+                    inParentheses = false;
+                    refinementAdded = false;
+                    open = i + 1;
+                }
+
+                // If we didn't find the closing paren, just increment and try again.
+            } else {
+                // Processing refinement  ------------------------------
+                let refinementChunk: string;
+                if (field[i] === "(") {
+                    if (field[i + 1] === "(") {
+                        // This is an escape sequence
+                        // Take the current refinement chunk + the first paren (eg: `xyz(`)
+                        refinementChunk = field.substring(open, i + 1);
+
+
+                        // Skip over the next character (ie, `(`)
+                        open = i + 2;
+                        i++;
+                    } else {
+                        // This is possibly the start of a numeric genre.
+                        // Take the current refinement chunk
+                        refinementChunk = field.substring(open, i);
+
+                        // Transition back to numeric genre processing.
+                        inParentheses = true;
+                        open = i;
+                    }
+
+                    // Add the refinement chunk to the last result
+                    appendToLastResult(refinementChunk);
+                    refinementAdded = true;
+                }
+
+                // If we didn't find an opening paren, just increment and try again
+            }
+        }
+
+        // Process the remainder
+        // If we didn't find any results, then just return undefined.
+        if (results.length === 0) {
+            return undefined;
+        }
+
+        appendToLastResult(field.substring(open));
+        return results;
+    }
 }
 
 export class UserTextInformationFrame extends TextInformationFrame {