From d03f2f6347765f619dadc575134c0925d2a987d1 Mon Sep 17 00:00:00 2001 From: TautCony Date: Wed, 8 Jul 2026 13:13:12 +0800 Subject: [PATCH 01/17] refactor: remove IExpressionService and related tests - Deleted IExpressionService interface and its implementation. - Updated RuntimeChapterSaveServiceTests and ChapterExportServiceTests to remove dependencies on ExpressionService. - Added UVa-12803 input/output fixtures for expression evaluation. - Removed ExpressionServiceTests and integrated Lua expression evaluation tests. - Enhanced LuaExpressionScriptServiceTests to cover arithmetic and math library functions. --- .../Cli/ChapterToolCliApplication.cs | 2 +- .../Exporting/ChapterExportService.cs | 10 - .../ChapterOutputProjectionService.cs | 9 - src/ChapterTool.Core/README.md | 82 ++- .../Transform/ChapterExpressionService.cs | 9 - .../Transform/ExpressionEvaluationResult.cs | 14 - .../Transform/ExpressionService.cs | 582 ------------------ .../Transform/IExpressionService.cs | 25 - .../RuntimeChapterSaveServiceTests.cs | 4 +- .../Exporting/ChapterExportServiceTests.cs | 2 +- .../Transform/{expression.in => UVa-12803.in} | 0 .../{expression.out => UVa-12803.out} | 0 .../Importing/CueImporterTests.cs | 4 +- .../Transform/ExpressionServiceTests.cs | 182 ------ .../LuaExpressionScriptServiceTests.cs | 56 ++ 15 files changed, 100 insertions(+), 881 deletions(-) delete mode 100644 src/ChapterTool.Core/Transform/ExpressionEvaluationResult.cs delete mode 100644 src/ChapterTool.Core/Transform/ExpressionService.cs delete mode 100644 src/ChapterTool.Core/Transform/IExpressionService.cs rename tests/ChapterTool.Core.Tests/Fixtures/Transform/{expression.in => UVa-12803.in} (100%) rename tests/ChapterTool.Core.Tests/Fixtures/Transform/{expression.out => UVa-12803.out} (100%) delete mode 100644 tests/ChapterTool.Core.Tests/Transform/ExpressionServiceTests.cs diff --git a/src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs b/src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs index f8a4f620..e96e0a46 100644 --- a/src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs +++ b/src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs @@ -16,7 +16,7 @@ public sealed class ChapterToolCliApplication( { private readonly ICliConsole console = console ?? new SystemCliConsole(); private readonly RuntimeChapterImporterRegistry importerRegistry = importerRegistry ?? CreateImporterRegistry(); - private readonly ChapterExportService exporter = exporter ?? new ChapterExportService(new Core.Transform.ChapterTimeFormatter(), new Core.Transform.ExpressionService()); + private readonly ChapterExportService exporter = exporter ?? new ChapterExportService(new Core.Transform.ChapterTimeFormatter()); public int ShowFormats() { diff --git a/src/ChapterTool.Core/Exporting/ChapterExportService.cs b/src/ChapterTool.Core/Exporting/ChapterExportService.cs index 8ebfe30d..1e7c0ed2 100644 --- a/src/ChapterTool.Core/Exporting/ChapterExportService.cs +++ b/src/ChapterTool.Core/Exporting/ChapterExportService.cs @@ -32,16 +32,6 @@ public ChapterExportService(IChapterTimeFormatter timeFormatter, ILuaExpressionS chapterConversionService = new ChapterConversionService(timeFormatter); } - /// - /// Exports ChapterTool chapter data to supported chapter formats. - /// - /// The chapter time formatter. - /// The _ value. - public ChapterExportService(IChapterTimeFormatter timeFormatter, IExpressionService _) - : this(timeFormatter, new LuaExpressionScriptService()) - { - } - /// /// Executes the Export operation. /// diff --git a/src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs b/src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs index f646c3da..01ae43dc 100644 --- a/src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs +++ b/src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs @@ -20,15 +20,6 @@ public ChapterOutputProjectionService(ILuaExpressionScriptService? luaExpression this.luaExpressionService = luaExpressionService ?? new LuaExpressionScriptService(); } - /// - /// Projects chapter output data before export by applying expressions, ordering, and naming options. - /// - /// The _ value. - public ChapterOutputProjectionService(IExpressionService _) - : this(new LuaExpressionScriptService()) - { - } - /// /// Executes the Project operation. /// diff --git a/src/ChapterTool.Core/README.md b/src/ChapterTool.Core/README.md index 3a04199d..372019de 100644 --- a/src/ChapterTool.Core/README.md +++ b/src/ChapterTool.Core/README.md @@ -13,8 +13,8 @@ dotnet add package ChapterTool.Core - **Import** chapters from common chapter formats and media-container adapters: CUE, FLAC, TAK, IFO, MPLS, XPL, MP4/media containers via `IMediaChapterReader`, OGM, Matroska XML, WebVTT, plain text, Premiere markers - **Export** chapters to multiple chapter formats: OGM Text, Matroska XML, QPFile, TimeCodes, tsMuxeR Meta, CUE, JSON, WebVTT, Celltimes, Chapter→QPFile - **Edit** chapter data: time edit, frame edit, rename, delete, insert, reorder, shift, apply name templates -- **Transform** chapter times: infix math expression engine, Lua scripting engine, frame rate detection and conversion -- **Combine & append** chapter segments from multi-part sources (MPLS/DVD) +- **Transform** chapter times: Lua expression scripting, frame rate detection and conversion +- **Combine & append** chapter segments from multipart sources (MPLS/DVD) ## Quick Start @@ -22,15 +22,15 @@ dotnet add package ChapterTool.Core ```csharp using ChapterTool.Core.Importing; -using ChapterTool.Core.Importing.Cue; +using ChapterTool.Core.Importing.Disc; using ChapterTool.Core.Models; using ChapterTool.Core.Transform; var formatter = new ChapterTimeFormatter(); -// CUE files don't require extra dependencies -IChapterImporter importer = new CueChapterImporter(); -var request = new ChapterImportRequest("/path/to/chapters.cue"); +// MPLS playlists can expose one option per play item/angle. +IChapterImporter importer = new MplsChapterImporter(); +var request = new ChapterImportRequest("/path/to/BDMV/PLAYLIST/00001.mpls"); var result = await importer.ImportAsync(request, CancellationToken.None); if (result.Success) @@ -99,34 +99,29 @@ var detected = fpsService.Detect(info, tolerance: 0.001m); // Change frame rate var fpsResult = ChapterFpsTransformService.ChangeFps(info, sourceFps: 23.976m, targetFps: 25m); -// Evaluate a mathematical expression against chapter times -var exprService = new ExpressionService(); -var eval = exprService.EvaluateInfix("t + 1.0", timeSeconds: 60m, framesPerSecond: 23.976m); - -// Use Lua scripting for advanced transforms var luaService = new LuaExpressionScriptService(); var context = new LuaExpressionContext(chapter, index: 1, count: 10, timeSeconds: 60m, framesPerSecond: 23.976m); -var luaResult = luaService.Evaluate("return t + 1", context); +var luaResult = luaService.Evaluate("t + 1.0", context); ``` ## Supported Formats ### Import -| Format | Importer | Extensions | -|--------|----------|------------| -| CUE Sheet | `CueChapterImporter` | `.cue` | -| FLAC CUE | `FlacCueImporter` | `.flac` | -| TAK CUE | `TakCueImporter` | `.tak` | -| DVD IFO | `IfoChapterImporter` | `.ifo` | -| Blu-ray MPLS | `MplsChapterImporter` | `.mpls` | -| Blu-ray XPL | `XplChapterImporter` | `.xpl` | +| Format | Importer | Extensions | +|----------------------------------|-------------------------------------------------------------------|--------------| +| CUE Sheet | `CueChapterImporter` | `.cue` | +| FLAC CUE | `FlacCueImporter` | `.flac` | +| TAK CUE | `TakCueImporter` | `.tak` | +| DVD IFO | `IfoChapterImporter` | `.ifo` | +| Blu-ray MPLS | `MplsChapterImporter` | `.mpls` | +| Blu-ray XPL | `XplChapterImporter` | `.xpl` | | Media files, including MP4 / M4V | `MediaChapterImporter` with caller-supplied `IMediaChapterReader` | configurable | -| OGM Text | `OgmChapterImporter` | `.txt` | -| Matroska XML | `XmlChapterImporter` | `.xml` | -| WebVTT | `WebVttChapterImporter` | `.vtt` | -| Plain Text | `TextChapterImporter` | `.txt` | -| Premiere Markers | `PremiereMarkerListImporter` | `.csv` | +| OGM Text | `OgmChapterImporter` | `.txt` | +| Matroska XML | `XmlChapterImporter` | `.xml` | +| WebVTT | `WebVttChapterImporter` | `.vtt` | +| Plain Text | `TextChapterImporter` | `.txt` | +| Premiere Markers | `PremiereMarkerListImporter` | `.csv` | ### Media Reader Adapters @@ -163,29 +158,28 @@ ValueTask ReadAsync(string path, CancellationToken cance ### Export -| Format | `ChapterExportFormat` | -|--------|----------------------| -| OGM Text | `Txt` | -| Matroska XML | `Xml` | -| QPFile | `Qpfile` | -| TimeCodes | `TimeCodes` | -| tsMuxeR Meta | `TsMuxerMeta` | -| CUE Sheet | `Cue` | -| JSON | `Json` | -| WebVTT | `WebVtt` | -| Celltimes | `Celltimes` | -| Chapter → QPFile | `Chapter2Qpfile` | +| Format | `ChapterExportFormat` | +|------------------|-----------------------| +| OGM Text | `Txt` | +| Matroska XML | `Xml` | +| QPFile | `Qpfile` | +| TimeCodes | `TimeCodes` | +| tsMuxeR Meta | `TsMuxerMeta` | +| CUE Sheet | `Cue` | +| JSON | `Json` | +| WebVTT | `WebVtt` | +| Celltimes | `Celltimes` | +| Chapter → QPFile | `Chapter2Qpfile` | ## Expression Engine -`ExpressionService` supports infix mathematical expressions with: - -- **Variables**: `t` (time in seconds), `fps` (frames per second) -- **Operators**: `+`, `-`, `*`, `/`, `%`, `^`, `>`, `<`, `>=`, `<=`, ternary `?:` -- **Functions**: `abs`, `acos`, `asin`, `atan`, `atan2`, `cos`, `sin`, `tan`, `cosh`, `sinh`, `tanh`, `exp`, `log`, `log10`, `sqrt`, `ceil`, `floor`, `round`, `int`, `sign`, `pow`, `max`, `min` -- **Constants**: `M_E`, `M_PI`, `M_LN2`, `M_LN10`, `M_SQRT2`, etc. +`LuaExpressionScriptService` evaluates Lua expression scripts for chapter time transforms. +Simple arithmetic can be entered without `return`, while full scripts may use `return` or define `transform(chapter)`. -For advanced transforms, `LuaExpressionScriptService` provides full Lua scripting with access to chapter properties (`t`, `fps`, `index`, `count`) and the `math` standard library. Built-in presets include identity, time offset, frame rounding, and half-frame shift. +- **Globals**: `t` (time in seconds), `fps` (frames per second), `index`, `count`, and `chapter` +- **Libraries**: safe Lua `math`, `string`, and `table` libraries +- **Aliases**: common math helpers such as `floor`, `ceil`, `round`, `sin`, `sqrt`, and `sign` +- **Presets**: identity, time offset, frame rounding, and half-frame shift ## Diagnostic System diff --git a/src/ChapterTool.Core/Transform/ChapterExpressionService.cs b/src/ChapterTool.Core/Transform/ChapterExpressionService.cs index 9f39a719..0730de45 100644 --- a/src/ChapterTool.Core/Transform/ChapterExpressionService.cs +++ b/src/ChapterTool.Core/Transform/ChapterExpressionService.cs @@ -20,15 +20,6 @@ public ChapterExpressionService(ILuaExpressionScriptService? luaExpressionServic this.luaExpressionService = luaExpressionService ?? new LuaExpressionScriptService(); } - /// - /// Applies expression-based time transforms to chapter data. - /// - /// The _ value. - public ChapterExpressionService(IExpressionService _) - : this(new LuaExpressionScriptService()) - { - } - /// /// Executes the Apply operation. /// diff --git a/src/ChapterTool.Core/Transform/ExpressionEvaluationResult.cs b/src/ChapterTool.Core/Transform/ExpressionEvaluationResult.cs deleted file mode 100644 index 414bd85e..00000000 --- a/src/ChapterTool.Core/Transform/ExpressionEvaluationResult.cs +++ /dev/null @@ -1,14 +0,0 @@ -using ChapterTool.Core.Diagnostics; - -namespace ChapterTool.Core.Transform; - -/// -/// Represents the result of expression evaluation. -/// -/// The Success value. -/// The Value value. -/// The Diagnostics value. -public sealed record ExpressionEvaluationResult( - bool Success, - decimal Value, - IReadOnlyList Diagnostics); diff --git a/src/ChapterTool.Core/Transform/ExpressionService.cs b/src/ChapterTool.Core/Transform/ExpressionService.cs deleted file mode 100644 index be972bb8..00000000 --- a/src/ChapterTool.Core/Transform/ExpressionService.cs +++ /dev/null @@ -1,582 +0,0 @@ -using System.Globalization; -using ChapterTool.Core.Diagnostics; - -namespace ChapterTool.Core.Transform; - -/// -/// Evaluates infix and postfix mathematical expressions for chapter time transforms. -/// -public sealed class ExpressionService : IExpressionService -{ - private static readonly Dictionary Constants = new(StringComparer.Ordinal) - { - ["M_E"] = 2.71828182845904523536m, - ["M_LOG2E"] = 1.44269504088896340736m, - ["M_LOG10E"] = 0.43429448190325182765m, - ["M_PI"] = 3.14159265358979323846m, - ["M_PI_2"] = 1.57079632679489661923m, - ["M_PI_4"] = 0.78539816339744830962m, - ["M_1_PI"] = 0.31830988618379067154m, - ["M_2_PI"] = 0.63661977236758134308m, - ["M_2_SQRTPI"] = 1.12837916709551257390m, - ["M_LN2"] = 0.69314718055994530942m, - ["M_LN10"] = 2.30258509299404568402m, - ["M_SQRT2"] = 1.41421356237309504880m, - ["M_SQRT1_2"] = 0.70710678118654752440m - }; - - private static readonly Dictionary Functions = new(StringComparer.Ordinal) - { - ["abs"] = 1, - ["acos"] = 1, - ["asin"] = 1, - ["atan"] = 1, - ["atan2"] = 2, - ["cos"] = 1, - ["sin"] = 1, - ["tan"] = 1, - ["cosh"] = 1, - ["sinh"] = 1, - ["tanh"] = 1, - ["exp"] = 1, - ["log"] = 1, - ["log10"] = 1, - ["sqrt"] = 1, - ["ceil"] = 1, - ["floor"] = 1, - ["round"] = 1, - ["int"] = 1, - ["sign"] = 1, - ["pow"] = 2, - ["max"] = 2, - ["min"] = 2 - }; - - private static readonly Dictionary Precedence = new(StringComparer.Ordinal) - { - [">"] = -1, - ["<"] = -1, - [">="] = -1, - ["<="] = -1, - ["+"] = 0, - ["-"] = 0, - ["*"] = 1, - ["/"] = 1, - ["%"] = 1, - ["^"] = 2, - ["u+"] = 3, - ["u-"] = 3, - ["?:"] = -2 - }; - - private static readonly HashSet RightAssociativeOperators = new(StringComparer.Ordinal) - { - "^", - "u+", - "u-", - "?:" - }; - - /// - /// Executes the EvaluateInfix operation. - /// - /// The expression text. - /// The chapter time in seconds. - /// The frame rate in frames per second. - /// The operation result. - public ExpressionEvaluationResult EvaluateInfix(string expression, decimal timeSeconds, decimal framesPerSecond) - { - try - { - if (string.IsNullOrWhiteSpace(expression)) - { - expression = "t"; - } - - var postfix = ToPostfix(Tokenize(expression)); - return EvaluatePostfix(postfix, timeSeconds, framesPerSecond); - } - catch (Exception exception) when (exception is InvalidOperationException or FormatException or KeyNotFoundException or OverflowException) - { - return exception is ExpressionException ee - ? Failure(timeSeconds, ee.Code, ee.Message, ee.Arguments) - : Failure(timeSeconds, "InvalidExpression", exception.Message); - } - } - - /// - /// Executes the EvaluatePostfix operation. - /// - /// The tokens value. - /// The chapter time in seconds. - /// The frame rate in frames per second. - /// The operation result. - public ExpressionEvaluationResult EvaluatePostfix(IEnumerable tokens, decimal timeSeconds, decimal framesPerSecond) - { - try - { - var values = new Dictionary(StringComparer.Ordinal) - { - ["t"] = timeSeconds, - ["fps"] = framesPerSecond - }; - var stack = new Stack(); - - foreach (var token in tokens.TakeWhile(token => !token.StartsWith("//", StringComparison.Ordinal)).Where(token => token.Length > 0)) - { - if (decimal.TryParse(token, NumberStyles.Number, CultureInfo.InvariantCulture, out var number)) - { - stack.Push(number); - } - else if (values.TryGetValue(token, out var variable)) - { - stack.Push(variable); - } - else if (Constants.TryGetValue(token, out var constant)) - { - stack.Push(constant); - } - else if (Functions.ContainsKey(token)) - { - ApplyFunction(token, stack); - } - else if (Precedence.ContainsKey(token)) - { - ApplyOperator(token, stack); - } - else if (token is "and" or "or" or "xor") - { - throw new ExpressionException("InvalidExpression.UnsupportedOperator", $"Unsupported operator '{token}'.", Args(("token", token))); - } - else - { - throw new ExpressionException("InvalidExpression.UnknownToken", $"Unknown token '{token}'.", Args(("token", token))); - } - } - - if (stack.Count != 1) - { - throw new ExpressionException("InvalidExpression.Incomplete", "Expression did not reduce to one value."); - } - - return new ExpressionEvaluationResult(true, stack.Pop(), []); - } - catch (Exception exception) when (exception is InvalidOperationException or DivideByZeroException or OverflowException) - { - return exception is ExpressionException ee - ? Failure(timeSeconds, ee.Code, ee.Message, ee.Arguments) - : Failure(timeSeconds, "InvalidExpression", exception.Message); - } - } - - private static ExpressionEvaluationResult Failure(decimal fallback, string code, string message, IReadOnlyDictionary? arguments = null) => - new( - false, - fallback, - [ - new ChapterDiagnostic(DiagnosticSeverity.Warning, code, message, Arguments: arguments) - ]); - - private static IReadOnlyDictionary? Args(params (string Name, object? Value)[] pairs) => - pairs.Length == 0 ? null : pairs.ToDictionary(static pair => pair.Name, static pair => pair.Value, StringComparer.Ordinal); - - private sealed class ExpressionException : InvalidOperationException - { - /// - /// Executes the ExpressionException operation. - /// - /// The diagnostic code. - /// The diagnostic message. - /// The arguments value. - /// The innerException value. - public ExpressionException(string code, string message, IReadOnlyDictionary? arguments = null, Exception? innerException = null) - : base(message, innerException) - { - Code = code; - Arguments = arguments; - } - - /// - /// Gets the Code value. - /// - public string Code { get; } - /// - /// Provides the object value. - /// - public IReadOnlyDictionary? Arguments { get; } - } - - private static List Tokenize(string expression) - { - var tokens = new List(); - for (var i = 0; i < expression.Length;) - { - var c = expression[i]; - if (char.IsWhiteSpace(c)) - { - i++; - continue; - } - - if (c == '/' && i + 1 < expression.Length && expression[i + 1] == '/') - { - break; - } - - if (char.IsDigit(c) || c == '.') - { - var start = i++; - while (i < expression.Length && (char.IsDigit(expression[i]) || expression[i] == '.')) - { - i++; - } - - tokens.Add(expression[start..i]); - continue; - } - - if (char.IsLetter(c) || c == '_') - { - var start = i++; - while (i < expression.Length && (char.IsLetterOrDigit(expression[i]) || expression[i] == '_')) - { - i++; - } - - tokens.Add(expression[start..i]); - continue; - } - - if (c is '>' or '<' && i + 1 < expression.Length && expression[i + 1] == '=') - { - tokens.Add(expression.Substring(i, 2)); - i += 2; - continue; - } - - if ("()+-*/%^,<>\u003f:".Contains(c, StringComparison.Ordinal)) - { - tokens.Add(c.ToString(CultureInfo.InvariantCulture)); - i++; - continue; - } - - throw new ExpressionException("InvalidExpression.InvalidCharacter", $"Invalid character '{c}'.", Args(("character", c))); - } - - return tokens; - } - - private static List ToPostfix(IReadOnlyList tokens) - { - var output = new List(); - var operators = new Stack(); - var previous = string.Empty; - - foreach (var token in tokens) - { - if (decimal.TryParse(token, NumberStyles.Number, CultureInfo.InvariantCulture, out _) || - token is "t" or "fps" || - Constants.ContainsKey(token)) - { - if (previous.Length > 0 && - previous != "(" && - previous != "," && - previous != "?" && - previous != ":" && - !Precedence.ContainsKey(previous)) - { - throw new ExpressionException("InvalidExpression.MissingOperator", $"Missing operator before '{token}'.", Args(("token", token))); - } - - output.Add(token); - } - else if (Functions.ContainsKey(token)) - { - if (previous is not "" and not "(" and not "," and not "?" and not ":" && !Precedence.ContainsKey(previous)) - { - throw new ExpressionException("InvalidExpression.MissingOperatorBeforeFunction", $"Missing operator before function '{token}'.", Args(("token", token))); - } - - operators.Push(token); - } - else switch (token) - { - case "," when previous.Length == 0 || - previous == "(" || - previous == "," || - Precedence.ContainsKey(previous) || - Functions.ContainsKey(previous) || - previous == "?": - throw new ExpressionException("InvalidExpression.MisplacedComma", "Misplaced comma."); - case ",": - { - while (operators.Count > 0 && operators.Peek() != "(") - { - output.Add(operators.Pop()); - } - - if (operators.Count == 0) - { - throw new ExpressionException("InvalidExpression.MisplacedComma", "Misplaced comma."); - } - - break; - } - case "(" when previous.Length > 0 && - previous != "(" && - previous != "," && - previous != "?" && - previous != ":" && - !Precedence.ContainsKey(previous) && - !Functions.ContainsKey(previous): - throw new ExpressionException("InvalidExpression.MissingOperatorBeforeParen", "Missing operator before '('."); - case "(": - operators.Push(token); - break; - case ")" when previous.Length == 0 || - previous == "(" || - previous == "," || - Precedence.ContainsKey(previous) || - Functions.ContainsKey(previous) || - previous == "?": - throw new ExpressionException("InvalidExpression.MissingOperandBeforeParen", "Missing operand before ')'."); - case ")": - { - while (operators.Count > 0 && operators.Peek() != "(") - { - output.Add(operators.Pop()); - } - - if (operators.Count == 0) - { - throw new ExpressionException("InvalidExpression.UnbalancedParentheses", "Unbalanced parentheses."); - } - - operators.Pop(); - if (operators.Count > 0 && Functions.ContainsKey(operators.Peek())) - { - output.Add(operators.Pop()); - } - - break; - } - case "?" when previous.Length == 0 || - previous == "(" || - previous == "," || - previous == "?" || - Precedence.ContainsKey(previous) || - Functions.ContainsKey(previous): - throw new ExpressionException("InvalidExpression.TernaryMissingCondition", "Operator '?' requires a condition."); - case "?": - { - while (operators.Count > 0 && Precedence.TryGetValue(operators.Peek(), out var lastPrecedence) && - ShouldPopOperator(lastPrecedence, "?:")) - { - output.Add(operators.Pop()); - } - - operators.Push(token); - break; - } - case ":" when previous.Length == 0 || - previous == "(" || - previous == "," || - previous == "?" || - Precedence.ContainsKey(previous) || - Functions.ContainsKey(previous): - throw new ExpressionException("InvalidExpression.TernaryMissingTrueExpression", "Operator ':' requires a true expression."); - case ":": - { - while (operators.Count > 0 && operators.Peek() != "?" && operators.Peek() != "(") - { - output.Add(operators.Pop()); - } - - if (operators.Count == 0 || operators.Peek() != "?") - { - throw new ExpressionException("InvalidExpression.TernaryUnmatchedColon", "Operator ':' requires a matching '?'."); - } - - operators.Pop(); - - while (operators.Count > 0 && Precedence.TryGetValue(operators.Peek(), out var lastPrecedence) && - ShouldPopOperator(lastPrecedence, "?:")) - { - output.Add(operators.Pop()); - } - - operators.Push("?:"); - break; - } - default: - { - if (Precedence.ContainsKey(token)) - { - var isUnarySign = token is "-" or "+" && - (previous.Length == 0 || previous is "(" or "," or "?" or ":" || Precedence.ContainsKey(previous)); - var operatorToken = isUnarySign ? $"u{token}" : token; - - if (!isUnarySign && - (previous.Length == 0 || previous == "(" || previous == "," || previous == "?" || Precedence.ContainsKey(previous))) - { - throw new ExpressionException("InvalidExpression.OperatorRequiresLeftOperand", $"Operator '{token}' requires a left operand.", Args(("token", token))); - } - - while (operators.Count > 0 && Precedence.TryGetValue(operators.Peek(), out var lastPrecedence) && - ShouldPopOperator(lastPrecedence, operatorToken)) - { - output.Add(operators.Pop()); - } - - operators.Push(operatorToken); - } - else - { - throw new ExpressionException("InvalidExpression.UnsupportedToken", $"Unsupported token '{token}'.", Args(("token", token))); - } - - break; - } - } - - previous = token; - } - - if (previous == ",") - { - throw new ExpressionException("InvalidExpression.MisplacedComma", "Misplaced comma."); - } - - if (previous == ":") - { - throw new ExpressionException("InvalidExpression.InsufficientOperands", "Operator ':' requires a false expression.", Args(("token", previous))); - } - - if (Precedence.ContainsKey(previous)) - { - throw new ExpressionException("InvalidExpression.InsufficientOperands", $"Token '{previous}' requires more operands.", Args(("token", previous))); - } - - while (operators.Count > 0) - { - var token = operators.Pop(); - if (token is "(" or "?") - { - throw token == "(" - ? new ExpressionException("InvalidExpression.UnbalancedParentheses", "Unbalanced parentheses.") - : new ExpressionException("InvalidExpression.TernaryUnmatchedQuestion", "Operator '?' requires a matching ':'."); - } - - output.Add(token); - } - - return output; - } - - private static bool ShouldPopOperator(int previousPrecedence, string currentOperator) - { - return previousPrecedence > Precedence[currentOperator] || - (previousPrecedence == Precedence[currentOperator] && !RightAssociativeOperators.Contains(currentOperator)); - } - - private static void ApplyOperator(string op, Stack stack) - { - switch (op) - { - case "u+" or "u-": - { - var value = Pop(stack, op); - stack.Push(op == "u-" ? -value : value); - return; - } - case "?:": - { - var falseValue = Pop(stack, op); - var trueValue = Pop(stack, op); - var condition = Pop(stack, op); - stack.Push(condition == 0 ? falseValue : trueValue); - return; - } - } - - var rhs = Pop(stack, op); - var lhs = Pop(stack, op); - stack.Push(op switch - { - "+" => lhs + rhs, - "-" => lhs - rhs, - "*" => lhs * rhs, - "/" => lhs / rhs, - "%" => lhs % rhs, - "^" => (decimal)Math.Pow((double)lhs, (double)rhs), - ">" => lhs > rhs ? 1 : 0, - "<" => lhs < rhs ? 1 : 0, - ">=" => lhs >= rhs ? 1 : 0, - "<=" => lhs <= rhs ? 1 : 0, - _ => throw new ExpressionException("InvalidExpression.UnsupportedOperator", $"Unsupported operator '{op}'.", Args(("token", op))) - }); - } - - private static void ApplyFunction(string function, Stack stack) - { - stack.Push(function switch - { - "abs" => Math.Abs(Pop(stack, function)), - "acos" => Unary(Math.Acos), - "asin" => Unary(Math.Asin), - "atan" => Unary(Math.Atan), - "atan2" => Binary(Math.Atan2), - "cos" => Unary(Math.Cos), - "sin" => Unary(Math.Sin), - "tan" => Unary(Math.Tan), - "cosh" => Unary(Math.Cosh), - "sinh" => Unary(Math.Sinh), - "tanh" => Unary(Math.Tanh), - "exp" => Unary(Math.Exp), - "log" => Unary(Math.Log), - "log10" => Unary(Math.Log10), - "sqrt" => Unary(Math.Sqrt), - "ceil" => Math.Ceiling(Pop(stack, function)), - "floor" => Math.Floor(Pop(stack, function)), - "round" => Math.Round(Pop(stack, function)), - "int" => Math.Truncate(Pop(stack, function)), - "sign" => Math.Sign(Pop(stack, function)), - "pow" => Binary(Math.Pow), - "max" => Max(PopPair(stack, function)), - "min" => Min(PopPair(stack, function)), - _ => throw new ExpressionException("InvalidExpression.UnsupportedFunction", $"Unsupported function '{function}'.", Args(("function", function))) - }); - return; - - decimal Binary(Func func) - { - var rhs = Pop(stack, function); - var lhs = Pop(stack, function); - return (decimal)func((double)lhs, (double)rhs); - } - - decimal Unary(Func func) => (decimal)func((double)Pop(stack, function)); - } - - private static decimal Max((decimal Left, decimal Right) pair) => Math.Max(pair.Left, pair.Right); - - private static decimal Min((decimal Left, decimal Right) pair) => Math.Min(pair.Left, pair.Right); - - private static (decimal Left, decimal Right) PopPair(Stack stack, string token) - { - var rhs = Pop(stack, token); - var lhs = Pop(stack, token); - return (lhs, rhs); - } - - private static decimal Pop(Stack stack, string token) - { - if (!stack.TryPop(out var value)) - { - throw new ExpressionException("InvalidExpression.InsufficientOperands", $"Token '{token}' requires more operands.", Args(("token", token))); - } - - return value; - } -} diff --git a/src/ChapterTool.Core/Transform/IExpressionService.cs b/src/ChapterTool.Core/Transform/IExpressionService.cs deleted file mode 100644 index 35362ca4..00000000 --- a/src/ChapterTool.Core/Transform/IExpressionService.cs +++ /dev/null @@ -1,25 +0,0 @@ -namespace ChapterTool.Core.Transform; - -/// -/// Evaluates mathematical chapter time expressions. -/// -public interface IExpressionService -{ - /// - /// Evaluates an infix expression against a chapter time context. - /// - /// The expression text. - /// The chapter time in seconds. - /// The frame rate in frames per second. - /// The expression evaluation result. - ExpressionEvaluationResult EvaluateInfix(string expression, decimal timeSeconds, decimal framesPerSecond); - - /// - /// Evaluates postfix expression tokens against a chapter time context. - /// - /// The postfix expression tokens. - /// The chapter time in seconds. - /// The frame rate in frames per second. - /// The expression evaluation result. - ExpressionEvaluationResult EvaluatePostfix(IEnumerable tokens, decimal timeSeconds, decimal framesPerSecond); -} diff --git a/tests/ChapterTool.Avalonia.Tests/Services/RuntimeChapterSaveServiceTests.cs b/tests/ChapterTool.Avalonia.Tests/Services/RuntimeChapterSaveServiceTests.cs index 8a61f79d..46a55b89 100644 --- a/tests/ChapterTool.Avalonia.Tests/Services/RuntimeChapterSaveServiceTests.cs +++ b/tests/ChapterTool.Avalonia.Tests/Services/RuntimeChapterSaveServiceTests.cs @@ -21,7 +21,7 @@ public async Task RuntimeSaveWritesCueBesideRequestedDirectory() 75, TimeSpan.FromMinutes(1), [new Chapter(1, TimeSpan.Zero, "Intro")]); - var service = new RuntimeChapterSaveService(new ChapterExportService(new ChapterTimeFormatter(), new ExpressionService())); + var service = new RuntimeChapterSaveService(new ChapterExportService(new ChapterTimeFormatter())); try { @@ -54,7 +54,7 @@ public async Task RuntimeSavePreservesExporterDiagnostics() new Chapter(1, TimeSpan.Zero, "Chapter 01"), new Chapter(2, TimeSpan.FromSeconds(30), "Chapter 02") ]); - var service = new RuntimeChapterSaveService(new ChapterExportService(new ChapterTimeFormatter(), new ExpressionService())); + var service = new RuntimeChapterSaveService(new ChapterExportService(new ChapterTimeFormatter())); try { diff --git a/tests/ChapterTool.Core.Tests/Exporting/ChapterExportServiceTests.cs b/tests/ChapterTool.Core.Tests/Exporting/ChapterExportServiceTests.cs index 0a7f3480..1a3ee5bf 100644 --- a/tests/ChapterTool.Core.Tests/Exporting/ChapterExportServiceTests.cs +++ b/tests/ChapterTool.Core.Tests/Exporting/ChapterExportServiceTests.cs @@ -8,7 +8,7 @@ namespace ChapterTool.Core.Tests.Exporting; public sealed class ChapterExportServiceTests { - private readonly ChapterExportService service = new(new ChapterTimeFormatter(), new ExpressionService()); + private readonly ChapterExportService service = new(new ChapterTimeFormatter()); [Fact] public void Txt_export_writes_ogm_pairs() diff --git a/tests/ChapterTool.Core.Tests/Fixtures/Transform/expression.in b/tests/ChapterTool.Core.Tests/Fixtures/Transform/UVa-12803.in similarity index 100% rename from tests/ChapterTool.Core.Tests/Fixtures/Transform/expression.in rename to tests/ChapterTool.Core.Tests/Fixtures/Transform/UVa-12803.in diff --git a/tests/ChapterTool.Core.Tests/Fixtures/Transform/expression.out b/tests/ChapterTool.Core.Tests/Fixtures/Transform/UVa-12803.out similarity index 100% rename from tests/ChapterTool.Core.Tests/Fixtures/Transform/expression.out rename to tests/ChapterTool.Core.Tests/Fixtures/Transform/UVa-12803.out diff --git a/tests/ChapterTool.Core.Tests/Importing/CueImporterTests.cs b/tests/ChapterTool.Core.Tests/Importing/CueImporterTests.cs index e0e2a0aa..84f266ce 100644 --- a/tests/ChapterTool.Core.Tests/Importing/CueImporterTests.cs +++ b/tests/ChapterTool.Core.Tests/Importing/CueImporterTests.cs @@ -239,7 +239,7 @@ public void CueExporterWritesHeaderSkipsSeparatorsAndUsesOutputOrder() new Chapter(-1, Chapter.SeparatorTime, ""), new Chapter(3, TimeSpan.FromSeconds(65.5), "B") ]); - var exporter = new ChapterExportService(new ChapterTimeFormatter(), new ExpressionService()); + var exporter = new ChapterExportService(new ChapterTimeFormatter()); var result = exporter.Export(info, new ChapterExportOptions(ChapterExportFormat.Cue, SourceFileName: "source.wav")); @@ -267,7 +267,7 @@ public void WebVttExporterWritesHeaderAndCuesWithEndTimes() new Chapter(2, TimeSpan.FromSeconds(30), "Main Content"), new Chapter(3, TimeSpan.FromSeconds(90), "Conclusion") ]); - var exporter = new ChapterExportService(new ChapterTimeFormatter(), new ExpressionService()); + var exporter = new ChapterExportService(new ChapterTimeFormatter()); var result = exporter.Export(info, new ChapterExportOptions(ChapterExportFormat.WebVtt)); diff --git a/tests/ChapterTool.Core.Tests/Transform/ExpressionServiceTests.cs b/tests/ChapterTool.Core.Tests/Transform/ExpressionServiceTests.cs deleted file mode 100644 index 3964ecdc..00000000 --- a/tests/ChapterTool.Core.Tests/Transform/ExpressionServiceTests.cs +++ /dev/null @@ -1,182 +0,0 @@ -using ChapterTool.Core.Transform; - -namespace ChapterTool.Core.Tests.Transform; - -public sealed class ExpressionServiceTests -{ - private readonly ExpressionService service = new(); - - [Theory] - [InlineData("t + 1", 11)] - [InlineData("fps / 2", 12)] - [InlineData("floor(1.9) + ceil(1.1)", 3)] - [InlineData("max(1, 3) + min(2, 4)", 5)] - [InlineData("M_PI > 3", 1)] - [InlineData("M_LOG2E > 1", 1)] - [InlineData("1 + (2 * 3)", 7)] - [InlineData("(1 + 2) * 3", 9)] - [InlineData("2 ^ 3 ^ 2", 512)] - [InlineData("2 + 3 * 4 ^ 2", 50)] - [InlineData("-t + +2", -8)] - [InlineData("1 + -2 * 3", -5)] - [InlineData("10 % 3", 1)] - [InlineData("pow(2, 5)", 32)] - [InlineData("1 ? 2 : 3", 2)] - [InlineData("0 ? 2 : 3", 3)] - [InlineData("t > 5 ? t + 1 : fps / 2", 11)] - [InlineData("t < 5 ? t + 1 : fps / 2", 12)] - [InlineData("1 ? 0 ? 2 : 3 : 4", 3)] - [InlineData("0 ? 1 : 0 ? 2 : 3", 3)] - [InlineData("0 ? 1 : -2", -2)] - [InlineData("(0 ? 1 : 2) ^ 3", 8)] - public void EvaluateInfix_supports_documented_tokens(string expression, decimal expected) - { - var result = service.EvaluateInfix(expression, 10, 24); - - Assert.True(result.Success); - Assert.Equal(expected, Math.Round(result.Value, 6)); - } - - [Theory] - [InlineData("1+1/2+1/3+1/4+1/5+1/6+1/7+1/8+1/9+1/10", "2.9289682539682539682539682540")] - [InlineData("1-1/2-1/4+1/8-1/16+1/32-1/64+1/128-1/256+1/512-1/1024", "0.3330078125")] - [InlineData("1-(1/2)-(1/4)+(1/8)-(1/16)+(1/32)-(1/64)+(1/128)-(1/256)+(1/512)-(1/1024)", "0.3330078125")] - [InlineData("2^2^2^2", "65536")] - [InlineData("2^(2^(2^2))", "65536")] - public void EvaluateInfix_matches_legacy_arithmetic_cases(string expression, string expected) - { - var result = service.EvaluateInfix(expression, 0, 0); - - Assert.True(result.Success); - Assert.Equal(decimal.Parse(expected), result.Value); - } - - [Theory] - [InlineData("floor(1.133) + floor(log10(1023)) - ceil(0.9)", 3)] - [InlineData("abs(-1908.8976)", 1908.8976)] - [InlineData("abs(1908.8976)", 1908.8976)] - [InlineData("abs(-1908)", 1908)] - [InlineData("abs(1908)", 1908)] - [InlineData("log10(1000.0)", 3)] - [InlineData("log10(10 ^ 14)", 14)] - public void EvaluateInfix_matches_legacy_function_cases(string expression, decimal expected) - { - var result = service.EvaluateInfix(expression, 0, 0); - - Assert.True(result.Success); - Assert.Equal(expected, Math.Round(result.Value, 10)); - } - - [Theory] - [InlineData("sin(asin(1))", 1)] - [InlineData("cos(acos(1))", 1)] - [InlineData("tan(atan(1))", 1)] - [InlineData("log10(5482.2158)", 3.73895612695404)] - [InlineData("log10(458723662312872.125782332587)", 14.6615511428938)] - [InlineData("log10(0.12348583358871)", -0.908382862219234)] - public void EvaluateInfix_matches_legacy_near_function_cases(string expression, decimal expected) - { - var result = service.EvaluateInfix(expression, 0, 0); - - Assert.True(result.Success); - Assert.InRange(result.Value, expected - 0.0000000001m, expected + 0.0000000001m); - } - - [Fact] - public void EvaluateInfix_stops_at_comment() - { - var result = service.EvaluateInfix("2^10%10 + 6 \t///comment sample", 0, 0); - - Assert.True(result.Success); - Assert.Equal(10, result.Value); - } - - [Fact] - public void EvaluatePostfix_matches_legacy_postfix_case() - { - var result = service.EvaluatePostfix("2 10 ^ 10 % 6 +".Split(), 0, 0); - - Assert.True(result.Success); - Assert.Equal(10, result.Value); - } - - [Fact] - public void EvaluateInfix_matches_legacy_expression_fixture_cases() - { - var input = File.ReadAllLines(FixtureResolver.Fixture("Transform", "expression.in")); - var output = File.ReadAllLines(FixtureResolver.Fixture("Transform", "expression.out")); - - Assert.Equal(input.Length, output.Length); - - for (var i = 0; i < input.Length; i++) - { - var result = service.EvaluateInfix(input[i], 0, 0); - - Assert.True(result.Success, $"Expression #{i + 1} failed: {input[i]}"); - Assert.Equal(output[i], result.Value.ToString("0.00")); - } - } - - [Fact] - public void EvaluatePostfix_stops_at_comment() - { - var result = service.EvaluatePostfix(["t", "1", "+", "//", "100", "*"], 10, 24); - - Assert.True(result.Success); - Assert.Equal(11, result.Value); - } - - [Theory] - [InlineData("t +")] - [InlineData("and(1, 2)")] - [InlineData("1 and 2")] - [InlineData("(t + 1")] - [InlineData("t + 1)")] - [InlineData("max(1 2)")] - [InlineData("max(1, )")] - [InlineData("1 2")] - [InlineData("sin 1")] - [InlineData("1 * * 2")] - [InlineData("1 ? 2")] - [InlineData("1 : 2")] - [InlineData("1 ? : 2")] - [InlineData("1 ? 2 :")] - [InlineData("1 ? 2 : 3 : 4")] - public void Invalid_expression_returns_warning_and_original_time(string expression) - { - var result = service.EvaluateInfix(expression, 10, 24); - - Assert.False(result.Success); - Assert.Equal(10, result.Value); - Assert.StartsWith("InvalidExpression", Assert.Single(result.Diagnostics).Code); - } - - [Theory] - [InlineData("2+", "InvalidExpression.InsufficientOperands")] - [InlineData("2-", "InvalidExpression.InsufficientOperands")] - [InlineData("2*", "InvalidExpression.InsufficientOperands")] - [InlineData("2/", "InvalidExpression.InsufficientOperands")] - [InlineData("2%", "InvalidExpression.InsufficientOperands")] - [InlineData("2^", "InvalidExpression.InsufficientOperands")] - [InlineData("2>", "InvalidExpression.InsufficientOperands")] - [InlineData("2<", "InvalidExpression.InsufficientOperands")] - [InlineData("2>=", "InvalidExpression.InsufficientOperands")] - [InlineData("2<=", "InvalidExpression.InsufficientOperands")] - [InlineData("2?", "InvalidExpression.TernaryUnmatchedQuestion")] - [InlineData("2?3:", "InvalidExpression.InsufficientOperands")] - [InlineData("2?3", "InvalidExpression.TernaryUnmatchedQuestion")] - [InlineData("floor(", "InvalidExpression.UnbalancedParentheses")] - [InlineData("floor()", "InvalidExpression.MissingOperandBeforeParen")] - [InlineData("floor(2,", "InvalidExpression.MisplacedComma")] - [InlineData("(2+", "InvalidExpression.InsufficientOperands")] - [InlineData("+", "InvalidExpression.InsufficientOperands")] - [InlineData("-", "InvalidExpression.InsufficientOperands")] - public void Incomplete_expression_reports_specific_warning_code(string expression, string expectedCode) - { - var result = service.EvaluateInfix(expression, 10, 24); - - Assert.False(result.Success); - Assert.Equal(10, result.Value); - Assert.Equal(expectedCode, Assert.Single(result.Diagnostics).Code); - } -} diff --git a/tests/ChapterTool.Core.Tests/Transform/LuaExpressionScriptServiceTests.cs b/tests/ChapterTool.Core.Tests/Transform/LuaExpressionScriptServiceTests.cs index d0f1bb4f..6fe44562 100644 --- a/tests/ChapterTool.Core.Tests/Transform/LuaExpressionScriptServiceTests.cs +++ b/tests/ChapterTool.Core.Tests/Transform/LuaExpressionScriptServiceTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using ChapterTool.Core.Models; using ChapterTool.Core.Transform; @@ -25,6 +26,30 @@ public void Evaluates_shorthand_arithmetic_without_return() Assert.Equal(11, result.Value); } + [Theory] + [InlineData("t + 1", 11)] + [InlineData("fps / 2", 12)] + [InlineData("math.floor(1.9) + math.ceil(1.1)", 3)] + [InlineData("math.max(1, 3) + math.min(2, 4)", 5)] + [InlineData("math.pi > 3 and 1 or 0", 1)] + [InlineData("1 + (2 * 3)", 7)] + [InlineData("(1 + 2) * 3", 9)] + [InlineData("2 ^ 3 ^ 2", 512)] + [InlineData("2 + 3 * 4 ^ 2", 50)] + [InlineData("-t + 2", -8)] + [InlineData("1 + -2 * 3", -5)] + [InlineData("10 % 3", 1)] + [InlineData("math.pow(2, 5)", 32)] + [InlineData("t > 5 and t + 1 or fps / 2", 11)] + [InlineData("t < 5 and t + 1 or fps / 2", 12)] + public void Evaluates_lua_arithmetic_and_context_tokens(string expression, decimal expected) + { + var result = service.Evaluate(expression, Context(timeSeconds: 10, fps: 24)); + + Assert.True(result.Success, DiagnosticText(result)); + Assert.Equal(expected, Math.Round(result.Value, 6)); + } + [Fact] public void Evaluates_direct_return_script() { @@ -54,6 +79,37 @@ public void Supports_safe_math_aliases_for_low_friction_arithmetic() Assert.Equal(10.5m, result.Value); } + [Theory] + [InlineData("math.sin(math.asin(1))", 1)] + [InlineData("math.cos(math.acos(1))", 1)] + [InlineData("math.tan(math.atan(1))", 1)] + [InlineData("math.log(1000, 10)", 3)] + [InlineData("math.sqrt(81)", 9)] + public void Evaluates_lua_math_library_functions(string expression, decimal expected) + { + var result = service.Evaluate(expression, Context(timeSeconds: 0, fps: 24)); + + Assert.True(result.Success, DiagnosticText(result)); + Assert.InRange(result.Value, expected - 0.0000000001m, expected + 0.0000000001m); + } + + [Fact] + public void Evaluates_uva_12803_expression_fixture_cases_with_lua() + { + var input = File.ReadAllLines(FixtureResolver.Fixture("Transform", "UVa-12803.in")); + var output = File.ReadAllLines(FixtureResolver.Fixture("Transform", "UVa-12803.out")); + + Assert.Equal(input.Length, output.Length); + + for (var i = 0; i < input.Length; i++) + { + var result = service.Evaluate(input[i], Context(timeSeconds: 0, fps: 24)); + + Assert.True(result.Success, $"Expression #{i + 1} failed: {input[i]}{Environment.NewLine}{DiagnosticText(result)}"); + Assert.Equal(output[i], result.Value.ToString("0.00", CultureInfo.InvariantCulture)); + } + } + [Theory] [InlineData("return nil")] [InlineData("return 'bad'")] From ac5cea4f32509c0da0d68bf60eb32618c1c8c42a Mon Sep 17 00:00:00 2001 From: TautCony Date: Wed, 8 Jul 2026 13:44:42 +0800 Subject: [PATCH 02/17] chore: add code-map and remove outdated review documents - Deleted phase review documents for core domain, infrastructure, viewmodels, views, scripts, localization, and cross-cutting analysis. - Removed the summary document detailing the overall code review findings and verdicts. --- AGENTS.md | 44 +-- docs/code-map/README.md | 33 +++ docs/code-map/avalonia.md | 161 ++++++++++ docs/code-map/core.md | 119 ++++++++ docs/code-map/infrastructure.md | 116 ++++++++ docs/code-map/testing.md | 110 +++++++ docs/code-review-2026-07-06.md | 280 ------------------ docs/code-review-fix-todo-2026-07-06.md | 40 --- docs/i18n-audit-and-fix-plan.md | 165 ----------- .../avalonia-modernization-review.md | 0 .../{ => migrations}/avalonia-rewrite-spec.md | 0 docs/{ => migrations}/coverage-matrix.md | 0 .../feature-implementation-progress-report.md | 0 docs/{ => migrations}/gui-verification.md | 0 .../legacy-new-implementation-diff.md | 0 .../modules/01-ui-shell-and-interactions.md | 0 .../modules/02-core-model-transform-export.md | 0 .../03-text-xml-matroska-vtt-importers.md | 0 .../04-disc-playlist-media-importers.md | 0 .../modules/05-cue-flac-tak-importers.md | 0 .../06-supporting-ui-platform-services.md | 0 .../07-tests-build-distribution-assets.md | 0 .../openspec-avalonia-dotnet10-spec-split.md | 0 docs/{ => migrations}/packaging-strategy.md | 0 docs/{ => migrations}/progress.md | 0 .../spec-implementation-verification.md | 0 docs/review-2026-07-05/agent-findings.md | 83 ------ docs/review-2026-07-05/fixes-plan.md | 122 -------- docs/review-2026-07-05/phase-0-baseline.md | 86 ------ docs/review-2026-07-05/phase-1-core.md | 50 ---- .../phase-2-infrastructure.md | 52 ---- docs/review-2026-07-05/phase-3-viewmodels.md | 55 ---- docs/review-2026-07-05/phase-4-views.md | 80 ----- docs/review-2026-07-05/phase-5-scripts.md | 86 ------ .../review-2026-07-05/phase-6-localization.md | 77 ----- .../review-2026-07-05/phase-7-crosscutting.md | 74 ----- docs/review-2026-07-05/summary.md | 107 ------- 37 files changed, 565 insertions(+), 1375 deletions(-) create mode 100644 docs/code-map/README.md create mode 100644 docs/code-map/avalonia.md create mode 100644 docs/code-map/core.md create mode 100644 docs/code-map/infrastructure.md create mode 100644 docs/code-map/testing.md delete mode 100644 docs/code-review-2026-07-06.md delete mode 100644 docs/code-review-fix-todo-2026-07-06.md delete mode 100644 docs/i18n-audit-and-fix-plan.md rename docs/{ => migrations}/avalonia-modernization-review.md (100%) rename docs/{ => migrations}/avalonia-rewrite-spec.md (100%) rename docs/{ => migrations}/coverage-matrix.md (100%) rename docs/{ => migrations}/feature-implementation-progress-report.md (100%) rename docs/{ => migrations}/gui-verification.md (100%) rename docs/{ => migrations}/legacy-new-implementation-diff.md (100%) rename docs/{ => migrations}/modules/01-ui-shell-and-interactions.md (100%) rename docs/{ => migrations}/modules/02-core-model-transform-export.md (100%) rename docs/{ => migrations}/modules/03-text-xml-matroska-vtt-importers.md (100%) rename docs/{ => migrations}/modules/04-disc-playlist-media-importers.md (100%) rename docs/{ => migrations}/modules/05-cue-flac-tak-importers.md (100%) rename docs/{ => migrations}/modules/06-supporting-ui-platform-services.md (100%) rename docs/{ => migrations}/modules/07-tests-build-distribution-assets.md (100%) rename docs/{ => migrations}/openspec-avalonia-dotnet10-spec-split.md (100%) rename docs/{ => migrations}/packaging-strategy.md (100%) rename docs/{ => migrations}/progress.md (100%) rename docs/{ => migrations}/spec-implementation-verification.md (100%) delete mode 100644 docs/review-2026-07-05/agent-findings.md delete mode 100644 docs/review-2026-07-05/fixes-plan.md delete mode 100644 docs/review-2026-07-05/phase-0-baseline.md delete mode 100644 docs/review-2026-07-05/phase-1-core.md delete mode 100644 docs/review-2026-07-05/phase-2-infrastructure.md delete mode 100644 docs/review-2026-07-05/phase-3-viewmodels.md delete mode 100644 docs/review-2026-07-05/phase-4-views.md delete mode 100644 docs/review-2026-07-05/phase-5-scripts.md delete mode 100644 docs/review-2026-07-05/phase-6-localization.md delete mode 100644 docs/review-2026-07-05/phase-7-crosscutting.md delete mode 100644 docs/review-2026-07-05/summary.md diff --git a/AGENTS.md b/AGENTS.md index 5d031fc5..7bed76e6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,9 +1,8 @@ # AGENTS.md -## Dev Environment Tips +## Repository Overview -- This repo contains the legacy WinForms project under `Time_Shift/` and the current .NET 10 Avalonia/Core/Infrastructure solution under `src/`. -- Use `ChapterTool.Avalonia.slnx` for the current Avalonia/Core/Infrastructure solution. +- This repo is the current .NET 10 ChapterTool codebase. Use `ChapterTool.Avalonia.slnx` as the main solution. - Main projects: - `src/ChapterTool.Core` - `src/ChapterTool.Infrastructure` @@ -12,12 +11,20 @@ - `tests/ChapterTool.Infrastructure.Tests` - `tests/ChapterTool.Avalonia.Tests` - Prefer `rg` for searching files and text. -- When reading files with PowerShell, explicitly use UTF-8, for example: - - `Get-Content -Raw -Encoding utf8 path\to\file` - - `[System.IO.File]::ReadAllText($path, [System.Text.Encoding]::UTF8)` -- Keep this file focused on durable repository guidance. Do not add one-off implementation notes, completed change records, or transient archive paths here. -- Do not port WinForms absolute positioning into Avalonia. Use responsive Avalonia layout panels and stable sizing constraints. +- Use `docs/code-map/` as the primary navigation index for the current codebase. Update the relevant files there when feature work changes module ownership, entry points, runtime wiring, or the main tests a maintainer should inspect. - Keep user-facing Chinese strings as valid UTF-8. Validate localization through behavior, rendered UI, or resource-level checks rather than hard-coding incidental mojibake examples. +- Keep this file focused on durable repository guidance. Do not add one-off implementation notes, completed change records, or transient archive paths here. + +## PowerShell Guidance + +- On Windows, prefer `pwsh.exe` over `powershell.exe` unless Windows PowerShell 5.1 is explicitly required. +- For short native-command invocations, pass the executable and arguments separately. Store the executable path in a variable, keep each native argument as one array item, invoke with `&`, and capture `$LASTEXITCODE` immediately. +- For cmdlets and file operations, prefer PowerShell-native commands with splatting and `-LiteralPath` when working with real paths. +- For file operations, prefer explicit path and encoding handling. Use `-LiteralPath` for real paths, specify UTF-8 when reading or writing text, and avoid relying on implicit wildcard expansion or default encodings. + - `Get-Content -Raw -Encoding utf8 -LiteralPath $path` + - `[System.IO.File]::ReadAllText($path, [System.Text.Encoding]::UTF8)` +- For multiline scripts, complex quoting, JSON, XML, regular expressions, or non-ASCII paths, write a temporary `.ps1` file and run it with `pwsh.exe -NoLogo -NoProfile -NonInteractive -File script.ps1`. +- Do not use `Invoke-Expression` for normal task execution. ## OpenSpec Workflow @@ -31,27 +38,28 @@ - After completing and archiving a change, validate all specs: - `openspec validate --all` -## Testing Instructions +## Testing And Build - Run the focused Avalonia tests after XAML or UI shell changes: - `dotnet test tests\ChapterTool.Avalonia.Tests\ChapterTool.Avalonia.Tests.csproj --no-restore` -- Keep Avalonia Headless tests isolated without serializing the entire Avalonia test assembly. Do not reintroduce assembly-level `CollectionBehavior(DisableTestParallelization = true)` for this project; instead put every class containing `[AvaloniaFact]` or `[AvaloniaTheory]` in `AvaloniaHeadlessTestCollection`. The guard test `HeadlessTestCollectionGuardTests` exists to catch missed classes. -- When a test constructs `SettingsToolViewModel` and then calls `LoadAsync` explicitly, pass `autoLoad: false`. Otherwise the constructor starts a background load and the test performs the same initialization twice, which slows Headless runs and can introduce races. -- Do not test source/configuration files by reading them as text and asserting strings. This includes `.cs`, `.axaml`, `.csproj`, scripts, CI YAML, README, and docs. Prefer compiled coverage, behavior tests, runtime verification, structured public APIs, or integration checks. - Run the full solution tests before finalizing broader changes: - `dotnet test ChapterTool.Avalonia.slnx --no-restore` -- Do not run multiple `dotnet test` commands for projects in this solution in parallel. The test projects share referenced project `obj/` outputs, and parallel external test processes can fail with locked files such as `src/ChapterTool.Core/obj/Debug/net10.0/ChapterTool.Core.dll`. Prefer the full solution test command above, or run individual test projects sequentially. - Build the Avalonia app when changing app project files: - `dotnet build src\ChapterTool.Avalonia\ChapterTool.Avalonia.csproj --no-restore` +- If dependencies, target frameworks, or generated project assets change, restore/build once before running no-restore test commands. - The CI workflow is in `.github/workflows/dotnet-ci.yml`. - If a test/build fails because `ChapterTool.Avalonia.exe` is locked, close the running app or run: - `Get-Process ChapterTool.Avalonia -ErrorAction SilentlyContinue | Stop-Process` +- Do not run multiple `dotnet test` commands for projects in this solution in parallel. The test projects share referenced project `obj/` outputs, and parallel external test processes can fail with locked files such as `src/ChapterTool.Core/obj/Debug/net10.0/ChapterTool.Core.dll`. Prefer the full solution test command above, or run individual test projects sequentially. +- Keep Avalonia Headless tests isolated without serializing the entire Avalonia test assembly. Do not reintroduce assembly-level `CollectionBehavior(DisableTestParallelization = true)` for this project; instead put every class containing `[AvaloniaFact]` or `[AvaloniaTheory]` in `AvaloniaHeadlessTestCollection`. The guard test `HeadlessTestCollectionGuardTests` exists to catch missed classes. +- When a test constructs `SettingsToolViewModel` and then calls `LoadAsync` explicitly, pass `autoLoad: false`. Otherwise the constructor starts a background load and the test performs the same initialization twice, which slows Headless runs and can introduce races. +- Do not test source/configuration files by reading them as text and asserting strings. This includes `.cs`, `.axaml`, `.csproj`, scripts, CI YAML, README, and docs. Prefer compiled coverage, behavior tests, runtime verification, structured public APIs, or integration checks. - Add or update tests for changed behavior, especially UI layout constraints, UTF-8 labels, import/export behavior, and platform-service boundaries. -- If dependencies, target frameworks, or generated project assets change, restore/build once before running no-restore test commands. -## UI Implementation Notes +## Avalonia UI Guidelines -- The Avalonia main window should preserve workflow zones, not WinForms pixel geometry: +- Use responsive Avalonia layout panels and stable sizing constraints. Do not rely on absolute positioning for normal workflow controls. +- The Avalonia main window should preserve these workflow zones: - top load/save and frame controls - central chapter grid - bottom options area @@ -63,12 +71,12 @@ - Buttons should center content horizontally and vertically. - Do not expose Windows registry-dependent actions, such as file association, as always-visible primary UI. - When verifying visual layout changes, capture screenshots at default, wide, and narrow sizes and store them under `artifacts/`. -- Avoid static string assertions over source/configuration layout. Validate UI through Avalonia compilation, behavior-level tests, and screenshots/runtime checks instead. - Preserve accessible names, keyboard navigation, focus behavior, and localization boundaries when changing controls. -## PR Instructions +## Change And PR Expectations - Keep changes scoped to the current feature or fix. - Mention the primary test commands run in the PR or final summary. - For UI changes, include screenshot artifact paths when available. +- When a feature change affects code ownership or lookup paths, update the relevant files under `docs/code-map/` in the same change. - Do not revert unrelated user or generated changes in a dirty worktree. diff --git a/docs/code-map/README.md b/docs/code-map/README.md new file mode 100644 index 00000000..1a1d1d25 --- /dev/null +++ b/docs/code-map/README.md @@ -0,0 +1,33 @@ +# ChapterTool Code Map + +This directory is the maintainer navigation index for the current codebase. + +Use it when you need to quickly locate the code behind a feature without repository-wide searching first. + +## Documents + +- `core.md` + - domain models, import/edit/transform/export logic +- `infrastructure.md` + - external tools, process execution, settings persistence, platform services +- `avalonia.md` + - desktop shell, CLI entrypoints, view/viewmodel/runtime service wiring +- `testing.md` + - which test project and test files verify each code area + +## How To Use + +1. Start from the feature you need to change or debug. +2. Open the module document that owns the behavior. +3. Follow the listed entry points before using repository-wide search. +4. Use `testing.md` to find the fastest verification path. + +## Maintenance Rule + +Update these documents in the same change when feature work alters: + +- module ownership +- key entry points +- runtime wiring between modules +- the primary files a maintainer should inspect first +- the primary tests used to verify that area diff --git a/docs/code-map/avalonia.md b/docs/code-map/avalonia.md new file mode 100644 index 00000000..372c51b0 --- /dev/null +++ b/docs/code-map/avalonia.md @@ -0,0 +1,161 @@ +# Avalonia Code Map + +`src/ChapterTool.Avalonia` owns the desktop shell, CLI entrypoints, view/viewmodel coordination, runtime orchestration, localization, and theme application. + +## Ownership + +### Application shell + +Startup and main shell entry points: + +- `src/ChapterTool.Avalonia/Program.cs` +- `src/ChapterTool.Avalonia/App.axaml` +- `src/ChapterTool.Avalonia/App.axaml.cs` +- `src/ChapterTool.Avalonia/Views/MainWindow.axaml` +- `src/ChapterTool.Avalonia/Views/MainWindow.axaml.cs` +- `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` + +Role split: + +- `MainWindow.axaml`: shell layout and bindings +- `MainWindow.axaml.cs`: drag/drop, picker triggers, keyboard/UI-only behavior +- `MainWindowViewModel.cs`: commands, state, workflow orchestration, status/progress, tool windows + +### Composition root + +Runtime wiring is centralized in: + +- `src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs` + +This is the first file to inspect when dependency wiring or service registration changes. + +### Views + +- `src/ChapterTool.Avalonia/Views/MainWindow.axaml` +- `src/ChapterTool.Avalonia/Views/Controls/ExpressionEditor.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/SettingsToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/ColorSettingsView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/LanguageToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/ExpressionToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/TemplateNamesToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/ForwardShiftToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Tools/TextToolView.axaml` + +### ViewModels + +- `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` +- `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs` +- `src/ChapterTool.Avalonia/ViewModels/ToolWindowViewModels.cs` +- `src/ChapterTool.Avalonia/ViewModels/ChapterRowViewModel.cs` +- `src/ChapterTool.Avalonia/ViewModels/UiCommand.cs` +- `src/ChapterTool.Avalonia/ViewModels/ShortcutRouter.cs` + +### Runtime and UI services + +- `src/ChapterTool.Avalonia/Services/RuntimeChapterLoadService.cs` +- `src/ChapterTool.Avalonia/Services/RuntimeChapterSaveService.cs` +- `src/ChapterTool.Avalonia/Services/RuntimeChapterImporterRegistry.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaSettingsPickerService.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaThemeApplicationService.cs` + +### CLI + +- `src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs` +- `src/ChapterTool.Avalonia/Cli/ChapterToolCliCommands.cs` +- `src/ChapterTool.Avalonia/Cli/ChapterToolCliSupport.cs` +- `src/ChapterTool.Avalonia/Cli/CliConsole.cs` + +### Localization + +- `src/ChapterTool.Avalonia/Localization/AppLocalizationManager.cs` +- `src/ChapterTool.Avalonia/Localization/IAppLocalizer.cs` +- `src/ChapterTool.Avalonia/Localization/AppLocalizationResources.cs` +- `src/ChapterTool.Avalonia/Localization/AppLanguage.cs` +- `src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx` +- `src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx` +- `src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx` + +## Feature Lookup + +### Main window layout, binding, workflow zones + +Start with: + +- `src/ChapterTool.Avalonia/Views/MainWindow.axaml` +- `src/ChapterTool.Avalonia/Views/MainWindow.axaml.cs` +- `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` + +### Main command workflow + +Start with: + +- `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` + +If keyboard routing matters: + +- `src/ChapterTool.Avalonia/ViewModels/ShortcutRouter.cs` + +If command execution semantics change: + +- `src/ChapterTool.Avalonia/ViewModels/UiCommand.cs` + +### Tool windows + +Start with: + +- `src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs` + +Then inspect the matching pair in: + +- `src/ChapterTool.Avalonia/Views/Tools/` +- `src/ChapterTool.Avalonia/ViewModels/` + +### Load/save/import behavior exposed in UI + +Start with: + +- `src/ChapterTool.Avalonia/Services/RuntimeChapterLoadService.cs` +- `src/ChapterTool.Avalonia/Services/RuntimeChapterSaveService.cs` +- `src/ChapterTool.Avalonia/Services/RuntimeChapterImporterRegistry.cs` + +If the wiring looks wrong, inspect: + +- `src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs` + +### Expression editor UI + +Start with: + +- `src/ChapterTool.Avalonia/Views/Tools/ExpressionToolView.axaml` +- `src/ChapterTool.Avalonia/Views/Controls/ExpressionEditor.axaml` +- `src/ChapterTool.Avalonia/ViewModels/ToolWindowViewModels.cs` + +### Settings / theme / language UI + +Start with: + +- `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs` +- `src/ChapterTool.Avalonia/Services/AvaloniaThemeApplicationService.cs` +- `src/ChapterTool.Avalonia/Localization/AppLocalizationManager.cs` + +### CLI behavior + +Start with: + +- `src/ChapterTool.Avalonia/Program.cs` +- `src/ChapterTool.Avalonia/Cli/ChapterToolCliApplication.cs` + +Use `ChapterToolCliCommands.cs` and `ChapterToolCliSupport.cs` for argument parsing and supported format definitions. + +### Localization changes + +Start with: + +- `src/ChapterTool.Avalonia/Localization/Resources/` + +If resource projection or language switching behavior changes, inspect: + +- `src/ChapterTool.Avalonia/Localization/AppLocalizationManager.cs` diff --git a/docs/code-map/core.md b/docs/code-map/core.md new file mode 100644 index 00000000..5c281c43 --- /dev/null +++ b/docs/code-map/core.md @@ -0,0 +1,119 @@ +# Core Code Map + +`src/ChapterTool.Core` owns the chapter domain model and pure business behavior. + +This layer is where import normalization, chapter editing, frame/time transforms, and export formatting belong. + +## Ownership + +### Models + +Canonical data contracts shared across the pipeline: + +- `src/ChapterTool.Core/Models/Chapter.cs` +- `src/ChapterTool.Core/Models/ChapterInfo.cs` +- `src/ChapterTool.Core/Models/ChapterInfoGroup.cs` +- `src/ChapterTool.Core/Models/ChapterSourceOption.cs` +- `src/ChapterTool.Core/Models/SourceMediaReference.cs` + +`ChapterInfo` is the main unit passed between import, edit, transform, and export flows. + +### Diagnostics + +Shared diagnostic contracts: + +- `src/ChapterTool.Core/Diagnostics/ChapterDiagnostic.cs` +- `src/ChapterTool.Core/Diagnostics/DiagnosticSeverity.cs` + +### Importing + +Import contracts and format-specific parsers: + +- `src/ChapterTool.Core/Importing/IChapterImporter.cs` +- `src/ChapterTool.Core/Importing/ChapterImportRequest.cs` +- `src/ChapterTool.Core/Importing/ChapterImportResult.cs` +- `src/ChapterTool.Core/Importing/ChapterLoadProgress.cs` + +Important format entry points: + +- Text dispatcher: `src/ChapterTool.Core/Importing/Text/TextChapterImporter.cs` +- OGM text: `src/ChapterTool.Core/Importing/Text/OgmChapterImporter.cs` +- Premiere marker CSV: `src/ChapterTool.Core/Importing/Text/PremiereMarkerListImporter.cs` +- Matroska XML: `src/ChapterTool.Core/Importing/Text/XmlChapterImporter.cs` +- WebVTT: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs` +- CUE sheet parsing: `src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs` +- Embedded FLAC/TAK CUE: `src/ChapterTool.Core/Importing/Cue/FlacCueImporter.cs`, `src/ChapterTool.Core/Importing/Cue/TakCueImporter.cs` +- DVD/Blu-ray playlist parsing: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs`, `src/ChapterTool.Core/Importing/Disc/MplsChapterImporter.cs`, `src/ChapterTool.Core/Importing/Disc/MplsPlaylistFile.cs`, `src/ChapterTool.Core/Importing/Disc/XplChapterImporter.cs` +- Media normalization contract: `src/ChapterTool.Core/Importing/Media/MediaChapterImporter.cs`, `src/ChapterTool.Core/Importing/Media/IMediaChapterReader.cs` + +### Editing + +In-memory chapter mutations: + +- `src/ChapterTool.Core/Editing/IChapterEditingService.cs` +- `src/ChapterTool.Core/Editing/ChapterEditingService.cs` +- `src/ChapterTool.Core/Editing/ChapterSegmentService.cs` +- `src/ChapterTool.Core/Editing/ChapterEditResult.cs` + +### Transform + +Frame/time and expression logic: + +- `src/ChapterTool.Core/Transform/FrameRateService.cs` +- `src/ChapterTool.Core/Transform/ChapterFpsTransformService.cs` +- `src/ChapterTool.Core/Transform/ChapterExpressionService.cs` +- `src/ChapterTool.Core/Transform/LuaExpressionScriptService.cs` +- `src/ChapterTool.Core/Transform/ExpressionAuthoringService.cs` +- `src/ChapterTool.Core/Transform/ChapterTimeFormatter.cs` +- `src/ChapterTool.Core/Transform/ChapterRounding.cs` + +### Exporting + +Output projection and format serialization: + +- `src/ChapterTool.Core/Exporting/ChapterExportService.cs` +- `src/ChapterTool.Core/Exporting/ChapterExportOptions.cs` +- `src/ChapterTool.Core/Exporting/ChapterExportFormat.cs` +- `src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs` +- `src/ChapterTool.Core/Exporting/ChapterConversionService.cs` +- `src/ChapterTool.Core/Exporting/XmlChapterLanguageCatalog.cs` + +## Feature Lookup + +### Import behavior + +Start in the matching importer under `Importing/`. + +Use these shortcuts: + +- `.txt` source detection and dispatch: `Importing/Text/TextChapterImporter.cs` +- disc binary parsing: `Importing/Disc/MplsPlaylistFile.cs` or the matching disc importer +- media chapter normalization after raw reader output: `Importing/Media/MediaChapterImporter.cs` + +### Chapter row editing + +Start with: + +- `src/ChapterTool.Core/Editing/ChapterEditingService.cs` + +For multi-part behavior, segment combining, or append flows: + +- `src/ChapterTool.Core/Editing/ChapterSegmentService.cs` + +### Frame rate and time transforms + +Start with: + +- detection: `src/ChapterTool.Core/Transform/FrameRateService.cs` +- FPS conversion: `src/ChapterTool.Core/Transform/ChapterFpsTransformService.cs` +- expression-driven rewrites: `src/ChapterTool.Core/Transform/ChapterExpressionService.cs` +- Lua evaluation: `src/ChapterTool.Core/Transform/LuaExpressionScriptService.cs` +- time parse/format bugs: `src/ChapterTool.Core/Transform/ChapterTimeFormatter.cs` + +### Export behavior + +Start with: + +- projection before serialization: `src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs` +- format-specific serialization: `src/ChapterTool.Core/Exporting/ChapterExportService.cs` +- text-to-QP/celltimes conversion: `src/ChapterTool.Core/Exporting/ChapterConversionService.cs` diff --git a/docs/code-map/infrastructure.md b/docs/code-map/infrastructure.md new file mode 100644 index 00000000..49f82beb --- /dev/null +++ b/docs/code-map/infrastructure.md @@ -0,0 +1,116 @@ +# Infrastructure Code Map + +`src/ChapterTool.Infrastructure` owns process execution, external tool discovery, settings persistence, filesystem/platform integration, and import adapters that depend on native tools or container libraries. + +## Ownership + +### Media and tool-backed import adapters + +- ffprobe-backed media chapters: + - `src/ChapterTool.Infrastructure/Importing/Media/FfprobeMediaChapterReader.cs` +- ATL-backed MP4 chapters: + - `src/ChapterTool.Infrastructure/Importing/Media/AtlMp4ChapterReader.cs` +- Matroska chapter extraction: + - `src/ChapterTool.Infrastructure/Importing/Matroska/MatroskaChapterImporter.cs` +- BDMV / eac3to path: + - `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs` + +### External tool discovery and process execution + +- tool lookup: + - `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs` + - `src/ChapterTool.Infrastructure/Tools/ExternalToolPathResolver.cs` + - `src/ChapterTool.Infrastructure/Tools/MkvToolNixInstallProbe.cs` +- process execution: + - `src/ChapterTool.Infrastructure/Processes/ProcessRunner.cs` +- service contracts: + - `src/ChapterTool.Infrastructure/Services/IExternalToolLocator.cs` + - `src/ChapterTool.Infrastructure/Services/IProcessRunner.cs` + - `src/ChapterTool.Infrastructure/Services/ProcessRunRequest.cs` + - `src/ChapterTool.Infrastructure/Services/ProcessRunResult.cs` + - `src/ChapterTool.Infrastructure/Services/ExternalToolLocation.cs` + +### Settings and configuration persistence + +- schema: + - `src/ChapterTool.Infrastructure/Configuration/AppSettings.cs` +- storage: + - `src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs` + - `src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs` +- corrupt-file handling: + - `src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs` + - `src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFileException.cs` +- JSON source generation: + - `src/ChapterTool.Infrastructure/Configuration/AppJsonSerializerContext.cs` + +### Platform services + +- shell/OS launch behavior: + - `src/ChapterTool.Infrastructure/Platform/ShellService.cs` +- native dependency lookup: + - `src/ChapterTool.Infrastructure/Platform/FileSystemNativeDependencyService.cs` + - `src/ChapterTool.Infrastructure/Platform/INativeDependencyService.cs` +- app log surface: + - `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs` +- test/dummy platform services: + - `src/ChapterTool.Infrastructure/Platform/MemoryClipboardService.cs` + - `src/ChapterTool.Infrastructure/Platform/ScriptedDialogService.cs` + - `src/ChapterTool.Infrastructure/Platform/RecordingWindowService.cs` + +## Feature Lookup + +### ffprobe import issues + +Start with: + +- `src/ChapterTool.Infrastructure/Importing/Media/FfprobeMediaChapterReader.cs` + +Then inspect: + +- `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs` +- `src/ChapterTool.Infrastructure/Processes/ProcessRunner.cs` + +### MP4 embedded chapter issues + +Start with: + +- `src/ChapterTool.Infrastructure/Importing/Media/AtlMp4ChapterReader.cs` + +### MKV / mkvextract issues + +Start with: + +- `src/ChapterTool.Infrastructure/Importing/Matroska/MatroskaChapterImporter.cs` + +Then inspect: + +- `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs` +- `src/ChapterTool.Infrastructure/Tools/MkvToolNixInstallProbe.cs` + +### BDMV / eac3to issues + +Start with: + +- `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs` + +### External tool path resolution + +Start with: + +- `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs` + +Use `ExternalToolPathResolver.cs` when path expansion, executable name, or default candidate rules are involved. + +### Settings persistence and corruption handling + +Start with: + +- `src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs` +- `src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs` + +### Shell, terminal, file reveal, and app log issues + +Start with: + +- `src/ChapterTool.Infrastructure/Platform/ShellService.cs` +- `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs` diff --git a/docs/code-map/testing.md b/docs/code-map/testing.md new file mode 100644 index 00000000..d4d004ce --- /dev/null +++ b/docs/code-map/testing.md @@ -0,0 +1,110 @@ +# Test Code Map + +This file maps production areas to the test projects and high-signal test files that verify them. + +## Test Projects + +- Core behavior: + - `tests/ChapterTool.Core.Tests` +- Infrastructure behavior: + - `tests/ChapterTool.Infrastructure.Tests` +- Avalonia shell, runtime UI services, localization, CLI: + - `tests/ChapterTool.Avalonia.Tests` + +## Core Test Map + +Use `tests/ChapterTool.Core.Tests` when changing pure parsing, editing, transform, or export behavior. + +High-signal test files: + +- importing + - `tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/CueImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/DiscImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/IfoImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/MplsImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/XplImporterTests.cs` + - `tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs` +- editing + - `tests/ChapterTool.Core.Tests/Editing/ChapterEditingServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Editing/ChapterSegmentServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Editing/SampleChapterNameTemplateTests.cs` +- transform + - `tests/ChapterTool.Core.Tests/Transform/FrameRateServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Transform/ChapterFpsTransformServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Transform/ChapterTimeFormatterTests.cs` + - `tests/ChapterTool.Core.Tests/Transform/ChapterRoundingTests.cs` + - `tests/ChapterTool.Core.Tests/Transform/LuaExpressionScriptServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Transform/ExpressionAuthoringServiceTests.cs` +- exporting + - `tests/ChapterTool.Core.Tests/Exporting/ChapterExportServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Exporting/ChapterOutputProjectionServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Exporting/ChapterConversionServiceTests.cs` + - `tests/ChapterTool.Core.Tests/Exporting/XmlChapterLanguageCatalogTests.cs` + +Fixtures: + +- `tests/ChapterTool.Core.Tests/Fixtures/` + +## Infrastructure Test Map + +Use `tests/ChapterTool.Infrastructure.Tests` when changing process/tool/platform/settings behavior or tool-backed import adapters. + +High-signal test files: + +- tool lookup: + - `tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs` +- ffprobe: + - `tests/ChapterTool.Infrastructure.Tests/FfprobeMediaChapterReaderTests.cs` + - `tests/ChapterTool.Infrastructure.Tests/Importing/FfprobeMediaChapterIntegrationTests.cs` +- MP4 / ATL: + - `tests/ChapterTool.Infrastructure.Tests/AtlMp4ChapterReaderTests.cs` + - `tests/ChapterTool.Infrastructure.Tests/Importing/Mp4IntegrationTests.cs` +- Matroska / mkvextract: + - `tests/ChapterTool.Infrastructure.Tests/MatroskaChapterImporterTests.cs` + - `tests/ChapterTool.Infrastructure.Tests/Importing/MatroskaIntegrationTests.cs` +- BDMV / eac3to: + - `tests/ChapterTool.Infrastructure.Tests/BdmvChapterImporterTests.cs` +- process runner: + - `tests/ChapterTool.Infrastructure.Tests/ProcessRunnerTests.cs` +- platform services: + - `tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs` + - `tests/ChapterTool.Infrastructure.Tests/ApplicationLogPanelProviderTests.cs` +- settings persistence: + - `tests/ChapterTool.Infrastructure.Tests/SettingsMigrationTests.cs` + +Fixtures: + +- `tests/ChapterTool.Infrastructure.Tests/Fixtures/Importing/Media/` + +## Avalonia Test Map + +Use `tests/ChapterTool.Avalonia.Tests` when changing UI shell, view models, runtime UI services, localization, headless layout, or CLI behavior. + +High-signal test files: + +- view models + - `tests/ChapterTool.Avalonia.Tests/ViewModels/MainWindowViewModelTests.cs` + - `tests/ChapterTool.Avalonia.Tests/ViewModels/SettingsToolViewModelTests.cs` + - `tests/ChapterTool.Avalonia.Tests/ViewModels/ToolWindowViewModelTests.cs` +- commands and services + - `tests/ChapterTool.Avalonia.Tests/Commands/UiCommandTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Services/` +- CLI + - `tests/ChapterTool.Avalonia.Tests/Cli/ChapterToolCliApplicationTests.cs` +- localization + - `tests/ChapterTool.Avalonia.Tests/Localization/LocalizationTests.cs` +- headless shell/layout/integration + - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowStateHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/ToolViewsHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/SettingsToolHeadlessTests.cs` + - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowHeadlessTestHost.cs` + +## Quick Routing + +- parsing or export semantics changed: start in `tests/ChapterTool.Core.Tests` +- external tool, settings, process, or platform boundary changed: start in `tests/ChapterTool.Infrastructure.Tests` +- view, viewmodel, CLI, localization, or runtime UI orchestration changed: start in `tests/ChapterTool.Avalonia.Tests` diff --git a/docs/code-review-2026-07-06.md b/docs/code-review-2026-07-06.md deleted file mode 100644 index eb785e78..00000000 --- a/docs/code-review-2026-07-06.md +++ /dev/null @@ -1,280 +0,0 @@ -# Code Review - 2026-07-06 - -## Scope - -This review inspected the current `src/` and `tests/` code for: - -- fake or incomplete stub behavior exposed as real functionality -- low-value or misleading tests -- unusual compatibility or platform handling -- deviations from the repository guidance and common .NET/Avalonia practices - -Four subagents reviewed Core, Infrastructure, Avalonia/UI, and cross-repository suspicious patterns in parallel. The review was read-only; no tests were run. - -Existing working tree note: `scripts/publish.sh` was already modified before this review and was not touched. - -## Summary - -No critical issues were found. The highest-risk findings are: - -- WebVTT import drops cue end times and calculates duration incorrectly. -- Windows terminal fallback in `ShellService` builds a command string from a path. -- File association support exists as a partial service surface but is not complete enough to be trustworthy. -- Preview UI is available when no chapter data exists and opens an empty window. - -## High Severity - -### WebVTT cue end times are parsed but discarded - -- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:40` -- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:48` -- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:62` - -The importer validates the cue end time with `TimeSpan.TryParse(parts[1], out _)`, but the parsed value is discarded. `Chapter.End` is never populated, and `ChapterInfo.Duration` is set to the last chapter start time. - -Impact: imported WebVTT chapters lose end times. Any later export or duration-dependent workflow can produce wrong final segment timing. - -Recommendation: keep the parsed end value, pass it as `End` when creating `Chapter`, and calculate duration from the last valid cue end. - -### Windows terminal fallback is command-injection prone - -- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:62` - -The Windows fallback path builds a command string: - -```text -cmd /c start cmd /k "cd /d {directoryPath}" -``` - -Because `directoryPath` is user/path data, characters such as `&` or quotes can change the command. Exceptions around this path are also swallowed. - -Impact: malformed or adversarial paths can execute unintended shell commands; failures are invisible to the user. - -Recommendation: avoid command-string composition. Start `cmd.exe` directly with fixed arguments and set `ProcessStartInfo.WorkingDirectory = directoryPath`, or use a safer platform abstraction that returns a structured failure. - -### File association service is incomplete and not wired to the documented command surface - -- File: `src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs:27` -- File: `src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs:60` -- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:520` -- Related spec: `openspec/specs/avalonia-ui-shell/spec.md:60` - -`WindowsFileAssociationService` writes only the ProgID description and extension default value. It does not write `shell\open\command`, icon metadata, ownership markers, or shell change notifications. `UnregisterAsync` deletes the extension key without confirming it is still owned by ChapterTool. - -The spec says the main window ViewModel shall expose a file association command, but the current command list has no such command. The composition root can create the service, but no user-visible workflow appears to consume it. - -Impact: registration can report success without making files open correctly, and unregister can remove another app's association. The implementation also looks like a feature but lacks an end-to-end user path. - -Recommendation: either complete the service and UI workflow, or remove/hide the feature surface until it is ready. A complete implementation should write an open command, track ownership, guard unregistration, refresh shell associations, and have tests through an injectable registry abstraction. - -### Preview opens an empty window with no loaded chapters - -- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:260` -- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:148` -- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:1304` - -The preview command is an unconditional `WindowCommand("preview")`. With no current chapter data, `BuildPreview()` returns empty text, so the user can open a blank preview window. - -Impact: this is a visible UI stub: the control appears usable but has no meaningful behavior in the empty state. Keyboard access such as `F11` can trigger the same result. - -Recommendation: make `PreviewCommand.CanExecute` depend on loaded chapter data, and ensure buttons, menus, and shortcuts share that state. Alternatively, show an explicit empty-state message. - -## Medium Severity - -### `CueChapterImporter` has a fake injectable parser - -- File: `src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs:3` -- File: `src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs:29` - -The constructor accepts `CueSheetParser? parser` and stores it, but `ImportAsync` calls the static `CueSheetParser.Parse` method. - -Impact: callers and tests may think parser behavior is replaceable, but the injected object is ignored. - -Recommendation: remove the constructor parameter and field, or introduce a real parser interface/instance method and use it. - -### `IfoChapterImporter` is fake-async and ignores request content - -- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:16` -- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:18` -- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:21` - -`ImportAsync` awaits `Task.CompletedTask`, then reads only from `request.Path`. It ignores `request.Content`. - -Impact: IFO behaves differently from other importers that support in-memory content. The fake await also obscures the synchronous file-only behavior. - -Recommendation: parse from a seekable stream and prefer `request.Content` when present. If file-only sync parsing is intentional for now, remove the fake await and document the limitation. - -### Core tests depend on Infrastructure - -- File: `tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj:30` -- File: `tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs:4` - -`ChapterTool.Core.Tests` references `ChapterTool.Infrastructure` and instantiates `FfprobeMediaChapterReader`. - -Impact: Core tests become mixed integration tests and can fail because of Infrastructure parser behavior rather than Core importer behavior. - -Recommendation: test Core's `MediaChapterImporter` using a fake `IMediaChapterReader` that returns `MediaChapterEntry` values. Keep ffprobe JSON/process parsing tests in `ChapterTool.Infrastructure.Tests`. - -### External tool locator treats any file as an executable - -- File: `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs:37` -- File: `tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs:15` - -Tool resolution uses `File.Exists` only, and tests use empty text files as successful tool candidates. - -Impact: on Linux/macOS, non-executable files can be reported as found. The settings UI can show success, then runtime execution fails later. - -Recommendation: add executable validation. On Unix-like systems, check execute permissions. On Windows, validate expected executable naming, and consider an optional lightweight `--version` probe for configured tools. - -### Corrupt settings silently reset to defaults - -- File: `src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs:43` -- File: `src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs:29` - -Invalid JSON is caught and replaced with default settings without logging or surfacing a diagnostic. - -Impact: user settings can appear to reset, and a later save can overwrite the corrupt file with defaults, making recovery harder. - -Recommendation: log the parse failure, preserve the corrupt file as `.corrupt`, and expose a user-visible warning before defaults are saved back. - -### Shell failures are silently swallowed - -- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:41` -- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:74` -- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:105` - -Reveal/open-terminal helper paths catch broad exceptions and ignore them. - -Impact: users see an action that does nothing, with no status or log entry. - -Recommendation: catch expected process/platform exceptions and return or log structured failure information. - -### FFmpeg directory setting actually validates ffprobe - -- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:248` -- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:255` -- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:577` -- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:593` - -`FfmpegPath` is presented as an FFmpeg directory setting, but validation and discovery check `ffprobe`. - -Impact: users may think they configured FFmpeg, while the app actually validates ffprobe. The setting overlaps semantically with `FfprobePath`. - -Recommendation: rename it to ffprobe directory if that is the intended behavior, or validate/discover `ffmpeg` if it truly represents FFmpeg. - -### Native file picker strings are hard-coded English - -- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:11` -- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:16` -- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:37` -- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:54` -- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:71` - -File picker titles and file type labels are English string literals. - -Impact: localized UI still opens English system dialogs. - -Recommendation: inject `IAppLocalizer` into `AvaloniaFilePickerService`, or have callers pass localized titles/filter names. - -### Icon buttons lack accessible names - -- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:260` -- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:267` -- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:275` -- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:505` -- File: `src/ChapterTool.Avalonia/Views/Tools/SettingsToolView.axaml:101` - -Several icon buttons have tooltip text and automation IDs, but no localized `AutomationProperties.Name`. - -Impact: screen readers may not announce a useful action name. Automation IDs are not a substitute for accessible names. - -Recommendation: add localized `AutomationProperties.Name` and, where useful, `AutomationProperties.HelpText`. Headless tests should verify the accessible name, not only command binding. - -### BDMV parser moves data through diagnostics and narrow regexes - -- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:160` -- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:344` - -The BDMV importer stores eac3to stdout in a diagnostic message and then parses it later with narrow text regexes. - -Impact: adding another diagnostic can break assumptions, and small eac3to output/version/localization changes can stop detection. - -Recommendation: return an internal structured result that carries stdout separately from diagnostics. Add fixture coverage from real eac3to outputs and handle known format variants. - -## Low Severity - -### Test fakes live in production Infrastructure - -- File: `src/ChapterTool.Infrastructure/Platform/MemoryClipboardService.cs:5` -- File: `src/ChapterTool.Infrastructure/Platform/ScriptedDialogService.cs:5` -- File: `src/ChapterTool.Infrastructure/Platform/RecordingWindowService.cs:5` -- File: `tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs:63` - -These are test-double style services in the production Infrastructure assembly. The test name calls them skeletons. - -Impact: they can be accidentally injected into production paths and silently record operations instead of performing real platform behavior. - -Recommendation: move them to test projects, or mark them internal/test-only and avoid exposing them as production services. - -### `UiCommandTests` waits with a fixed delay - -- File: `tests/ChapterTool.Avalonia.Tests/Commands/UiCommandTests.cs:47` - -The test waits for `async void` command completion with `Task.Delay(50)`. - -Impact: slow CI machines or scheduling delays can make the test flaky. - -Recommendation: use a `TaskCompletionSource`, property changed event, or polling with a bounded timeout for `ExecutionError`. - -### Matroska integration setup blocks on async - -- File: `tests/ChapterTool.Infrastructure.Tests/Importing/MatroskaIntegrationTests.cs:27` - -`IAsyncLifetime.InitializeAsync` uses `.AsTask().Result`. - -Impact: this is a sync-over-async pattern with deadlock and cancellation risks. - -Recommendation: make `InitializeAsync` an `async ValueTask` and `await LocateAsync(...)`. - -### Screenshot tests are weak regression guards - -- File: `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowHeadlessTests.cs:190` -- File: `tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs:117` - -The tests mainly capture screenshots and assert the files exist/non-empty. - -Impact: a badly misaligned or semantically blank UI can still pass. - -Recommendation: keep screenshots as artifacts, but add behavioral/layout assertions such as key controls visible, no bounds overflow, and meaningful non-background pixel regions. - -### MKVToolNix registry DisplayIcon parsing misses quoted paths - -- File: `src/ChapterTool.Infrastructure/Tools/MkvToolNixInstallProbe.cs:78` - -The install probe trims a trailing comma suffix but does not normalize quoted `DisplayIcon` values. - -Impact: common registry values such as `"C:\Program Files\...\mkvtoolnix-gui.exe",0` may fail to resolve to the install directory. - -Recommendation: strip quotes after trimming the icon suffix, then take the directory. Add a quoted DisplayIcon test. - -### eac3to export can show a console window - -- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:171` -- File: `tests/ChapterTool.Infrastructure.Tests/BdmvChapterImporterTests.cs:51` - -The eac3to chapter export request explicitly sets `CreateNoWindow: false`, and the test locks in that behavior. - -Impact: GUI import can flash or show a console window. - -Recommendation: default to hidden process windows unless eac3to requires visibility. If visibility is required, document the reason and test that reason rather than the raw flag. - -## Non-Issues / Filtered Results - -The review did not find meaningful hits for: - -- `NotImplementedException` -- `PlatformNotSupportedException` -- `Thread.Sleep` -- global test parallelization disablement in Avalonia tests -- tests reading source/configuration files and asserting incidental source strings - diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md deleted file mode 100644 index b1def272..00000000 --- a/docs/code-review-fix-todo-2026-07-06.md +++ /dev/null @@ -1,40 +0,0 @@ -# Code Review Fix TODO - 2026-07-06 - -Source report: `docs/code-review-2026-07-06.md` - -## Rules - -- Fix one issue at a time. -- Run focused tests after each fix. -- Commit each completed fix separately. -- Do not include unrelated working tree changes, especially the pre-existing `scripts/publish.sh` modification. - -## High Priority - -- [x] Fix WebVTT import so cue end times and duration are preserved. -- [x] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. -- [x] Complete Windows file association registry behavior so registration writes an open command and unregistration protects existing associations. -- [ ] Add or explicitly defer a non-primary settings/command surface for file association. -- [x] Prevent preview from opening as an empty stub when no chapters are loaded. - -## Medium Priority - -- [x] Remove the fake injectable parser from `CueChapterImporter`. -- [x] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. -- [x] Remove the Infrastructure dependency from Core tests. -- [x] Validate external tools as executables, not just existing files. -- [x] Preserve and surface corrupt settings files instead of silently resetting. -- [x] Return or log shell service failures instead of swallowing them. -- [x] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. -- [x] Localize native file picker titles and file type labels. -- [x] Add accessible names for icon-only buttons. -- [x] Refactor BDMV parsing so stdout is not passed through diagnostics. - -## Low Priority - -- [x] Move production test-double services out of Infrastructure or mark them test-only. -- [x] Replace fixed `Task.Delay` in `UiCommandTests`. -- [x] Remove sync-over-async from Matroska integration setup. -- [x] Strengthen screenshot tests with layout/content assertions. -- [x] Handle quoted MKVToolNix `DisplayIcon` registry values. -- [x] Hide eac3to export process windows unless visibility is required. diff --git a/docs/i18n-audit-and-fix-plan.md b/docs/i18n-audit-and-fix-plan.md deleted file mode 100644 index 92cc0df4..00000000 --- a/docs/i18n-audit-and-fix-plan.md +++ /dev/null @@ -1,165 +0,0 @@ -# 国际化（i18n）排查与修复计划 - -> 生成日期：2026-07-04 -> 范围：`ChapterTool.Avalonia` / `ChapterTool.Core` / `ChapterTool.Infrastructure` -> 现象：选择中文（或日文）后，部分日志、状态、诊断信息仍显示英文。 - -## 执行结果（截至 2026-07-04） - -| 任务 | 状态 | 说明 | -|------|------|------| -| Task 1 — Diagnostic.* 本地化 | ✅ 已完成 | 新增 73 + 18 个 `Diagnostic.*` 键（三语）；`ChapterDiagnostic` 增加 `Arguments`；`LocalizeDiagnostic` 支持 `{message}` 自动回填与命名占位符；`LogDiagnostics` 改用本地化消息。 | -| Task 2 — operation/action 本地化 | ✅ 已完成 | 新增 `Action.*`(7)/`EditKind.*`(3)/`Operation.*`(6) 键；`ApplyEdit`、`LogDiagnostics` 全部调用点改用本地化字符串。 | -| Task 3 — ExpressionService 错误本地化 | ✅ 已完成 | 引入 `ExpressionException(code,message,args)`，18 个错误码 `InvalidExpression.*` 三语；测试断言改为 `StartsWith`。 | -| Task 4 — 外部工具/依赖缺失消息 | ✅ 已随 Task 1 完成 | `MissingDependency`/`MatroskaMissingDependency`/`FfprobeMissingDependency` 为静态本地化键（内嵌工具名），显示时覆盖定位器英文消息。 | -| Task 5 — 清理废弃本地化抽象 | ✅ 已完成 | 删除 `ILocalizationService`/`LocalizationService`（Core/Infrastructure）；从 `PlatformServiceTests` 移除对应测试块。无悬空引用。 | - -**资源键总数**：每语言由 126 → **233** 键（三语严格对齐，占位符一致性测试守护）。 -**测试**：Core 276 / Infrastructure 88 / Avalonia.Localization 11 全绿；Avalonia 全量（含 headless）在 Task 1-2 阶段已全绿（Task 3/5 仅触及 Core/Infrastructure）。 - ---- - -## 一、现状架构概览 - -| 维度 | 说明 | -|------|------| -| 资源文件 | `src/ChapterTool.Avalonia/Localization/Resources/Strings.{zh-CN,en-US,ja-JP}.resx`（各 126 键，键/占位符一致性有测试守护） | -| 默认/回退文化 | **zh-CN**（`AppLanguage.DefaultCultureName = "zh-CN"`，无中性 `Strings.resx`） | -| 活跃本地化服务 | `IAppLocalizer` / `AppLocalizationManager`（Avalonia 层），`SetCulture` 同时写入 `Application.Current.Resources` 供 `{DynamicResource}` 使用 | -| XAML | 全量使用 `{DynamicResource Key}`，UI 文本无硬编码英文 ✅ | -| 日志通道 | `MainWindowViewModel.Log(key,...)` 注入 `MessageKey`；显示时由 `FormatLogEntry` 经 `Localizer.Format` 重新本地化。**只有传入真实 `Log.*`/`Status.*` 键才会本地化**，传字面量英文则绕过本地化 | - -### 文化切换链路（已正确） -`AppSettings.Language` → 启动 `MainWindowViewModel.LoadSettings` → `Localizer.SetCulture` → `CultureChanged` 事件 → 各 ViewModel 刷新 + XAML `{DynamicResource}` 自动更新 + 持久化。 - -## 二、缺陷清单（按优先级） - -### P0 — 诊断信息（Diagnostic）完全不翻译 ⭐ 最大问题 -- **位置**：`MainWindowViewModel.LocalizeDiagnostic`（`MainWindowViewModel.cs:1365`） -- **机制**：代码已构造 `Diagnostic.{Code}` 键查找，但 **三个 resx 文件中 `Diagnostic.*` 键数为 0**。 -- **后果**：状态栏、日志面板对所有诊断一律回退到 `diagnostic.Message`（硬编码英文）。 -- **涉及诊断码（约 60 个）**：`InvalidFrameRate`、`NoSegments`、`UnsupportedCombineSource`、`InvalidChapterIndex`、`InvalidExpression`、`PartialParse`、`EmptyCueFile`、`MalformedCueSyntax`、`MatroskaProcessFailed`、`FfprobeProcessFailed`、`Mp4ReadFailed`、`MissingDependency`、`Stdout` 等（完整清单见附录 A）。 -- **来源文件**：`src/ChapterTool.Core/{Editing,Transform,Exporting,Importing}/...`、`src/ChapterTool.Infrastructure/{Platform,Importing,Tools}/...` - -### P1 — 日志 operation/action 参数硬编码英文 -- **A. `ApplyEdit(result, action)`**（`MainWindowViewModel.cs:890`） - - `action` 作为本地化模板 `Log.EditChapters` 的 `{action}` 占位符，但调用方传入全英文：`$"Delete rows: indexes=..."`、`$"Insert row: index=..."`、`$"Shift frames forward: ..."`、`$"Edit {kind}: row=..."`、`$"Combine segments: ..."`。 - - → 模板被翻译，但内嵌的 `{action}` 片段恒为英文。 -- **B. `LogDiagnostics(operation, ...)`**（`MainWindowViewModel.cs:1516`） - - 多处调用传入英文字面量：`"Load"`、`"Save"`、`"Create zones"`、`"Append load"`、`"Append edit"`、`"Output projection"` 等（`{operation}` 占位符）。 - - 同时该函数把 `diagnostic.Message`（英文）直接塞入 `Log.Diagnostic` 的 `{message}` 占位符，**未走 `LocalizeDiagnostic`**。 - -### P2 — ExpressionService 异常消息全英文 -- **位置**：`src/ChapterTool.Core/Transform/ExpressionService.cs:140,261,271,283,293,303,320,338,348` -- 抛出的英文消息（`"Misplaced comma."`、`"Unbalanced parentheses."`、`"Expression did not reduce to one value."` 等）经 `InvalidExpression` 诊断原样显示给用户。 - -### P3 — 外部工具/原生依赖缺失消息全英文 -- `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs:77`：`$"External tool '{toolId}' was not found."` -- `src/ChapterTool.Infrastructure/Platform/FileSystemNativeDependencyService.cs:21`：`$"Native dependency '{dependencyId}' was not found."` -- 被各 importer 原样透传为 `ChapterDiagnostic.Message`（code=`MissingDependency`）。 - -### P4 — 重复/废弃的本地化抽象（清理项） -- `src/ChapterTool.Core/Services/ILocalizationService.cs` + `src/ChapterTool.Infrastructure/Platform/LocalizationService.cs` -- 构造时资源字典为空，**未被 `AppCompositionRoot` 接入**，仅出现在测试中。与 `IAppLocalizer` 重复，易致混淆。 - -### 已验证为正常（无需改动） -- 所有 `.axaml` 用户文本均走 `{DynamicResource}`。 -- `SetStatus` / `SetProgressStatus` 一律使用 `Status.*` 键。 -- `SettingsToolViewModel`、`AvaloniaSettingsCloseConfirmationService` 全本地化。 -- 无 `Console/Debug/Trace.WriteLine`。 - -## 三、修复任务（subagent 顺序执行，避免并发改 resx 冲突） - -> ⚠️ 所有任务共享 `Strings.*.resx`，**必须串行**；每个 subagent 完成后由主控做构建/测试验证再启动下一个。 - -### Task 1 — 诊断信息本地化（P0）⭐ 核心 -**目标**：状态栏与日志面板的诊断消息随当前语言切换。 - -**改动**： -1. `ChapterDiagnostic` 增加 `IReadOnlyDictionary? Arguments = null`（可选，向后兼容）。 -2. 三个 resx 新增全部 `Diagnostic.{Code}` 键（含 `{占位符}`，见附录 A 的带参清单）。 -3. `LocalizeDiagnostic` 改为 `Localizer.Format(diagnosticKey, diagnostic.Arguments)`。 -4. `LogDiagnostics` 中 `{message}` 改用 `LocalizeDiagnostic(diagnostic)`，`{code}` 保留原始码（技术标识）。 -5. 对带插值的诊断生产点（`InvalidChapterIndex`、`PartialParse(line)`、`OrderShiftNormalized` 等）改为传 `Arguments`，`Message` 保留英文作技术回退。 -6. `LocalizationTests` 增加 `Diagnostic.*` 三语键/占位符一致性校验。 - -**验收**：`dotnet test ChapterTool.Avalonia.slnx --no-restore` 通过；切换语言后诊断消息随之变化。 - -### Task 2 — 日志 operation/action 参数本地化（P1） -**目标**：消除日志行中内嵌的英文片段。 - -**改动**： -1. 新增 `Operation.*` 键（`Operation.Load/Save/CreateZones/AppendLoad/AppendEdit/OutputProjection/AppendLoadEdit` 等）三语。 -2. 新增 `Log.EditChapters.*` 子键或 `Action.*` 键（`DeleteRows/InsertRow/ShiftFramesForward/EditCell/CombineSegments/EditChapters`）三语，带 `{indexes}`/`{index}`/`{frames}`/`{kind}`/`{row}`/`{value}`/`{count}`/`{sourceType}` 占位符。 -3. `ApplyEdit` 签名改为接收 `LocalizedMessage` 或 `(string key, args)` 而非原始英文字符串；更新全部调用点。 -4. `LogDiagnostics` 的 `operation` 参数改为本地化键路径；更新全部调用点。 - -**验收**：构建通过；日志面板在中文/日文下不再出现 "Delete rows"、"Load" 等英文片段。 - -### Task 3 — ExpressionService 错误消息本地化（P2） -**目标**：表达式解析错误随语言切换。 - -**改动**： -1. 为 `ExpressionService` 中每种解析失败新增 `Expression.*` 键三语（或扩展 `Diagnostic.InvalidExpression.*`）。 -2. 改造抛错路径：以「错误码 + 参数」形式携带，最终在 `InvalidExpression` 诊断处本地化；或令 `ExpressionService` 接收本地化查表回调。 -3. 同步附录 A 的 `InvalidExpression` 处理。 - -**验收**：构造错误表达式，中文/日文下提示为对应语言。 - -### Task 4 — 外部工具/依赖缺失消息本地化（P3） -**目标**：`MissingDependency` 诊断消息本地化。 - -**改动**： -1. `ExternalToolLocator` / `FileSystemNativeDependencyService` 返回结构化位置（`ToolId`/`DependencyId`）而非预格式化英文 `Message`。 -2. 由 importer 在生成 `MissingDependency` 诊断时携带 `Arguments={toolId/dependencyId}`，复用 Task 1 的 `Diagnostic.MissingDependency` 键。 -3. 校验 `BdmvChapterImporter`、`MatroskaChapterImporter`、`FfprobeMediaChapterReader` 的透传链路。 - -**验收**：缺依赖场景下提示为当前语言。 - -### Task 5 — 清理废弃本地化抽象（P4） -**目标**：消除 `ILocalizationService`/`LocalizationService` 死代码与潜在误用。 - -**改动**： -1. 确认仅 `tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs` 使用。 -2. 删除接口与实现 + 测试引用，或统一并入 `IAppLocalizer`（视依赖最小化原则择一）。 - -**验收**：构建+全量测试通过，无悬空引用。 - -## 四、执行顺序与验证 -1. Task 1（P0）→ `dotnet build` + `dotnet test ChapterTool.Avalonia.slnx --no-restore` -2. Task 2（P1）→ 同上 -3. Task 3（P2）→ 同上 -4. Task 4（P3）→ 同上 -5. Task 5（P4）→ 同上 -6. 终验：`openspec validate --all`（如涉及 spec）+ 全量测试 - -## 附录 A — 诊断码完整清单（待补 Diagnostic.* 键） - -### 静态消息（无占位符） -``` -InvalidFrameRate, InvalidFrameText, NoRowsSelected, NoSegments, -UnsupportedCombineSource, UnsupportedAppendSource, UnsupportedExportFormat, -NoChapters, NoChaptersFound, EmptyCueFile, MalformedCueSyntax, -InvalidContainerHeader, FlacEmbeddedCueNotFound, EmbeddedCueNotFound, -InvalidMpls, InvalidStructure, InvalidIfo, InvalidXml, InvalidEntryElement, -InvalidChapterPair, EmptyXml, XmlInvalidRoot, XmlNoChapters, XplNoChapters, -XplParseFailed, EmptyChapters, OgmInvalidFirstLine, PremiereMarkerListInvalid, -WebVttInvalidHeader, WebVttMalformedCue, InvalidTimeText, InvalidTimecodeText, -InvalidChapterText, InvalidExpressionTime, UnsupportedPlatform, -DependencyExecutionCancelled, DependencyExecutionFailed, DependencyExecutionTimedOut, -DependencyOutputUnrecognized, DependencyOutputMissing, DependencyOutputEmpty, -MatroskaCannotStart, MatroskaProcessCancelled, MatroskaProcessFailed, -MatroskaProcessTimedOut, FfprobeEmptyOutput, FfprobeParseFailed, -FfprobeProcessCancelled, FfprobeProcessFailed, FfprobeProcessTimedOut, -Mp4FileInaccessible, Mp4FileNotFound, Mp4InvalidPath, Mp4MalformedMetadata, -Mp4ReadFailed, Mp4UnsupportedMetadata, Stdout, MissingDependency -``` - -### 带占位符消息 -| Code | 占位符 | 示例 | -|------|--------|------| -| `InvalidChapterIndex` | `{index}` | Chapter index {index} is out of range. | -| `PartialParse` | `{line}` | Parsing stopped at line: {line} | -| `OrderShiftNormalized` | （待核） | 订单位移已归一化 | -| `MissingDependency` | `{toolId}` / `{dependencyId}` | External tool '{toolId}' was not found. | -| `Stdout` | `{output}` | （原样透传命令输出） | diff --git a/docs/avalonia-modernization-review.md b/docs/migrations/avalonia-modernization-review.md similarity index 100% rename from docs/avalonia-modernization-review.md rename to docs/migrations/avalonia-modernization-review.md diff --git a/docs/avalonia-rewrite-spec.md b/docs/migrations/avalonia-rewrite-spec.md similarity index 100% rename from docs/avalonia-rewrite-spec.md rename to docs/migrations/avalonia-rewrite-spec.md diff --git a/docs/coverage-matrix.md b/docs/migrations/coverage-matrix.md similarity index 100% rename from docs/coverage-matrix.md rename to docs/migrations/coverage-matrix.md diff --git a/docs/feature-implementation-progress-report.md b/docs/migrations/feature-implementation-progress-report.md similarity index 100% rename from docs/feature-implementation-progress-report.md rename to docs/migrations/feature-implementation-progress-report.md diff --git a/docs/gui-verification.md b/docs/migrations/gui-verification.md similarity index 100% rename from docs/gui-verification.md rename to docs/migrations/gui-verification.md diff --git a/docs/legacy-new-implementation-diff.md b/docs/migrations/legacy-new-implementation-diff.md similarity index 100% rename from docs/legacy-new-implementation-diff.md rename to docs/migrations/legacy-new-implementation-diff.md diff --git a/docs/modules/01-ui-shell-and-interactions.md b/docs/migrations/modules/01-ui-shell-and-interactions.md similarity index 100% rename from docs/modules/01-ui-shell-and-interactions.md rename to docs/migrations/modules/01-ui-shell-and-interactions.md diff --git a/docs/modules/02-core-model-transform-export.md b/docs/migrations/modules/02-core-model-transform-export.md similarity index 100% rename from docs/modules/02-core-model-transform-export.md rename to docs/migrations/modules/02-core-model-transform-export.md diff --git a/docs/modules/03-text-xml-matroska-vtt-importers.md b/docs/migrations/modules/03-text-xml-matroska-vtt-importers.md similarity index 100% rename from docs/modules/03-text-xml-matroska-vtt-importers.md rename to docs/migrations/modules/03-text-xml-matroska-vtt-importers.md diff --git a/docs/modules/04-disc-playlist-media-importers.md b/docs/migrations/modules/04-disc-playlist-media-importers.md similarity index 100% rename from docs/modules/04-disc-playlist-media-importers.md rename to docs/migrations/modules/04-disc-playlist-media-importers.md diff --git a/docs/modules/05-cue-flac-tak-importers.md b/docs/migrations/modules/05-cue-flac-tak-importers.md similarity index 100% rename from docs/modules/05-cue-flac-tak-importers.md rename to docs/migrations/modules/05-cue-flac-tak-importers.md diff --git a/docs/modules/06-supporting-ui-platform-services.md b/docs/migrations/modules/06-supporting-ui-platform-services.md similarity index 100% rename from docs/modules/06-supporting-ui-platform-services.md rename to docs/migrations/modules/06-supporting-ui-platform-services.md diff --git a/docs/modules/07-tests-build-distribution-assets.md b/docs/migrations/modules/07-tests-build-distribution-assets.md similarity index 100% rename from docs/modules/07-tests-build-distribution-assets.md rename to docs/migrations/modules/07-tests-build-distribution-assets.md diff --git a/docs/openspec-avalonia-dotnet10-spec-split.md b/docs/migrations/openspec-avalonia-dotnet10-spec-split.md similarity index 100% rename from docs/openspec-avalonia-dotnet10-spec-split.md rename to docs/migrations/openspec-avalonia-dotnet10-spec-split.md diff --git a/docs/packaging-strategy.md b/docs/migrations/packaging-strategy.md similarity index 100% rename from docs/packaging-strategy.md rename to docs/migrations/packaging-strategy.md diff --git a/docs/progress.md b/docs/migrations/progress.md similarity index 100% rename from docs/progress.md rename to docs/migrations/progress.md diff --git a/docs/spec-implementation-verification.md b/docs/migrations/spec-implementation-verification.md similarity index 100% rename from docs/spec-implementation-verification.md rename to docs/migrations/spec-implementation-verification.md diff --git a/docs/review-2026-07-05/agent-findings.md b/docs/review-2026-07-05/agent-findings.md deleted file mode 100644 index 834e39a9..00000000 --- a/docs/review-2026-07-05/agent-findings.md +++ /dev/null @@ -1,83 +0,0 @@ -# Agent Findings — Sub-agent Dispatch Log - -> Record of sub-agent dispatches, candidate findings, adoption/rejection decisions, and verification notes. - -## Dispatch Summary - -| Phase | Task ID | Duration | Candidate Findings | Adopted | Rejected | Downgraded | -|-------|---------|----------|--------------------|---------|----------|-------------| -| 1 Core | bg_b9a2378f | 9m21s | 2 + 1 suspect | 2 (P2,P3) | 1 (P1 FP) | — | -| 2 Infra | bg_5f185fb5 | 9m47s | 2 + 3 suspects | 2 (P2,P2) | — | 1 (P1→P2) | -| 3 VM | bg_cbd33c95 | 7m52s | 4 + 3 suspects | 2 (P2,P3) | 1 (P1 FP) | 1 (P2→P3) | -| 4 Views | bg_1c8af626 | 6m31s | 3 + 3 suspects | 3 (P2,P3,P3) | — | 1 (P1→P2), 1 (P2→P3) | -| 5 Scripts | bg_183f5248 | 4m57s | 3 + 3 suspects | 4 (P2,P2,P3,P3) | — | — | -| 6 L10n | bg_0defc2e9 | 5m47s | 0 | 0 | — | — | - -## Rejected Findings (False Positives) - -### Phase 1 — ~~P1: MissingOperatorBeforeFunction missing resx key~~ - -**Sub-agent claim:** The code `InvalidExpression.MissingOperatorBeforeFunction` thrown at `ExpressionService.cs:262` has no corresponding `Diagnostic.InvalidExpression.MissingOperatorBeforeFunction` key in the resx. - -**Main-agent verification:** -``` -$ grep -n "MissingOperatorBeforeFunction" Strings.en-US.resx -:161: - -$ comm -23 <(all codes thrown) <(all keys present in resx) -(empty) -``` - -**Verdict: REJECTED.** Key exists at line 161 in all three resx files. All 18 `InvalidExpression.*` codes have complete coverage. - -**Root cause of sub-agent error:** The sub-agent checked for key presence by reading a narrow line range (`:158-166`) and didn't find the key in that window, but it was at `:161` — likely a parsing/line-counting discrepancy in the sub-agent's resx read. - ---- - -### Phase 3 — ~~P1: Selector identity mismatch (recomputed list + reference equality)~~ - -**Sub-agent claim:** `XmlLanguageDisplayOptions` returns a new array every getter call; `SelectorDisplayOption` has no value equality → UI selection desync. - -**Main-agent verification:** -- `XmlLanguageDisplayOptions => xmlLanguageDisplayOptions` returns the **same persistent `ObservableCollection`** (`MainWindowViewModel.cs:289`), not a new array. -- `RefreshXmlLanguageDisplayOptions` (`:1463-1487`) preserves identity: - - Count unchanged → `UpdateFrom()` mutates in place. - - Count changed → clear + re-add on same collection instance. -- `SelectedXmlLanguageDisplayOption` setter matches by `MainText` value (`:304`), not reference. -- Getter indexes by `XmlLanguageIndex` integer (`:296`), not reference. - -**Verdict: REJECTED.** The code explicitly handles identity preservation — this is deliberate defensive design, not a bug. - -**Root cause of sub-agent error:** The sub-agent inferred "recomputed array" from the `XmlLanguageDisplay.Options()` factory method without reading the `RefreshXmlLanguageDisplayOptions` caller that bridges the factory output into a persistent collection with identity-preserving mutation. - -## Downgraded Findings (Severity Calibration) - -| Original | Calibrated | Rationale | -|----------|------------|-----------| -| Phase 2 P1 (event race) | **P2** | Real C# event race, but impact bounded to logging path in a desktop app. Worst case: rare NRE in `ILogger.Log`, not data corruption. | -| Phase 3 P1 (culture mutation) | **P2** | Real design anti-pattern, but `Options()` call sites are UI-thread-bound and infrequent (culture change only). Cross-thread impact requires a concurrent background operation reading `CurrentCulture` during the brief `using` window. | -| Phase 4 P1 (CFBundleExecutable) | **P2** | Names match by default (no `AssemblyName` override). The real defect is the silent chmod skip, not the name itself. | -| Phase 3 P2 (live refresh leak) | **P3** | `TextToolView.axaml.cs:40` explicitly calls `DetachLiveRefresh()` on DataContext change — the View handles the lifecycle. | -| Phase 4 P2 (no unsubscribe) | **P3** | Main window owns its ViewModel; subscription graph is self-contained and collectable together. | - -## Verification Commands Run by Main Agent - -1. `grep -n "MissingOperatorBeforeFunction" Strings.*.resx` — key presence in all 3 files. -2. `comm -23 <(thrown codes) <(resx keys)` — zero missing diagnostic codes. -3. `grep -rn "ILocalizationService|LocalizationService" --type cs` repo-wide — zero dangling refs. -4. `dotnet build ChapterTool.Avalonia.slnx` — 0 warnings, 0 errors. -5. Full read of `ApplicationLogPanelProvider.cs`, `Info.plist`, `publish.sh` macOS block, `ToolWindowViewModels.cs`, `RefreshXmlLanguageDisplayOptions`. -6. `grep "IsChapterGridEmpty"` — overlay binding verified. -7. `grep "DetachLiveRefresh"` — View calls detach. -8. Empty-catch / `dynamic` / unsafe-cast scan — clean. - -## Assessment of Sub-agent Quality - -- **Phase 6 (Localization):** Excellent — thorough, programmatic, all claims verified. No false positives. -- **Phase 5 (Scripts):** Strong — accurate shell analysis, correct severity calls. -- **Phase 4 (Views):** Good on Info.plist audit and AGENTS.md compliance; slightly over-conservative on severity calibration. -- **Phase 2 (Infra):** Correct dangling-ref audit; P1 was technically right but over-calibrated for a desktop logging path. -- **Phase 3 (VM):** Correct XmlLanguageDisplay culture analysis; **produced a false positive** on selector identity by not reading the refresh method. -- **Phase 1 (Core):** Correct ExpressionService evaluation spot-checks; **produced a false positive** on the missing key by misreading the resx. - -**False-positive rate:** 2 of 13 candidate findings rejected (15%). Both were P1 claims that would have been the highest-severity items — highlighting the importance of main-agent verification for P0/P1 candidates per the skill protocol. diff --git a/docs/review-2026-07-05/fixes-plan.md b/docs/review-2026-07-05/fixes-plan.md deleted file mode 100644 index 0ded5ce7..00000000 --- a/docs/review-2026-07-05/fixes-plan.md +++ /dev/null @@ -1,122 +0,0 @@ -# Fixes Plan - -> Execution-oriented fix plan for findings from the 2026-07-05 code review. -> Ordered by impact × effort. Each fix is independently shippable. - -## Batch 1 — Pre-release Hardening (recommend before next tag) - -### Fix A: Remove ambient culture mutation from XmlLanguageDisplay -- **Addresses:** P2-1 -- **Files:** `src/ChapterTool.Avalonia/ViewModels/XmlLanguageDisplay.cs` -- **Complexity:** Medium (1 file, ~30 lines rewritten) -- **Change:** - 1. Delete `TemporaryCurrentUiCulture` class entirely. - 2. In `LanguageDisplayName`, call `CultureInfo.GetCultureInfo(language.Code).DisplayName` **without** swapping `CurrentCulture`/`CurrentUICulture`. For .NET, `CultureInfo.DisplayName` returns the display name in the culture's own language regardless of ambient UI culture — so the result is deterministic. - 3. For the `"und"` code, keep the existing `localizer.GetString("XmlLanguage.Undetermined")` lookup. - 4. Cache results per `localizer.CurrentCultureName` to avoid repeated `GetCultureInfo` calls. -- **Verification:** Open Settings → XML language selector; verify display names are correct in all 3 UI languages. Run `dotnet test ChapterTool.Avalonia.slnx --no-restore`. - -### Fix B: publish.sh interrupt resilience + executable validation -- **Addresses:** P2-5, P2-6, P3-4 -- **Files:** `scripts/publish.sh` -- **Complexity:** Low (~15 lines added) -- **Change:** - ```bash - # 1. Arg-value guard (line ~19, ~23): - case "$1" in - -Configuration) [[ $# -ge 2 ]] || { echo "ERROR: -Configuration requires a value" >&2; exit 2; }; Configuration="$2"; shift 2 ;; - -Runtime) [[ $# -ge 2 ]] || { echo "ERROR: -Runtime requires a value" >&2; exit 2; }; Runtime="$2"; shift 2 ;; - ... - esac - - # 2. Trap + stage-then-rename for macOS bundle: - trap 'rm -rf "$app_bundle"' INT TERM - # ... stage into "$app_bundle.tmp", then mv "$app_bundle.tmp" "$app_bundle" at the end - trap - INT TERM - - # 3. Hard error on missing executable: - [[ -f "$exe_path" ]] || { echo "ERROR: executable '$exe_path' not found" >&2; exit 1; } - chmod +x "$exe_path" - - # 4. Glob guard: - shopt -s nullglob - items=("$output"/*) - shopt -u nullglob - (( ${#items[@]} > 0 )) || { echo "ERROR: no publish output to bundle" >&2; exit 1; } - ``` -- **Verification:** `./scripts/publish.sh -Runtime osx-arm64` on macOS; verify `.app` launches. Test `-Runtime` with no value → clean error. - -## Batch 2 — Observability & Correctness Hardening - -### Fix C: ExpressionException innerException + OverflowException catch -- **Addresses:** P2-3, P3-1 -- **Files:** `src/ChapterTool.Core/Transform/ExpressionService.cs` -- **Complexity:** Low (~10 lines) -- **Change:** - 1. Add ctor: `ExpressionException(string code, string message, IReadOnlyDictionary? args = null, Exception? innerException = null) : base(message, innerException)`. - 2. Add `OverflowException` to the catch list at the two evaluator catch sites (`:89`, `:147`), mapping to a diagnostic (e.g., `InvalidExpression.Overflow`). -- **Verification:** `dotnet test tests/ChapterTool.Core.Tests --no-restore`. Add a test: `ExpressionService_EvaluatesOverflowExpression_ReturnsDiagnostic`. - -### Fix D: ApplicationLogPanelProvider event invocation local-copy -- **Addresses:** P2-2 -- **Files:** `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs` -- **Complexity:** Trivial (2 lines) -- **Change:** - ```csharp - // Line 106, replace: - EntryAdded?.Invoke(this, entry); - // With: - var handler = EntryAdded; - handler?.Invoke(this, entry); - ``` -- **Verification:** `dotnet test tests/ChapterTool.Infrastructure.Tests --no-restore`. - -### Fix E: IApplicationLogService default interface implementation -- **Addresses:** P2-4 -- **Files:** `src/ChapterTool.Core/Services/IApplicationLogService.cs` -- **Complexity:** Low (3 lines) -- **Change:** Add default impl to the event: - ```csharp - event EventHandler? EntryAdded - { - add { } - remove { } - } - ``` - This makes the event optional for implementers. `ApplicationLogPanelProvider` overrides it with a real event. -- **Verification:** Build passes; existing tests pass. - -## Batch 3 — Minor Lifecycle Hardening (can defer) - -### Fix F: MainWindow unsubscribe on close -- **Addresses:** P3-2 -- **Files:** `src/ChapterTool.Avalonia/Views/MainWindow.axaml.cs` -- **Complexity:** Low -- **Change:** Add `UnsubscribeViewModelCommandState()` method mirroring `SubscribeViewModelCommandState()`, call from `OnClosed` override. - -### Fix G: publish.ps1 try/finally cleanup -- **Addresses:** P3-5 -- **Files:** `scripts/publish.ps1` -- **Complexity:** Low - -### Fix H: Diagnostic placeholder backfill -- **Addresses:** P3-3 -- **Files:** `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` -- **Complexity:** Low -- **Change:** In `LocalizeDiagnostic`, after format, strip any remaining `{token}` patterns with empty string or `[?]`. - -### Fix I: TextToolView detach on DetachedFromVisualTree -- **Addresses:** P3-6 -- **Files:** `src/ChapterTool.Avalonia/Views/Tools/TextToolView.axaml.cs` -- **Complexity:** Low - -## Verification Commands (per AGENTS.md) - -After all fixes: -```powershell -dotnet build ChapterTool.Avalonia.slnx --no-restore -dotnet test ChapterTool.Avalonia.slnx --no-restore -openspec validate --all -``` - -For macOS bundle fix: `./scripts/publish.sh -Runtime osx-arm64` on a macOS host, verify `.app` launches. diff --git a/docs/review-2026-07-05/phase-0-baseline.md b/docs/review-2026-07-05/phase-0-baseline.md deleted file mode 100644 index 2924b9bf..00000000 --- a/docs/review-2026-07-05/phase-0-baseline.md +++ /dev/null @@ -1,86 +0,0 @@ -# Phase 0 — Baseline - -> Code review of `feat/improve-struct` vs `master`, including uncommitted changes. -> Generated: 2026-07-05 (Asia/Shanghai) - -## Branch & Change Surface - -- **Branch**: `feat/improve-struct` -- **Commits ahead of master**: 5 - - `2e2f18f` feat: Add macOS app bundle support and update application icon - - `c8522ba` test: speed up Avalonia unit test execution - - `25e4752` fix: Correct case of 'Full' in blame-hang-dump-type for CI - - `260c053` feat: Enhance ChapterDiagnostic with Arguments and improve error handling in expression evaluation - - `eb42d91` feat: Update store selection process and enhance log service integration -- **Uncommitted**: 12 modified + 3 untracked files (i18n follow-ups + new `XmlLanguageDisplay` ViewModel + new `show-chapter-empty-state` OpenSpec change). -- **Combined diff vs master (committed + uncommitted)**: 73 files changed, +4,161 / −810. - -## Tech Stack & Module Boundaries - -- .NET 10 / Avalonia 11 desktop app for chapter editing. -- Solution: `ChapterTool.Avalonia.slnx` - - `src/ChapterTool.Core` — domain: models, transformations (ExpressionService), importers (text/XML/OGM/cue/etc.), exporters, diagnostics, editing. - - `src/ChapterTool.Infrastructure` — external tools, process exec, settings, infra-backed importers (BDMV/Matroska/MP4 via ffprobe). - - `src/ChapterTool.Avalonia` — UI: ViewModels, Views (.axaml + code-behind), Services, Localization resx. - - `tests/` — Core.Tests, Infrastructure.Tests, Avalonia.Tests (incl. headless). -- Build verified at baseline: **`dotnet build ChapterTool.Avalonia.slnx` → 0 warnings, 0 errors.** - -## Intent of This Branch (per `docs/i18n-audit-and-fix-plan.md` & OpenSpec change) - -1. **i18n refactor** (largest portion of the diff): - - P0: Add ~91 new `Diagnostic.*` keys (3 languages), wire `ChapterDiagnostic.Arguments`, route `LocalizeDiagnostic` through `Localizer.Format`. - - P1: Add `Action.*`, `EditKind.*`, `Operation.*` keys; refactor `ApplyEdit` / `LogDiagnostics` to take localized keys instead of raw English strings. - - P2: New `ExpressionException(code, message, args)` in `ExpressionService`; 18 `InvalidExpression.*` codes. - - P3: `MissingDependency` / external-tool messages localized at the importer boundary. - - P4: **Delete** `ILocalizationService` + `LocalizationService` (Core/Infrastructure) — was unused dead code. -2. **macOS app bundle support**: new `Assets/MacOS/Info.plist`, app-icon assets (`icns`/`ico`/updated `svg`), `csproj` bundle config, new `scripts/publish.{ps1,sh}` helpers. -3. **Empty-grid UI feature** (uncommitted, OpenSpec change `show-chapter-empty-state`): render `chapter-empty.svg` overlay when grid is empty; new headless tests. -4. **New `XmlLanguageDisplay` ViewModel** (untracked): provides display-name localization for the XML chapter language `` selector. - -## Phase Plan (parallel sub-agents) - -| Phase | Layer | Scope | -|---|---|---| -| 1 | Core domain | `src/ChapterTool.Core/**` | -| 2 | Infrastructure | `src/ChapterTool.Infrastructure/**` (incl. deleted-abstraction dangling-ref audit) | -| 3 | Avalonia ViewModels | `ViewModels/**` (incl. new `XmlLanguageDisplay`) | -| 4 | Avalonia Views + bundle config | `Views/**`, `.axaml.cs`, `csproj`, `Info.plist`, `Assets/`, `Services/` | -| 5 | Build / publish | `scripts/**`, `.github/workflows/**`, `.gitignore` | -| 6 | Localization | 3× `resx`, `LocalizationTests.cs` | -| 7 (me) | Cross-cutting differential disproof | Horizontal sweep across all phases | -| 8 (me) | Aggregation | `summary.md`, `fixes-plan.md` | - -## High-Risk Surface Map (preliminary — to be confirmed/denied by phases) - -The following are the most likely defect clusters, listed before reading the diffs so sub-agent findings can be checked against this expectation: - -1. **`XmlLanguageDisplay` global culture mutation** (Phase 3): `TemporaryCurrentUiCulture : IDisposable` mutates `CultureInfo.CurrentCulture`/`CurrentUICulture` process-wide inside a `using`. Race-prone, exception-unsafe if `GetCultureInfo` throws after assignment, and impacts any concurrent culture-sensitive code. **Pre-rated P1 pending Phase 3 evidence.** -2. **`ILocalizationService` deletion dangling references** (Phase 2): If DI registration or any test still references the deleted type → runtime ActivatorException or compile failure. (Build passed → compile is clean; DI/runtime resolution still needs evidence.) -3. **Localization key/placeholder parity** (Phase 6): 91 new keys × 3 languages = lots of room for missing keys or mismatched `{name}` placeholders → runtime `FormatException` or English fallback. -4. **macOS `Info.plist` correctness** (Phase 4): Missing `CFBundleIdentifier`/`CFBundleExecutable`/version keys → bundle won't launch on macOS. -5. **`publish.sh` shell quoting** (Phase 5): Unquoted `$VAR` expansions on paths with spaces → silent partial artifacts. -6. **`ExpressionService` evaluation regression** (Phase 1): The refactor touches the core expression evaluator; verify precedence, div-by-zero, NaN, and that all throw sites now use `ExpressionException` (no dropped error cases). - -## Anti-Pattern Coverage Plan - -The following high-leakage patterns will be checked by every phase AND in the final cross-cutting pass: - -- Default branch on unknown input / unknown diagnostic code / unknown lang code. -- Async failure propagation (cancellation, IO failure → status bar / log panel). -- Half-committed state across persistence + cache + observable collections. -- Deferred-execution context invalidation (culture swap, captured closures, event handlers firing after window close). -- Single-point utility assumptions: encoding (resx UTF-8), time (frame-rate math), collision (lang codes), empty (null `Arguments`). -- Re-entrant UI (rapid button clicks, window close mid-async). - -## Out of Scope (explicitly excluded) - -- `.codex/skills/**` markdown files (tooling docs, no runtime behavior). -- `openspec/changes/**` proposal/design/tasks markdown (planning artifacts). -- Binary icon assets (`icns`/`ico`) — only their wiring (csproj `BuildAction`) is reviewed. -- Subjective translation quality — only structural/behavioral defects (missing keys, placeholder mismatches, UTF-8 mojibake). -- Style/formatting nits. - -## Pre-existing Review Context - -- `docs/review-2026-06-10/` exists but `summary.md` is empty; not a useful baseline. -- `docs/i18n-audit-and-fix-plan.md` documents the i18n refactor as "completed" with green tests; Phase 6 will verify that claim structurally rather than trust it. diff --git a/docs/review-2026-07-05/phase-1-core.md b/docs/review-2026-07-05/phase-1-core.md deleted file mode 100644 index 736b6f26..00000000 --- a/docs/review-2026-07-05/phase-1-core.md +++ /dev/null @@ -1,50 +0,0 @@ -# Phase 1 — Core Domain Review - -> Scope: `src/ChapterTool.Core/**` changes vs `master` (committed + uncommitted). -> Reviewer: sub-agent bg_b9a2378f + main-agent verification. - -## Phase Summary - -The ExpressionService refactor to structured `ExpressionException(code, message, args)` is consistent across all 18 throw sites. **All 18 `InvalidExpression.*` codes have corresponding `Diagnostic.InvalidExpression.*` keys in all three resx files** — verified programmatically by the main agent (`comm -23` between thrown codes and resx keys returned empty). Importer and diagnostic-arguments changes are structurally safe. One observability gap (no innerException) and one pre-existing overflow edge case noted. - -## Files Reviewed - -- `src/ChapterTool.Core/Diagnostics/ChapterDiagnostic.cs` -- `src/ChapterTool.Core/Editing/ChapterEditingService.cs` -- `src/ChapterTool.Core/Exporting/ChapterOutputProjectionService.cs` -- `src/ChapterTool.Core/Importing/Text/OgmChapterImporter.cs` -- `src/ChapterTool.Core/Importing/Text/XmlChapterImporter.cs` -- `src/ChapterTool.Core/Transform/ExpressionService.cs` -- `src/ChapterTool.Core/Services/IApplicationLogService.cs` -- `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs` (boundary) - -## Sub-agent Finding Rejected (FALSE POSITIVE) - -### ~~[P1] MissingOperatorBeforeFunction code missing localization key~~ — **REJECTED** - -The sub-agent claimed `Diagnostic.InvalidExpression.MissingOperatorBeforeFunction` was absent from the resx. **Main-agent verification disproved this**: the key exists at line 161 in all three resx files (`en-US`, `zh-CN`, `ja-JP`). Furthermore, a programmatic `comm -23` between all 18 codes thrown by `ExpressionService.cs` and all `Diagnostic.InvalidExpression.*` keys in `Strings.en-US.resx` returned **zero missing codes**. The i18n refactor's diagnostic-code-to-resx-key coverage is complete. - -## Findings - -### [P2] ExpressionException lacks innerException channel -- **Location:** `src/ChapterTool.Core/Transform/ExpressionService.cs:166-171` -- **Trigger:** Any future throw site that wraps a lower-level exception (parser/format helper) into `ExpressionException` cannot carry the root cause. -- **Impact:** Reduced debuggability. Current throw sites are all direct validation errors (no wrapping needed today), so immediate functional impact is low — but the refactor established a new exception type without the standard `.innerException` ctor, violating the "no swallow of inner exceptions" contract for future maintainers. -- **Fix:** Add `ExpressionException(string code, string message, IReadOnlyDictionary? args = null, Exception? innerException = null)` passing to `base(message, innerException)`. -- **Confidence:** High - -### [P3] Pre-existing overflow risk in double→decimal cast (not introduced by this diff) -- **Location:** `src/ChapterTool.Core/Transform/ExpressionService.cs:~505,508` -- **Trigger:** Extreme-domain inputs (very large exponents, NaN-producing trig/log) cast `double` results to `decimal`, which can throw `OverflowException` — not caught by the existing `InvalidOperationException|FormatException|KeyNotFoundException|DivideByZeroException` handlers. -- **Impact:** Unhandled exception propagates to caller; user sees a crash instead of a diagnostic. -- **Fix:** Add `OverflowException` to the catch list, or clamp NaN/Infinity before cast. -- **Confidence:** Medium (pre-existing, not a regression) - -## 漏检复盘 (Missed-pattern Retrospective) - -- **Unknown input default branch**: Checked `Tokenize`, `ToPostfix`, `EvaluatePostfix` — all unsupported tokens map to explicit diagnostics. Clean. -- **Downstream failure propagation**: Importers still return `ChapterImportResult.Failed(...)` on error. No silent success-on-error introduced. Clean. -- **Half-committed state**: Chapter lists only published on success paths. Clean. -- **Deferred-execution context invalidation**: Expression evaluation is eager over token stream; no captured mutable context. Clean. -- **Single-point utility assumptions**: Placeholder substitution null-safe; diagnostic argument transport null-guarded at call sites. Clean. -- **Evaluation spot-checks**: `1 + 2 * 3` → 7 ✅; `0 ? 2 : 3 + 1` → 4 ✅; `1/0` → DivideByZeroException caught ✅. diff --git a/docs/review-2026-07-05/phase-2-infrastructure.md b/docs/review-2026-07-05/phase-2-infrastructure.md deleted file mode 100644 index 6b022454..00000000 --- a/docs/review-2026-07-05/phase-2-infrastructure.md +++ /dev/null @@ -1,52 +0,0 @@ -# Phase 2 — Infrastructure Review - -> Scope: `src/ChapterTool.Infrastructure/**` changes vs `master`. -> Reviewer: sub-agent bg_5f185fb5 + main-agent verification. - -## Phase Summary - -`ILocalizationService` / `LocalizationService` deletion leaves **zero C# dangling references** repo-wide (confirmed via `git grep`). One event-invocation race found in `ApplicationLogPanelProvider` (calibrated P2 — real but low-impact in a logging path). The new `IApplicationLogService.EntryAdded` event is a breaking interface change for out-of-tree implementers (P2). - -## Files Reviewed - -- `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs` (full read + verified) -- `src/ChapterTool.Infrastructure/Services/IApplicationLogService.cs` -- `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs` - -## Dangling-Reference Audit - -``` -$ git grep -n -E "ILocalizationService|LocalizationService" -- "*.cs" -(no output) -``` - -**Verdict: CLEAN.** Zero C# references to the deleted types remain in production or test code. Mentions in docs/OpenSpec markdown are historical narrative, not runtime risk. - -## Findings - -### [P2] Event invocation race in ApplicationLogPanelProvider -- **Location:** `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs:106` -- **Trigger:** Thread A calls `Capture()` → reaches `EntryAdded?.Invoke(this, entry)`. Thread B unsubscribes (`EntryAdded -= handler`) between the null-check and the delegate invocation. -- **Impact:** Intermittent `NullReferenceException` from the logging path. In practice, low probability (desktop app, logging is near-sequential), but it's the textbook C# event race. An unhandled NRE from `ILogger.Log` could propagate depending on the logging pipeline's exception guards. -- **Fix:** Copy delegate to local before invoking: - ```csharp - var handler = EntryAdded; - handler?.Invoke(this, entry); - ``` - Note: invocation is correctly outside the lock (avoids reentrancy deadlock) — keep it there. -- **Confidence:** High (the race is real; severity calibrated from sub-agent's P1 to P2 given low-impact logging context) - -### [P2] IApplicationLogService.EntryAdded is a breaking interface change -- **Location:** `src/ChapterTool.Core/Services/IApplicationLogService.cs:7` -- **Trigger:** Any external implementer of `IApplicationLogService` compiled against the old interface. -- **Impact:** Compile-time break for out-of-tree consumers. In-repo, the single implementer (`ApplicationLogPanelProvider`) is updated. -- **Fix:** If backward-compat matters, add a default interface implementation: `event EventHandler? EntryAdded { add { } remove { } }` or document the contract change. -- **Confidence:** Medium (repo-local correctness is fine) - -## 漏检复盘 (Missed-pattern Retrospective) - -- **悬空引用**: 全仓 C# 搜索确认删除类型零残留。Clean. -- **接口新增成员**: 仓内实现已覆盖；对外破坏已报 P2。 -- **异步阻塞**: 未引入 `.Result` / `.Wait()`。Clean. -- **类型安全**: 无 `dynamic`、危险强转、空 `catch`。Clean. -- **事件并发反路径**: 命中 `EntryAdded?.Invoke` 竞态（P2）。 diff --git a/docs/review-2026-07-05/phase-3-viewmodels.md b/docs/review-2026-07-05/phase-3-viewmodels.md deleted file mode 100644 index a6089940..00000000 --- a/docs/review-2026-07-05/phase-3-viewmodels.md +++ /dev/null @@ -1,55 +0,0 @@ -# Phase 3 — Avalonia ViewModels Review - -> Scope: `src/ChapterTool.Avalonia/ViewModels/**` changes vs `master`. -> Reviewer: sub-agent bg_cbd33c95 + main-agent verification. - -## Phase Summary - -The `MainWindowViewModel` i18n refactor is coherent — `LocalizeDiagnostic` uses replace-based formatting (no `FormatException` vector), and all `ApplyEdit`/`LogDiagnostics` call sites within the file now pass localized keys. The new `XmlLanguageDisplay` has a legitimate ambient-culture-mutation design concern (P2). The sub-agent's "selector identity mismatch" finding was **rejected as a false positive** after code inspection. - -## Files Reviewed - -- `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs` (full) -- `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs` (full) -- `src/ChapterTool.Avalonia/ViewModels/ToolWindowViewModels.cs` (full) -- `src/ChapterTool.Avalonia/ViewModels/XmlLanguageDisplay.cs` (full, untracked new file) - -## Sub-agent Finding Rejected (FALSE POSITIVE) - -### ~~[P1] XML language selected-item identity mismatch~~ — **REJECTED** - -The sub-agent claimed `XmlLanguageDisplayOptions` "returns a new array every getter call." **Main-agent verification disproved this:** - -- `XmlLanguageDisplayOptions => xmlLanguageDisplayOptions` returns the **same persistent `ObservableCollection`** every call (`MainWindowViewModel.cs:289`). -- `RefreshXmlLanguageDisplayOptions` preserves identity by design (`MainWindowViewModel.cs:1463-1487`): - - If count unchanged: mutates items in-place via `UpdateFrom()` — item identity preserved. - - If count changed: clears + re-adds — collection identity preserved. -- The `SelectedXmlLanguageDisplayOption` setter matches by **value** (`option.MainText`, `StringComparison.OrdinalIgnoreCase`), not by reference (`:304-305`). -- The getter indexes by `XmlLanguageIndex` integer (`:296-298`), not by reference. - -This is deliberate defensive code that avoids the exact problem the sub-agent flagged. - -## Findings - -### [P2] Ambient culture mutation in XmlLanguageDisplay static helper -- **Location:** `src/ChapterTool.Avalonia/ViewModels/XmlLanguageDisplay.cs:27, 63-82` -- **Trigger:** `XmlLanguageDisplay.Options(localizer)` temporarily sets `CultureInfo.CurrentCulture` / `CurrentUICulture` (process-global) inside a `using var _ = new TemporaryCurrentUiCulture(...)`. -- **Impact:** During the `using` window, ALL threads in the process see the swapped culture. If a background operation (chapter save, expression eval, formatting) runs concurrently and reads `CurrentCulture`, it gets the wrong culture transiently. The `Options()` call itself is safe on the UI thread (exception-safe: `GetCultureInfo` runs before assignment in the ctor), but the side-effect scope is process-wide. In practice, severity is bounded because `Options()` is called only during culture-change refresh and tool-window construction — but it's an anti-pattern in a static helper. -- **Fix:** Remove `TemporaryCurrentUiCulture` entirely. Compute display labels via explicit resource lookup (`localizer.GetString("XmlLanguage." + code)`) with `CultureInfo.GetCultureInfo(code).DisplayName` called WITHOUT mutating ambient state — `DisplayName` respects the culture's own display name regardless of `CurrentUICulture` for neutral cultures. Cache results per UI culture. -- **Confidence:** High (the design issue is real; severity calibrated from sub-agent's P1 to P2) - -### [P3] Localized diagnostic template can leave unresolved placeholders -- **Location:** `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:1378-1393` -- **Trigger:** Localization template expects placeholders absent from `diagnostic.Arguments` (non-null dictionary missing keys). -- **Impact:** User-visible unresolved `{token}` in status/log text. No crash (replace-based formatter). -- **Fix:** Backfill missing keys from a known-defaults map, or strip unresolved tokens. -- **Confidence:** Medium - -## 漏检复盘 (Missed-pattern Retrospective) - -- **默认分支/未知输入**: `LocalizeDiagnostic` key-miss fallback handled; XmlLanguage index bounds checked. Clean. -- **异步失败路径**: `TextToolViewModel.OnEntryAdded` marshals to UI thread via `Dispatcher.UIThread.Post`. Clean. -- **半完成状态窗口**: `TemporaryCurrentUiCulture` constructor reads culture before assignment — no partial swap on ctor failure. But ambient-mutation design itself is the systemic risk (P2). -- **延迟执行上下文失效**: `Options()` static call lacks call-context guard — flagged as P2. -- **隐式协议/兼容前提**: `SelectorDisplayOption` reference-equality concern investigated and **rejected** — code uses `UpdateFrom` + value-matching. -- **ApplyEdit/LogDiagnostics call sites**: All updated to localized keys; no remaining raw-English callers in-file. diff --git a/docs/review-2026-07-05/phase-4-views.md b/docs/review-2026-07-05/phase-4-views.md deleted file mode 100644 index 50404a47..00000000 --- a/docs/review-2026-07-05/phase-4-views.md +++ /dev/null @@ -1,80 +0,0 @@ -# Phase 4 — Avalonia Views, Code-Behind & Bundle Config Review - -> Scope: `src/ChapterTool.Avalonia/Views/**`, `csproj`, `Assets/`, `Services/`. -> Reviewer: sub-agent bg_1c8af626 + main-agent verification. - -## Phase Summary - -No P0 found. The macOS `Info.plist` has all required keys with valid values; `CFBundleExecutable=ChapterTool.Avalonia` matches the default assembly name. The real risk is the **silent chmod skip** in `publish.sh` (calibrated P2). Empty-grid overlay binds correctly to `IsChapterGridEmpty` and respects AGENTS.md layout rules. Code-behind async handlers have try/catch. - -## Files Reviewed - -- `src/ChapterTool.Avalonia/Views/MainWindow.axaml` + `.axaml.cs` -- `src/ChapterTool.Avalonia/Views/Tools/SettingsToolView.axaml` -- `src/ChapterTool.Avalonia/Views/Tools/TextToolView.axaml` + `.axaml.cs` -- `src/ChapterTool.Avalonia/ChapterTool.Avalonia.csproj` -- `src/ChapterTool.Avalonia/Assets/MacOS/Info.plist` -- `src/ChapterTool.Avalonia/Services/{AvaloniaWindowService,RuntimeChapterLoadService,RuntimeChapterSaveService}.cs` - -## Info.plist & Bundle Config Audit - -All required keys present and valid: - -| Key | Value | Status | -|-----|-------|--------| -| `CFBundleName` | `ChapterTool` | ✅ | -| `CFBundleDisplayName` | `ChapterTool` | ✅ | -| `CFBundleIdentifier` | `com.tautcony.chaptertool` | ✅ reverse-DNS | -| `CFBundleVersion` | `1.0.0` | ✅ numeric | -| `CFBundleShortVersionString` | `1.0.0` | ✅ numeric | -| `CFBundlePackageType` | `APPL` | ✅ | -| `CFBundleExecutable` | `ChapterTool.Avalonia` | ⚠️ matches default assembly name, but no validation in publish script | -| `CFBundleIconFile` | `app-icon.icns` | ✅ | -| `LSMinimumSystemVersion` | `10.15` | ✅ | -| `NSHighResolutionCapable` | `true` | ✅ | -| `LSApplicationCategoryType` | `public.app-category.developer-tools` | ✅ | - -**csproj**: No explicit `AssemblyName` → defaults to `ChapterTool.Avalonia` (matches `CFBundleExecutable`). No `PublishSingleFile` (no rename risk). `ApplicationIcon` → `Assets\Icons\app-icon.ico` ✅. - -## AGENTS.md UI Compliance - -- No `Canvas`/absolute positioning in reviewed XAML ✅ -- DataGrid columns have `MinWidth` (56, 105.6, 144, 76.8) ✅ -- Empty-grid overlay: `HorizontalAlignment/VerticalAlignment=Center`, `IsHitTestVisible=False`, has automation id ✅ -- Overlay binds `IsVisible="{Binding IsChapterGridEmpty}"` → `Rows.Count == 0` with `OnPropertyChanged` at row mutation ✅ -- No visible Canvas regression ✅ - -## Findings - -### [P2] CFBundleExecutable name consistency + silent chmod skip in publish.sh -- **Location:** `src/ChapterTool.Avalonia/Assets/MacOS/Info.plist:19-20` + `scripts/publish.sh:94-95` -- **Trigger:** `publish.sh` does `[[ -f "$exe_path" ]] && chmod +x "$exe_path"` — if the executable doesn't exist with the expected name `ChapterTool.Avalonia`, the `chmod` is **silently skipped** and the bundle ships without a valid executable. LaunchServices will refuse to launch the `.app`. -- **Impact:** macOS bundle launch failure with no diagnostic at publish time. The names match by default today (no `AssemblyName` override), but there's no assertion/validation to catch drift if assembly naming changes. -- **Fix:** Make the executable check a hard error: - ```bash - [[ -f "$exe_path" ]] || { echo "ERROR: expected executable '$exe_path' not found" >&2; exit 1; } - chmod +x "$exe_path" - ``` -- **Confidence:** High (the silent-skip is real; the name-match risk is conditional on future changes) - -### [P3] MainWindow subscribes commands/Rows without unsubscribe on close -- **Location:** `src/ChapterTool.Avalonia/Views/MainWindow.axaml.cs:498-506` -- **Trigger:** `SubscribeViewModelCommandState()` attaches `CanExecuteChanged` to 18 commands + `Rows.CollectionChanged`. No `OnClosed` / `Detached` unsubscribe in the file. -- **Impact:** For the **main window** (which owns its ViewModel), the subscription graph is self-contained — when the window closes, both subscriber and source are collectable together. Risk only materializes if a ViewModel outlives the window (multi-window scenario). Low practical impact for single-window desktop app. -- **Fix:** Override `OnClosed` and detach, or use weak-event pattern. Defensive but not urgent. -- **Confidence:** High (the missing unsubscribe is real; severity calibrated from sub-agent's P2 to P3) - -### [P3] TextToolView control-level event handler permanently attached -- **Location:** `src/ChapterTool.Avalonia/Views/Tools/TextToolView.axaml.cs:18` -- **Trigger:** `DataContextChanged += OnDataContextChanged` attached without corresponding detach. -- **Impact:** Low in Avalonia control lifecycle. The view does call `subscribedViewModel.DetachLiveRefresh()` on DataContext change (`:40`), which handles the subscription leak the sub-agent flagged. Residual reference risk only in complex host scenarios. -- **Fix:** Add detach in `DetachedFromVisualTree` if control can outlive host transitions. -- **Confidence:** Medium - -## 漏检复盘 (Missed-pattern Retrospective) - -- **默认分支/非法输入**: `RuntimeChapterLoadService` has explicit failure diagnostics for invalid paths/unsupported extensions. Clean. -- **异步失败路径**: `OnDrop` has try/catch with status writeback (`:230-255`). Load/Save services propagate IO exceptions. Clean. -- **半提交状态窗口**: `RuntimeChapterSaveService` only appends "Saved" diagnostic after successful file write. Clean. -- **延迟执行上下文失效**: `ScheduleWindowCommandRefresh` uses `Dispatcher.UIThread.Post` — but no close-time unsubscribe (P3). -- **UI 布局规则**: Compliant with AGENTS.md (no Canvas, MinWidth present, overlay responsive). diff --git a/docs/review-2026-07-05/phase-5-scripts.md b/docs/review-2026-07-05/phase-5-scripts.md deleted file mode 100644 index a8ab2447..00000000 --- a/docs/review-2026-07-05/phase-5-scripts.md +++ /dev/null @@ -1,86 +0,0 @@ -# Phase 5 — Build / Publish Scripts Review - -> Scope: `scripts/**`, `.github/workflows/dotnet-ci.yml`, `.gitignore`. -> Reviewer: sub-agent bg_183f5248 + main-agent verification. - -## Phase Summary - -No P0/P1 found. `publish.sh` has `set -euo pipefail`, no `eval`/backticks, mostly correct quoting. Main risks are argument-parse robustness under `set -u` and interrupt resilience during macOS bundle restructuring. `publish.ps1` is solid PowerShell. CI workflow change (blame-hang flags) is benign. - -## Files Reviewed - -- `scripts/publish.sh` (new, 103 lines) -- `scripts/publish.ps1` (modified, 63 lines) -- `.github/workflows/dotnet-ci.yml` (1 line changed) -- `.gitignore` (1 line: `.omo/`) - -## Shell-script Audit (publish.sh) - -| Line | Pattern | Verdict | -|------|---------|---------| -| `:8` | `set -euo pipefail` | ✅ | -| throughout | quoting | ✅ mostly correct (`"$repo_root"`, `"$output"`) | -| `:8` | no `eval`/backticks | ✅ | -| `:76` | `mkdir -p` | ✅ idempotent | -| `:38-39` | `BASH_SOURCE[0]` for CWD-independence | ✅ | -| `:50-55` | `dotnet publish` args | ✅ valid | -| `:19,23` | arg value guard | ⚠️ missing `$#` check before consuming `$2` | -| `:79` | `for item in "$output"/*` | ⚠️ no `nullglob` → literal `*` on empty | -| `:82` | `mv "$item"` loop | ⚠️ non-atomic, no trap | -| `:94-95` | `[[ -f ... ]] && chmod +x` | ⚠️ silent skip if exe missing (see Phase 4) | - -No secrets/credentials found. RID list consistent with CI matrix (`win-x64`, `linux-x64`, `osx-x64`, `osx-arm64`). - -## PowerShell Audit (publish.ps1) - -- `param()` + `$ErrorActionPreference = "Stop"` ✅ -- CWD-independent via `$PSScriptRoot` ✅ -- `Test-Path` guard before `Remove-Item` ✅ -- `Copy-Item -Force`, `New-Item -ItemType Directory -Force` ✅ -- Missing `[CmdletBinding()]` and `#Requires -Version` (minor) -- No rollback on interrupt during move (`:36-38`) — P3 - -## Findings - -### [P2] Missing value guard in bash arg parsing -- **Location:** `scripts/publish.sh:19, 23` -- **Trigger:** `./scripts/publish.sh -Runtime` (trailing key without value). Under `set -u`, `$2` is unbound → abrupt exit with cryptic shell error. -- **Impact:** Poor operator UX; brittle automation. -- **Fix:** - ```bash - [[ $# -ge 2 ]] || { echo "ERROR: $1 requires a value" >&2; exit 2; } - ``` -- **Confidence:** High - -### [P2] No trap/cleanup on interrupt during macOS bundle restructuring -- **Location:** `scripts/publish.sh:75-83` -- **Trigger:** SIGINT/termination or command failure mid-loop while moving files into `.app/Contents/MacOS/`. -- **Impact:** Partial bundle state (some files in root, some in `.app`). Re-runs fail or require manual cleanup. -- **Fix:** Add `trap` for `INT TERM` with cleanup of partially built `.app`, or stage into temp dir then atomic `mv` rename. -- **Confidence:** Medium-High - -### [P3] Glob-empty edge case on literal pattern -- **Location:** `scripts/publish.sh:79-83` -- **Trigger:** Empty output dir (interrupted prior step). -- **Impact:** `mv` tries literal `*` → noisy failure. -- **Fix:** `shopt -s nullglob` in local scope or guard with array-length check. -- **Confidence:** High - -### [P3] publish.ps1 no rollback on interrupt during move -- **Location:** `scripts/publish.ps1:36-38` -- **Trigger:** Interrupt during `Copy-Item`/`Remove-Item` sequence. -- **Impact:** Partial artifact state. -- **Fix:** `try/finally` with cleanup, or stage-then-rename pattern. -- **Confidence:** Medium - -## CI Workflow Diff - -`.github/workflows/dotnet-ci.yml:30`: `dotnet test` gains `--blame-hang --blame-hang-timeout 10m --blame-hang-dump-type full`. Improves hang diagnostics. Does not alter matrix, runtime list, or publish wiring. **Benign.** - -## 漏检复盘 (Missed-pattern Retrospective) - -- **默认分支/未知输入**: 参数解析覆盖；发现 bash 缺 `$2` 存在性保护 (P2)。 -- **异步/中断路径**: 串行命令链无竞态；中断导致半成品状态风险存在 (P2/P3)。 -- **半提交状态窗口**: macOS bundle 重组存在"先删后搬"窗口 (P2)。 -- **协议/隐式约定**: README `-Runtime`/`-SelfContained` 与脚本一致；CI matrix 未破坏。 -- **安全边界**: 无 `eval`/反引号/未引用命令替换/密钥泄露。 diff --git a/docs/review-2026-07-05/phase-6-localization.md b/docs/review-2026-07-05/phase-6-localization.md deleted file mode 100644 index fbae1c57..00000000 --- a/docs/review-2026-07-05/phase-6-localization.md +++ /dev/null @@ -1,77 +0,0 @@ -# Phase 6 — Localization Consistency Review - -> Scope: 3× `Strings.*.resx` + `LocalizationTests.cs`. -> Reviewer: sub-agent bg_0defc2e9 + main-agent verification. - -## Phase Summary - -**No structural i18n defects found.** All three resx files have exactly 234 keys each, with zero missing/extra keys across languages. All 62 keys containing placeholders have identical placeholder name-sets across all three languages. No duplicate keys, no XML malformation, no UTF-8 mojibake detected. The i18n refactor's structural integrity is solid. - -## Files Reviewed - -- `src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx` (719 lines) -- `src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx` (719 lines) -- `src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx` (719 lines) -- `tests/ChapterTool.Avalonia.Tests/Localization/LocalizationTests.cs` (163 lines) - -## Key Parity Table - -| Culture | Key Count | Missing vs en-US | Extra vs en-US | Duplicates | -|---------|-----------|-------------------|-----------------|------------| -| en-US | 234 | — | — | 0 | -| ja-JP | 234 | 0 | 0 | 0 | -| zh-CN | 234 | 0 | 0 | 0 | - -**Verdict: PERFECT PARITY.** - -## Placeholder Parity - -- Total keys with placeholders: **62** -- Placeholder mismatches across languages: **0** -- Positional `{0}` placeholders: **0** (all use named `{name}` convention) -- Named-placeholder consistency: **100%** - -Spot-checked high-risk keys: `Log.Diagnostic` (6 placeholders: code/details/location/message/operation/severity) — identical in all 3 languages. `Log.ImportOption` (9 placeholders) — identical in all 3 languages. - -## Main-Agent Cross-Verification - -The main agent independently verified ExpressionService diagnostic-code coverage: - -``` -$ comm -23 <(grep -oE 'InvalidExpression\.[A-Za-z]+' ExpressionService.cs | sort -u) \ - <(grep -oE 'name="Diagnostic\.InvalidExpression\.[A-Za-z]+"' en-US.resx | ... | sort -u) -(empty — zero missing codes) -``` - -All 18 `InvalidExpression.*` codes thrown by `ExpressionService.cs` have corresponding `Diagnostic.InvalidExpression.*` keys in the resx. **Coverage complete.** - -## XML / UTF-8 Health - -- All three files: well-formed XML ✅ -- CJK spot-check (byte-level): `日本語`, `簡体字中国語`, `简体中文` — all valid UTF-8, no mojibake signatures (`Ã`, `Â`, U+FFFD) ✅ -- No empty `` where English counterpart is non-empty ✅ - -## Test Coverage - -`LocalizationTests.cs` enforces: -- Key parity: `SupportedCulturesHaveMatchingResourceKeys()` ✅ -- Placeholder parity: `LocalizedFormatStringsUseCompatiblePlaceholders()` ✅ -- Encoding artifacts: `NonEnglishResourcesDoNotContainEncodingArtifacts()` ✅ -- Runtime formatting: `LocalizerFallsBackAndFormatsMessages()` ✅ -- New diagnostic smoke test: `DiagnosticKeysLocalizeAcrossCultures()` ✅ - -**No coverage gap requiring a defect ticket.** - -## Findings - -**None.** This phase is clean. - -## 漏检复盘 (Missed-pattern Retrospective) - -- 键集合不一致: 三语 234 键一致，差集为空。Clean. -- 占位符不一致: 62 键逐一比对，完全一致。Clean. -- 命名/位置占位符混用: 未发现 `{0}` 位置参数。Clean. -- 重复 key: 三文件均无重复。Clean. -- XML 结构损坏: 三文件解析通过。Clean. -- UTF-8 损坏: CJK 字节级 spot-check 正常。Clean. -- 空翻译值: 无非英文空值。Clean. diff --git a/docs/review-2026-07-05/phase-7-crosscutting.md b/docs/review-2026-07-05/phase-7-crosscutting.md deleted file mode 100644 index 521446d7..00000000 --- a/docs/review-2026-07-05/phase-7-crosscutting.md +++ /dev/null @@ -1,74 +0,0 @@ -# Phase 7 — Cross-cutting Differential Disproof Pass - -> Horizontal sweep across all phases, independent of phase ordering. Goal: catch systemic patterns that individual phase reviews might miss. - -## Methodology - -Instead of reviewing by layer, this pass sweeps by **anti-pattern category** across the entire diff, looking for instances that might have fallen between phase boundaries. - -## Sweep Results - -### 1. All dispatch/command/protocol entries: default branch, param validation, failure回传 - -| Entry point | Default branch | Param validation | Failure propagation | Verdict | -|-------------|----------------|-------------------|---------------------|---------| -| `ExpressionService.Tokenize/ToPostfix/EvaluatePostfix` | ✅ all token types → explicit diagnostic | ✅ | ✅ `ExpressionException` | Clean | -| Importers (OGM/XML/BDMV) | ✅ unknown format → `ChapterImportResult.Failed` | ✅ extension check | ✅ diagnostics returned | Clean | -| `LocalizeDiagnostic` | ✅ key-miss → fallback to `diagnostic.Message` | ✅ null args guard | ✅ replace-based (no FormatException) | Clean | -| `XmlLanguageDisplay.LanguageDisplayName` | ✅ unknown code → `EnglishDisplayName` fallback | ✅ CultureNotFoundException catch | ✅ | Clean | -| `publish.sh` arg parser | ⚠️ missing `$2` guard | ❌ | `set -u` abort (P2) | Flagged | - -### 2. All async chains: failure, cancellation, timeout, idempotency, context invalidation - -| Async path | Failure handling | Cancellation | Reentrancy | Verdict | -|------------|-----------------|--------------|------------|---------| -| `MainWindow.OnDrop` (async void) | ✅ try/catch → status | N/A (no token) | ✅ command CanExecute | Clean | -| `MainWindow.OnOpened` (async void) | ✅ per Phase 4 | — | — | Clean | -| `MainWindow.OnKeyDown` (async void) | ✅ per Phase 4 | — | command-level | Clean | -| `TextToolViewModel.OnEntryAdded` | ✅ UI-thread marshalled | N/A | ✅ refresh is idempotent | Clean | -| `ColorSettingsViewModel.LoadAsync` | ✅ store-null guard | `CancellationToken.None` | ✅ | Clean | - -**No new `.Result` / `.Wait()` introduced anywhere in the diff.** - -### 3. All state-write chains: "half-committed state from ordering errors" - -| State write | Atomicity | Rollback | Verdict | -|-------------|-----------|----------|---------| -| `RuntimeChapterSaveService` | ✅ "Saved" diagnostic only after successful write | N/A (file write) | Clean | -| `RefreshXmlLanguageDisplayOptions` | ✅ count-check → either clear+add or in-place UpdateFrom | N/A | Clean | -| `ApplicationLogPanelProvider.Capture` | ✅ entry built before lock; added under lock; capacity trim under same lock | N/A | Clean | -| `publish.sh` macOS bundle move | ❌ non-atomic multi-file `mv` loop, no trap | ❌ no rollback (P2) | Flagged | - -### 4. All rebuild/batch/cleanup/migration chains: "remove-then-rebuild data window" - -| Rebuild path | Window risk | Verdict | -|--------------|-------------|---------| -| `RefreshXmlLanguageDisplayOptions` clear+add (count change) | Brief empty collection during clear+add — but UI binding reads on `PropertyChanged` which fires AFTER rebuild completes | Clean | -| `LanguageToolViewModel.ReplaceLanguages` clear+add | Same pattern — `OnPropertyChanged` after rebuild | Clean | -| `publish.sh` `rm -rf "$app_bundle"` then rebuild | Window exists but acceptable for publish artifact (not runtime data) | Acceptable | - -### 5. All content-rendering / rich-text / export chains: security boundary, scale - -| Render point | Input source | Scale boundary | Verdict | -|--------------|-------------|----------------|---------| -| `TextToolViewModel.Format` (JSON/XML pretty-print) | User chapter export text | ✅ catches `JsonException`/`XmlException` → returns raw text | Clean | -| `HighlightJson`/`HighlightXml` | Line-level text | O(n) per line, no regex backtracking | Clean | -| `LocalizeDiagnostic` | Diagnostic args (bounded set) | Replace-based, no composite format injection | Clean | - -### 6. All high-leverage utility functions: encoding, time, collision, naming, compatibility - -| Utility | Risk | Verdict | -|---------|------|---------| -| `XmlLanguageDisplay` culture swap | Process-global mutation (P2) | Flagged | -| `ExpressionService` double→decimal cast | OverflowException uncaught (P3, pre-existing) | Flagged | -| `Uri.IsHexDigit` usage in color parsing | ✅ correct hex validation | Clean | -| `AppLanguage.Normalize` | ✅ culture-name normalization | Clean | -| resx placeholder formatting | ✅ named-only, verified parity | Clean | - -## New Findings from This Pass - -**None.** All patterns flagged in this sweep were already captured by the phase reviews. No cross-cutting issue fell between phase boundaries. - -## Verdict - -The phase decomposition + this horizontal sweep provide **complete coverage** of the diff's high-risk surface. The two highest-risk categories (async/concurrency and state-write atomicity) are clean except for the already-flagged `publish.sh` interrupt window and `XmlLanguageDisplay` ambient mutation. diff --git a/docs/review-2026-07-05/summary.md b/docs/review-2026-07-05/summary.md deleted file mode 100644 index a5b6ea6e..00000000 --- a/docs/review-2026-07-05/summary.md +++ /dev/null @@ -1,107 +0,0 @@ -# Code Review Summary — `feat/improve-struct` vs `master` - -> Review date: 2026-07-05 -> Scope: all changes (committed + uncommitted) on `feat/improve-struct` vs `master` -> 73 files changed, +4,161 / −810 lines - -## Verdict - -**No P0 (critical/blocker) defects found.** The branch is structurally sound and the build passes (0 warnings / 0 errors). The i18n refactor — the largest portion of the diff — is **structurally complete**: all 234 keys × 3 languages are in parity, all 62 placeholder keys match across languages, and all 18 `InvalidExpression.*` diagnostic codes have resx coverage. The `ILocalizationService` deletion leaves zero dangling references. - -The 12 confirmed findings are all P2/P3 and cluster around three themes: (1) a culture-mutation anti-pattern in a new helper, (2) interrupt-resilience gaps in publish scripts, and (3) minor lifecycle/observability hardening. - -## Finding Count by Severity - -| Severity | Count | Theme | -|----------|-------|-------| -| **P0** | 0 | — | -| **P1** | 0 | — | -| **P2** | 6 | Culture mutation, event race, interface break, arg guard, interrupt cleanup, bundle validation | -| **P3** | 6 | Overflow edge case, unsub lifecycle, unresolved placeholders, glob edge case, ps1 rollback, view detach | -| **Rejected FPs** | 2 | Sub-agent false positives caught by main-agent verification | - -## Findings (P2 → P3) - -### P2-1: Ambient culture mutation in `XmlLanguageDisplay` -- **File:** `src/ChapterTool.Avalonia/ViewModels/XmlLanguageDisplay.cs:27, 63-82` -- `TemporaryCurrentUiCulture` mutates `CultureInfo.CurrentCulture`/`CurrentUICulture` process-wide inside a `using`. Concurrent background operations reading `CurrentCulture` during this window get the wrong culture. -- **Fix:** Remove ambient mutation; use explicit resource lookup + `CultureInfo.DisplayName` without swapping ambient state. - -### P2-2: Event invocation race in `ApplicationLogPanelProvider` -- **File:** `src/ChapterTool.Infrastructure/Platform/ApplicationLogPanelProvider.cs:106` -- `EntryAdded?.Invoke(this, entry)` without local-copy pattern — standard C# event race on concurrent unsubscribe. -- **Fix:** `var handler = EntryAdded; handler?.Invoke(this, entry);` - -### P2-3: `ExpressionException` lacks `innerException` channel -- **File:** `src/ChapterTool.Core/Transform/ExpressionService.cs:166-171` -- New exception type has no `innerException` constructor — future wrapping sites can't carry root cause. -- **Fix:** Add `Exception? innerException = null` ctor overload. - -### P2-4: Breaking interface change on `IApplicationLogService` -- **File:** `src/ChapterTool.Core/Services/IApplicationLogService.cs:7` -- New `EntryAdded` event breaks out-of-tree implementers. In-repo implementer updated. -- **Fix:** Default interface implementation or version the contract if external consumers exist. - -### P2-5: `publish.sh` missing arg-value guard -- **File:** `scripts/publish.sh:19, 23` -- `-Runtime` without value → `set -u` abort with cryptic error. -- **Fix:** `[[ $# -ge 2 ]] || { echo "ERROR: $1 requires a value" >&2; exit 2; }` - -### P2-6: No interrupt cleanup during macOS bundle restructuring + silent chmod skip -- **File:** `scripts/publish.sh:75-83, 94-95` -- Non-atomic multi-file `mv` loop with no `trap`; `chmod +x` silently skipped if executable missing. -- **Fix:** Add `trap` cleanup or stage-then-rename; make exe-existence a hard error. - -### P3-1: Pre-existing `OverflowException` uncaught in ExpressionService -- **File:** `src/ChapterTool.Core/Transform/ExpressionService.cs:~505` -- `double`→`decimal` cast on extreme-domain inputs; not in catch list. Pre-existing, not a regression. - -### P3-2: MainWindow command subscriptions not detached on close -- **File:** `src/ChapterTool.Avalonia/Views/MainWindow.axaml.cs:498-506` -- Self-contained for single-window app; risk only if ViewModel outlives window. - -### P3-3: Localized diagnostic template can leave unresolved placeholders -- **File:** `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:1378-1393` -- Non-null but incomplete `Arguments` dict → unresolved `{token}` in UI text. No crash. - -### P3-4: `publish.sh` glob-empty literal pattern -- **File:** `scripts/publish.sh:79-83` -- Empty output dir → `mv` tries literal `*`. - -### P3-5: `publish.ps1` no rollback on interrupt -- **File:** `scripts/publish.ps1:36-38` - -### P3-6: TextToolView permanent DataContextChanged handler -- **File:** `src/ChapterTool.Avalonia/Views/Tools/TextToolView.axaml.cs:18` - -## Rejected False Positives - -Two sub-agent P1 candidates were **rejected after main-agent verification**: - -1. ~~"MissingOperatorBeforeFunction resx key missing"~~ — key exists at line 161 in all 3 resx files; all 18 `InvalidExpression.*` codes have complete coverage. -2. ~~"Selector identity mismatch from rebuilt list"~~ — code deliberately preserves identity via `UpdateFrom()` in-place mutation + value-based selection matching. - -See `agent-findings.md` for full disproof evidence. - -## Cross-module Systemic Issues - -None identified. The branch's changes are well-contained within their layers. The i18n refactor touches Core (diagnostics), Infrastructure (deleted abstraction), and Avalonia (ViewModels + resx) coherently — the boundary contracts (diagnostic codes → resx keys → UI formatting) are intact end-to-end. - -## Patterns Checked and Confirmed Clean (Cross-cutting Pass) - -- All dispatch entries have default branches and param validation (except `publish.sh` arg guard). -- No new `.Result` / `.Wait()` / `async void` outside event handlers. -- No empty catches, no `dynamic`, no unsafe casts. -- No half-committed state in runtime data paths (chapter save, log capture, option refresh). -- resx key/placeholder parity verified programmatically (234 keys × 3 languages, 0 mismatches). -- Zero dangling references to deleted `ILocalizationService`/`LocalizationService` (repo-wide). -- Build: 0 warnings, 0 errors. - -## Coverage - -- **Fully reviewed:** Core (ExpressionService, diagnostics, importers, editing), Infrastructure (log provider, deleted abstraction), Avalonia ViewModels (all 4), Views (MainWindow + tool views), Info.plist + csproj, publish scripts, CI workflow, 3× resx, localization tests. -- **Out of scope:** `.codex/skills/**` markdown, `openspec/changes/**` planning docs, binary icon assets (only wiring reviewed). - -## Recommended Merge Posture - -**Mergeable as-is for functional correctness.** The P2 items are quality/hardening improvements that don't block the branch's stated goals (i18n completion + macOS bundle support). Recommend addressing P2-1 (culture mutation) and P2-6 (publish interrupt resilience) before the next release tag, as they affect runtime correctness and release-artifact integrity respectively. All other items can be follow-up. From 0066269ccf30d9a298400823ca401c6ae524d44e Mon Sep 17 00:00:00 2001 From: TautCony Date: Wed, 8 Jul 2026 13:57:40 +0800 Subject: [PATCH 03/17] refactor: improve headless test structure and remove redundant tests --- AGENTS.md | 4 +- docs/code-map/testing.md | 4 +- .../LocalizationAndLayoutHeadlessTests.cs | 113 -------------- .../Headless/MainWindowHeadlessTestHost.cs | 24 --- .../Headless/MainWindowHeadlessTests.cs | 99 ------------- .../MainWindowInteractionHeadlessTests.cs | 27 ---- .../Headless/SettingsToolHeadlessTests.cs | 139 ------------------ .../Headless/ToolViewsHeadlessTests.cs | 99 ------------- 8 files changed, 5 insertions(+), 504 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7bed76e6..67dee714 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,6 +52,8 @@ - `Get-Process ChapterTool.Avalonia -ErrorAction SilentlyContinue | Stop-Process` - Do not run multiple `dotnet test` commands for projects in this solution in parallel. The test projects share referenced project `obj/` outputs, and parallel external test processes can fail with locked files such as `src/ChapterTool.Core/obj/Debug/net10.0/ChapterTool.Core.dll`. Prefer the full solution test command above, or run individual test projects sequentially. - Keep Avalonia Headless tests isolated without serializing the entire Avalonia test assembly. Do not reintroduce assembly-level `CollectionBehavior(DisableTestParallelization = true)` for this project; instead put every class containing `[AvaloniaFact]` or `[AvaloniaTheory]` in `AvaloniaHeadlessTestCollection`. The guard test `HeadlessTestCollectionGuardTests` exists to catch missed classes. +- Keep Avalonia Headless tests focused on UI behavior and workflow outcomes. Prefer tests that drive user actions or state changes and then verify the resulting UI state, command routing, localization refresh, selection changes, or persisted behavior. +- Do not add Headless tests that only assert a control exists, a static label renders, a window opens, a screenshot file was written, or a layout has non-zero size unless that assertion is part of a broader user-facing behavior change being verified. - When a test constructs `SettingsToolViewModel` and then calls `LoadAsync` explicitly, pass `autoLoad: false`. Otherwise the constructor starts a background load and the test performs the same initialization twice, which slows Headless runs and can introduce races. - Do not test source/configuration files by reading them as text and asserting strings. This includes `.cs`, `.axaml`, `.csproj`, scripts, CI YAML, README, and docs. Prefer compiled coverage, behavior tests, runtime verification, structured public APIs, or integration checks. - Add or update tests for changed behavior, especially UI layout constraints, UTF-8 labels, import/export behavior, and platform-service boundaries. @@ -70,7 +72,7 @@ - Keep DataGrid columns protected with sensible `MinWidth` values so headers and content do not overlap when resized. - Buttons should center content horizontally and vertically. - Do not expose Windows registry-dependent actions, such as file association, as always-visible primary UI. -- When verifying visual layout changes, capture screenshots at default, wide, and narrow sizes and store them under `artifacts/`. +- When verifying visual layout changes manually, capture screenshots at default, wide, and narrow sizes and store them under `artifacts/`. Do not treat screenshot generation by itself as an automated test assertion. - Preserve accessible names, keyboard navigation, focus behavior, and localization boundaries when changing controls. ## Change And PR Expectations diff --git a/docs/code-map/testing.md b/docs/code-map/testing.md index d4d004ce..db4b9918 100644 --- a/docs/code-map/testing.md +++ b/docs/code-map/testing.md @@ -79,7 +79,7 @@ Fixtures: ## Avalonia Test Map -Use `tests/ChapterTool.Avalonia.Tests` when changing UI shell, view models, runtime UI services, localization, headless layout, or CLI behavior. +Use `tests/ChapterTool.Avalonia.Tests` when changing UI shell, view models, runtime UI services, localization, headless interaction flows, or CLI behavior. High-signal test files: @@ -94,7 +94,7 @@ High-signal test files: - `tests/ChapterTool.Avalonia.Tests/Cli/ChapterToolCliApplicationTests.cs` - localization - `tests/ChapterTool.Avalonia.Tests/Localization/LocalizationTests.cs` -- headless shell/layout/integration +- headless shell/interaction/integration - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowHeadlessTests.cs` - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs` - `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowStateHeadlessTests.cs` diff --git a/tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs b/tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs index 2e276d12..afe36d61 100644 --- a/tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs +++ b/tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs @@ -3,7 +3,6 @@ using Avalonia.VisualTree; using ChapterTool.Avalonia.Localization; using ChapterTool.Avalonia.ViewModels; -using ChapterTool.Avalonia.Views.Tools; using ChapterTool.Infrastructure.Configuration; using System.Text.RegularExpressions; @@ -47,16 +46,6 @@ public async Task Runtime_language_switch_refreshes_main_window_and_tool_text() Assert.False(string.IsNullOrWhiteSpace(xmlLanguageBox.SelectionBoxItem?.ToString())); Assert.StartsWith("jpn（", xmlLanguageBox.SelectionBoxItem?.ToString(), StringComparison.Ordinal); - var languageWindow = await MainWindowHeadlessTestHost.RenderToolAsync(new LanguageToolView(), new LanguageToolViewModel(host.ViewModel)); - try - { - Assert.True(MainWindowHeadlessTestHost.ContainsRenderedTextStatic(languageWindow, "语言")); - Assert.True(MainWindowHeadlessTestHost.ContainsRenderedTextStatic(languageWindow, "应用")); - } - finally - { - languageWindow.Close(); - } } private static string ChapterNameModeSelectionText(MainWindowHeadlessTestHost host) @@ -79,108 +68,6 @@ public async Task Unsupported_or_blank_language_falls_back_without_keys_or_mojib Assert.DoesNotContain(rendered, ContainsEncodingArtifact); } - [AvaloniaFact] - public async Task English_main_window_option_labels_have_room_at_default_and_narrow_widths() - { - using var host = new MainWindowHeadlessTestHost(localizer: new AppLocalizationManager("en-US")); - await host.LoadAsync("movie.txt"); - - foreach (var size in new[] { UiTestSize.Default, UiTestSize.Narrow }) - { - await host.LayoutAtAsync(size); - - foreach (var groupName in new[] - { - "FormatOptionsGroup", - "ChapterNameOptionsGroup", - "OrderShiftOptionsGroup", - "XmlLanguageOptionsGroup", - "ExpressionOptionsGroup" - }) - { - var group = host.RequiredControl(groupName); - var label = MainWindowHeadlessTestHost.RequiredDescendant( - group, - block => block.Classes.Contains("optionLabel"), - $"{groupName} label"); - - Assert.True(label.Bounds.Width > 0, $"{groupName} label width was {label.Bounds.Width}."); - Assert.True(label.Bounds.Height >= 14, $"{groupName} label height was {label.Bounds.Height}."); - } - - var artifact = await host.CaptureArtifactAsync($"main-window-en-{size.ToString().ToLowerInvariant()}.png"); - Assert.True(File.Exists(artifact)); - Assert.True(new FileInfo(artifact).Length > 0); - } - } - - [AvaloniaFact] - public async Task Main_window_layout_captures_default_wide_and_narrow_artifacts() - { - using var host = new MainWindowHeadlessTestHost(MainWindowHeadlessTestHost.ImportResult( - "movie.txt", - MainWindowHeadlessTestHost.Option("OGM", "movie.txt", "Intro", "Middle", "Ending"))); - await host.LoadAsync("movie.txt"); - - foreach (var size in new[] { UiTestSize.Default, UiTestSize.Wide, UiTestSize.Narrow }) - { - await host.LayoutAtAsync(size); - var artifact = await host.CaptureArtifactAsync($"main-window-{size.ToString().ToLowerInvariant()}.png"); - - Assert.True(File.Exists(artifact)); - Assert.True(new FileInfo(artifact).Length > 0); - Assert.True(host.RequiredControl