Improve Visio authoring safety and round-trip fidelity#1756
Improve Visio authoring safety and round-trip fidelity#1756PrzemyslawKlys merged 13 commits intomasterfrom
Conversation
|
Claude finished @PrzemyslawKlys's task —— View job Pull Request ReviewI've completed my comprehensive review of this significant Visio authoring safety and fidelity improvement PR. Here are my findings: SummaryThis PR implements substantial improvements to OfficeIMO's Visio processing capabilities, focusing on preserving XML fidelity during load/save operations and hardening shape hierarchy management. The changes are well-architected and address important round-trip fidelity issues. Code Quality AssessmentStrengths:
Code Organization:
Critical Issues to Address1. Shape Indexer Safety Issue (VisioShape.cs:392-409) // Current problematic code
_owner.PrepareChildForParent(value);
try {
_owner._children[index] = value;
} catch {
_owner.DetachChild(value); // Old child already detached!
throw;
}
_owner.DetachChild(existing);Recommendation: Validate first, then swap atomically. 2. Connector Ordering Issue (Per Codex review) 3. Missing Page Cell Filter (Per Codex review) Performance & Memory ConsiderationsPositive:
Areas for Optimization:
Security AssessmentNo security concerns identified. The code:
Test Coverage AnalysisExcellent coverage of:
Missing coverage areas (227 lines uncovered):
Automated Review ResponseThe Codex reviews identified several legitimate issues:
These should be addressed in a follow-up commit. Final RecommendationsApprove with follow-up required - This is high-quality work that significantly improves Visio fidelity, but the shape indexer race condition should be fixed promptly. Priority fixes:
Future enhancements:
The architectural improvements and comprehensive preservation logic make this a valuable contribution to OfficeIMO's capabilities. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ffca9d5e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
OfficeIMO.Visio/VisioPage.cs
Outdated
| if (childIndex < 0 || childIndex == currentIndex) { | ||
| return; |
There was a problem hiding this comment.
The ReparentShape contract says childIndex = -1 should append, but in the same-parent branch this path returns immediately, so calling ReparentShape(child, parent, -1) does nothing instead of moving the child to the end. This breaks the documented behavior and prevents callers from reordering children within a group using the advertised append mode.
Useful? React with 👍 / 👎.
OfficeIMO.Visio/VisioShape.cs
Outdated
| _owner.DetachChild(existing); | ||
| _owner.PrepareChildForParent(value); |
There was a problem hiding this comment.
The indexer detaches the existing child before validating the replacement, so if PrepareChildForParent throws (for example, replacement already has another parent or would create a cycle), the old child remains in _children but its Parent is now null. That leaves the hierarchy internally inconsistent and can later trigger validation/save failures after what should have been a rejected no-op update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e677ce6b5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set { | ||
| _page.PrepareConnectorForPage(value, _page._connectors[index]); | ||
| _page._connectors[index] = value; |
There was a problem hiding this comment.
The connector indexer setter writes value directly into the slot after PrepareConnectorForPage without checking whether value is already stored elsewhere (or is the same instance already at this index). If callers try to reorder connectors with index assignments (for example swapping two entries), this can duplicate object references in _connectors and also mutate automatic IDs during the assignment, leaving duplicate connector IDs and causing subsequent validation/save failures. The setter should mirror ShapeCollection behavior by short-circuiting same-reference updates and rejecting connectors already present at another index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8169e10bf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| VisioMaster firstMaster = masters[0].Master; | ||
| WritePreservedAttributes(writer, firstMaster.PreservedMastersRootAttributes); | ||
| WritePreservedElements(writer, firstMaster.PreservedMastersRootElements); |
There was a problem hiding this comment.
The <Masters> root-level preserved attributes/elements are taken only from masters[0].Master, so mixed documents can lose fidelity: if the first emitted master is a newly generated one (for example a dynamic-connector master from an earlier page) and a loaded master with preserved root metadata appears later, those preserved root fragments are never written back. Since load stores this metadata on loaded masters, writing from index 0 can silently strip masters.xml root extensions on save.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1756 +/- ##
==========================================
+ Coverage 69.68% 70.05% +0.37%
==========================================
Files 983 988 +5
Lines 115229 117875 +2646
Branches 23598 24294 +696
==========================================
+ Hits 80300 82583 +2283
- Misses 24839 24983 +144
- Partials 10090 10309 +219 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e79fbec249
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return !string.IsNullOrWhiteSpace(cellName) && | ||
| !string.Equals(cellName, "PageWidth", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(cellName, "PageHeight", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(cellName, "PageScale", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(cellName, "DrawingScale", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(cellName, "InhibitSnap", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The new ShouldPreservePageCell filter only skips five names, but VisioDocument.SaveCore always regenerates many additional PageSheet cells (for example DrawingSizeType, PageLockReplace, UIVisibility, DrawingResizeType, PageShapeSplit). In a load→save round-trip, those cells are preserved and then written again via WritePreservedElements, producing duplicate Cell entries with the same N value in the same PageSheet. This can create conflicting page metadata and unstable Visio behavior for otherwise unchanged documents.
Useful? React with 👍 / 👎.
| new XAttribute("N", "Value")); | ||
| clone.Add(valueCell); | ||
| } | ||
| valueCell.SetAttributeValue("V", value); |
There was a problem hiding this comment.
When a shape data row is preserved, WriteDataSection clones the original row and only updates the Value cell's V attribute. If that preserved Value cell carries a formula (F), the stale formula is kept, so Visio can recompute from F and ignore the new V value; user updates to shape.Data[...] then fail to persist after reopen. Updating a modeled value should also remove or rewrite formula attributes on the Value cell.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef677623b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
OfficeIMO.Visio/VisioPage.cs
Outdated
| if (childIndex > currentIndex) { | ||
| childIndex--; |
There was a problem hiding this comment.
When reparenting within the same parent, decrementing childIndex after RemoveAt shifts forward moves one slot too early. For example, with children [A,B,C], calling ReparentShape(B, parent, 2) should place B at index 2 ([A,C,B]), but this logic decrements to index 1 and leaves order effectively unchanged. Since the API documents childIndex as the insertion index in the target group, this breaks same-parent reordering for non-append forward moves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40dc5ee6b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((string.Equals(styleSheetId, "1", StringComparison.Ordinal) || string.Equals(styleSheetId, "2", StringComparison.Ordinal)) && | ||
| (string.Equals(localName, "BasedOn", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(localName, "LineStyle", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(localName, "FillStyle", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(localName, "TextStyle", StringComparison.OrdinalIgnoreCase))) { |
There was a problem hiding this comment.
The style-sheet preservation filter currently excludes BasedOn/LineStyle/FillStyle/TextStyle only for IDs 1 and 2, so ID 0 preserves those attributes. On save, CreateGeneratedStyleSheet("0", ...) already creates the same attributes and then appends preserved ones again, which produces duplicate attribute names on the same StyleSheet element and can throw during save (including load→save of OfficeIMO-generated files).
Useful? React with 👍 / 👎.
| if (entry.Shape != null && page.Shapes.Contains(entry.Shape)) { | ||
| WriteShapeElement(writer, ns, entry.Shape, persistedIds, pageMasters, masters); | ||
| emittedShapes.Add(entry.Shape); |
There was a problem hiding this comment.
When a page was loaded from disk, save emits each preserved shape entry as long as that shape is still present, without consulting its current index in page.Shapes. This means reordering existing shapes/connectors in memory after load (e.g., remove/insert to change z-order) is ignored because the old PreservedShapesChildren order wins, so authoring changes to stacking/order do not persist.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
The Visio layer still had a few authoring paths that could bypass page-managed state and produce invalid or inconsistent IDs, hierarchy links, or connector behavior. It also rebuilt several parts of Visio-authored XML too aggressively, which caused loss of document fidelity on round-trip even when OfficeIMO did not intend to change that metadata.
Impact
This makes OfficeIMO.Visio safer for programmatic authoring and much closer to the larger OfficeIMO goal of reliable editing without dropping existing document metadata. It improves compatibility with Visio-authored files, preserves more native behavior/layout metadata, and adds first-class APIs for grouped-shape and connector maintenance workflows.
Validation
dotnet test OfficeIMO.Tests/OfficeIMO.Tests.csproj -f net8.0 --filter "FullyQualifiedName~VisioLoadFidelity"dotnet test OfficeIMO.Tests/OfficeIMO.Tests.csproj -f net8.0 --filter "Visio"