From d217e17c27601efcc91861fbcb11e99fd1563c84 Mon Sep 17 00:00:00 2001 From: Andrei Errapart Date: Fri, 7 Aug 2026 23:16:36 +0200 Subject: [PATCH] Fix SchComponent.Comment not persisting through SchDoc/SchLib writers SchDocReader populated Comment from DesignItemId instead of the child "Comment" parameter SchLibReader correctly used, and neither writer ever pushed the property back into that parameter, so edits (and WithComment() on from-scratch components) were silently dropped. SchDocWriter's byte-faithful replay path made this worse for loaded documents: it re-emits the exact bytes captured at read time whenever the primitive count is unchanged, swallowing even direct edits to the backing parameter's Value. - SchDocReader/SchLibReader now derive Comment from the child "Comment" parameter once children are attached, recording a CommentAsRead baseline. - New SchLibWriter.SyncComponentComment pushes an edited Comment back into that parameter (creating one if absent), used by both writers. - SchDocWriter disables its byte-faithful fast path only when a Comment edit is actually detected, so untouched documents still round-trip byte-identical. - Add regression tests in SchDocRoundTripTests/SchLibRoundTripTests covering mutate-after-load and from-scratch WithComment. --- .../Models/Sch/SchComponent.cs | 9 +++++ .../Serialization/Readers/SchDocReader.cs | 19 ++++++++++ .../Serialization/Readers/SchLibReader.cs | 3 ++ .../Serialization/Writers/SchDocWriter.cs | 15 ++++++-- .../Serialization/Writers/SchLibWriter.cs | 31 ++++++++++++++++ .../RoundTrip/SchDocRoundTripTests.cs | 36 +++++++++++++++++++ .../RoundTrip/SchLibRoundTripTests.cs | 30 ++++++++++++++++ 7 files changed, 140 insertions(+), 3 deletions(-) diff --git a/src/OriginalCircuit.Altium/Models/Sch/SchComponent.cs b/src/OriginalCircuit.Altium/Models/Sch/SchComponent.cs index 5f773b8..955bca9 100644 --- a/src/OriginalCircuit.Altium/Models/Sch/SchComponent.cs +++ b/src/OriginalCircuit.Altium/Models/Sch/SchComponent.cs @@ -55,6 +55,15 @@ internal int CountChildPrimitives() /// public string? Comment { get; set; } + /// + /// Snapshot of as it stood immediately after read (whichever source it was + /// derived from). The writer compares the live value against this baseline to tell an explicit edit + /// of the convenience property from an untouched load, so it only pushes a change — and only then + /// disables the byte-faithful replay path — when the caller actually mutated . + /// Null for components built from scratch (no baseline to compare against). + /// + internal string? CommentAsRead { get; set; } + /// public string? DesignatorPrefix { get; set; } diff --git a/src/OriginalCircuit.Altium/Serialization/Readers/SchDocReader.cs b/src/OriginalCircuit.Altium/Serialization/Readers/SchDocReader.cs index bda55c7..dc481b2 100644 --- a/src/OriginalCircuit.Altium/Serialization/Readers/SchDocReader.cs +++ b/src/OriginalCircuit.Altium/Serialization/Readers/SchDocReader.cs @@ -6,6 +6,7 @@ using OriginalCircuit.Altium.Serialization.Compound; using OriginalCircuit.Altium.Serialization.Dto.Sch; using System.Globalization; +using System.Linq; using System.Text; using PinElectricalType = OriginalCircuit.Altium.Models.Sch.PinElectricalType; @@ -335,6 +336,23 @@ private void ReadFileHeader(CompoundFileAccessor accessor, SchDocument document, document.AddPrimitive(primitive); } } + + // Override the placeholder Comment (set from DesignItemId in CreateComponent, before children + // were known) with the actual child "Comment" parameter's value, once attached. Mirrors + // SchLibReader's post-processing so both readers derive Comment from the same field and it is + // writable/round-trippable via the same value SchDocWriter/SchLibWriter sync back to. + foreach (var component in document.Components.Cast()) + { + foreach (var param in component.Parameters) + { + if (string.Equals(param.Name, "Comment", StringComparison.OrdinalIgnoreCase)) + { + component.Comment = param.Value; + component.CommentAsRead = param.Value; + break; + } + } + } } private static SchComponent CreateComponent(Dictionary parameters) @@ -347,6 +365,7 @@ private static SchComponent CreateComponent(Dictionary parameter Name = dto.LibReference ?? dto.DesignItemId ?? string.Empty, Description = dto.ComponentDescription, Comment = dto.DesignItemId, + CommentAsRead = dto.DesignItemId, PartCount = Math.Max(0, dto.PartCount - 1), UniqueId = dto.UniqueId, Location = new CoordPoint(CoordFromDxp(dto.LocationX, dto.LocationXFrac), CoordFromDxp(dto.LocationY, dto.LocationYFrac)), diff --git a/src/OriginalCircuit.Altium/Serialization/Readers/SchLibReader.cs b/src/OriginalCircuit.Altium/Serialization/Readers/SchLibReader.cs index bc66aea..aeba1a9 100644 --- a/src/OriginalCircuit.Altium/Serialization/Readers/SchLibReader.cs +++ b/src/OriginalCircuit.Altium/Serialization/Readers/SchLibReader.cs @@ -389,7 +389,10 @@ private string GetSectionKeyFromRefName(string refName) if (string.Equals(param.Name, "Designator", StringComparison.OrdinalIgnoreCase)) component.DesignatorPrefix = param.Value; else if (string.Equals(param.Name, "Comment", StringComparison.OrdinalIgnoreCase)) + { component.Comment = param.Value; + component.CommentAsRead = param.Value; + } } // Parse PinSymbolLineWidth auxiliary stream to set per-pin SymbolLineWidth values. diff --git a/src/OriginalCircuit.Altium/Serialization/Writers/SchDocWriter.cs b/src/OriginalCircuit.Altium/Serialization/Writers/SchDocWriter.cs index e872dc6..f6e93dd 100644 --- a/src/OriginalCircuit.Altium/Serialization/Writers/SchDocWriter.cs +++ b/src/OriginalCircuit.Altium/Serialization/Writers/SchDocWriter.cs @@ -1,4 +1,5 @@ using System.Globalization; +using System.Linq; using OriginalCircuit.Altium.Models.Sch; using OriginalCircuit.Altium.Serialization.Compound; using OriginalCircuit.Altium.Serialization.Binary; @@ -266,12 +267,20 @@ private static void WriteFileHeader(CompoundFileAccessor cf, SchDocument documen using var ms = new MemoryStream(); using var writer = new BinaryFormatWriter(ms, leaveOpen: true); + // Push SchComponent.Comment into its backing "Comment" parameter before deciding how to + // serialize: the byte-faithful path below replays captured bytes rather than live model state, + // so a Comment edit must be detected here to force the typed path, or it would be silently lost. + var commentEdited = false; + foreach (var component in document.Components.Cast()) + commentEdited |= SchLibWriter.SyncComponentComment(component); + // Byte-faithful path: when the document was read from a file and is unedited, walk the captured // record order (each entry linked to its model object) and re-emit each record's parameters // verbatim — preserving order, duplicate keys and unmodeled parameters. Files built from scratch, - // edited (primitive added/removed), or containing binary-pin records fall through to the typed - // serialization below. - if (document.ReadOrderedRecords is { Count: > 0 } orderedRecords && + // edited (primitive added/removed or Comment changed), or containing binary-pin records fall + // through to the typed serialization below. + if (!commentEdited && + document.ReadOrderedRecords is { Count: > 0 } orderedRecords && document.HeaderParametersOrdered is { Count: > 0 } headerOrdered && document.LoadedPrimitiveCount == document.CountModeledPrimitives()) { diff --git a/src/OriginalCircuit.Altium/Serialization/Writers/SchLibWriter.cs b/src/OriginalCircuit.Altium/Serialization/Writers/SchLibWriter.cs index 015b504..48e07ee 100644 --- a/src/OriginalCircuit.Altium/Serialization/Writers/SchLibWriter.cs +++ b/src/OriginalCircuit.Altium/Serialization/Writers/SchLibWriter.cs @@ -4,6 +4,7 @@ using OriginalCircuit.Altium.Serialization.Binary; using System.Globalization; using System.IO.Compression; +using System.Linq; using System.Text; namespace OriginalCircuit.Altium.Serialization.Writers; @@ -239,8 +240,38 @@ private static string BuildOrderedParamString(List> return sb.ToString(); } + /// + /// Pushes into the child "Comment" parameter's Value + /// (creating the parameter if the component has none), so edits made through the convenience + /// property are reflected in the field Altium actually displays and persists. Without this, the + /// property is populated on read but has nowhere to go on write. Returns true if a parameter's + /// value changed or one was added, so callers with a byte-faithful replay path (SchDocWriter) know + /// to fall back to typed serialization instead of echoing stale captured bytes. + /// + internal static bool SyncComponentComment(SchComponent component) + { + if (component.Comment == null || component.Comment == component.CommentAsRead) + return false; + + var param = component.Parameters.FirstOrDefault(p => + string.Equals(p.Name, "Comment", StringComparison.OrdinalIgnoreCase)) as SchParameter; + + if (param != null) + { + if (param.Value == component.Comment) + return false; + param.Value = component.Comment; + return true; + } + + component.AddParameter(SchParameter.Create("Comment").WithValue(component.Comment).Build()); + return true; + } + private static void WriteComponent(CompoundFileAccessor cf, SchComponent component, Dictionary sectionKeys) { + SyncComponentComment(component); + var sectionKey = sectionKeys.TryGetValue(component.Name, out var key) ? key : GetSectionKeyFromName(component.Name); diff --git a/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchDocRoundTripTests.cs b/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchDocRoundTripTests.cs index a8a9532..6170fbc 100644 --- a/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchDocRoundTripTests.cs +++ b/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchDocRoundTripTests.cs @@ -667,6 +667,42 @@ public void FromScratch_SignalHarness_RoundTrips() Assert.Equal(200, rtHarness.Color); } + [Fact] + public void WriteThenRead_ComponentComment_RoundTrips() + { + var doc = new SchDocument(); + var comp = SchComponent.Create("R1").WithComment("10k").Build(); + doc.AddComponent(comp); + + var readBack = RoundTrip(doc); + + Assert.Equal("10k", ((SchComponent)readBack.Components[0]).Comment); + } + + [SkippableFact] + public void WriteThenRead_RealFile_MutatedComment_Persists() + { + // Regression test for a bug where SchDocWriter's byte-faithful replay path (taken whenever the + // primitive count is unchanged) echoed the original captured bytes verbatim, silently dropping + // an edit to SchComponent.Comment because the edit doesn't add or remove a primitive. + var testDataPath = GetTestDataPath(); + var filePath = Path.Combine(testDataPath, "DAC.SchDoc"); + if (!File.Exists(filePath)) { Skip.If(true, "Test data not available"); return; } + + var original = (SchDocument)new SchDocReader().Read(File.OpenRead(filePath)); + var comp = (SchComponent)original.Components[0]; + var before = comp.Comment; + comp.Comment = "MUTATED_COMMENT_VALUE"; + + using var ms = new MemoryStream(); + new SchDocWriter().Write(original, ms); + ms.Position = 0; + var rt = (SchDocument)new SchDocReader().Read(ms); + + Assert.NotEqual("MUTATED_COMMENT_VALUE", before); + Assert.Equal("MUTATED_COMMENT_VALUE", ((SchComponent)rt.Components[0]).Comment); + } + [SkippableFact] public void WriteThenRead_RealFiles_PreservesComponentProperties() { diff --git a/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchLibRoundTripTests.cs b/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchLibRoundTripTests.cs index 27297ef..b44cf7f 100644 --- a/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchLibRoundTripTests.cs +++ b/tests/OriginalCircuit.Altium.Tests/RoundTrip/SchLibRoundTripTests.cs @@ -688,6 +688,36 @@ public void Component_PreservesAllProperties() Assert.Equal(2, comp.PartCount); } + [Fact] + public void Component_Comment_RoundTrips() + { + // SchComponent.Comment is populated on read from the child "Comment" parameter but, without a + // sync step before write, has nowhere to go back to — WithComment() was silently a no-op. + var original = new SchLibrary(); + var component = SchComponent.Create("RESISTOR").WithComment("10k").Build(); + original.Add(component); + + var readBack = RoundTrip(original); + + Assert.Equal("10k", ((SchComponent)readBack.Components.First()).Comment); + } + + [Fact] + public void Component_MutatedComment_Persists() + { + var original = new SchLibrary(); + var component = SchComponent.Create("RESISTOR").WithComment("10k").Build(); + original.Add(component); + + var afterFirstRoundTrip = RoundTrip(original); + var comp = (SchComponent)afterFirstRoundTrip.Components.First(); + comp.Comment = "22k"; + + var afterSecondRoundTrip = RoundTrip(afterFirstRoundTrip); + + Assert.Equal("22k", ((SchComponent)afterSecondRoundTrip.Components.First()).Comment); + } + [Fact] public void MultipleComponents_PreservesAll() {