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() {