Skip to content

Prevent curve overwrite during GridLine deserialization#1101

Merged
katehryhorenko merged 4 commits intomasterfrom
GridLine-deserialization
Jun 3, 2025
Merged

Prevent curve overwrite during GridLine deserialization#1101
katehryhorenko merged 4 commits intomasterfrom
GridLine-deserialization

Conversation

@katehryhorenko
Copy link
Copy Markdown
Contributor

@katehryhorenko katehryhorenko commented Jun 2, 2025

🌌 BACKGROUND:

GridLine has three properties that can set its geometry: Curve, Geometry, and Line.
In some workflows (e.g., created by a function and later edited in Pringle), a GridLine can enter a corrupt state where:

  • Curve contains the correct value,
  • but it is overwritten during deserialization by Line or Geometry values,
  • leading to incorrect behavior or rendering.

📋 DESCRIPTION:

This PR prevents unwanted overwriting of the Curve during deserialization:

  • Introduces a deserializationIsInProgress flag:

    • Set to true inside the JsonConstructor.
    • Reset to false in the OnDeserialized callback.
  • Modifies the Line and Geometry setters to skip assigning to Curve during deserialization, preserving the original Curve if it's already set.

🧪 TESTING:

  • Enabled the suggest-everywhere flag.
  • Added a GridLine via suggestion.
  • Modified an existing GridLine.
  • Copied the GridLine to clipboard.
  • Saved and reloaded the file using deserialization.

This change is Reviewable

@katehryhorenko katehryhorenko requested a review from wynged June 2, 2025 20:07
Copy link
Copy Markdown
Member

@wynged wynged left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained


Elements/src/GridLine.cs line 33 at r1 (raw file):

        // TODO: Remove this constructor once we remove the Line and Geometry properties.
        [JsonConstructor]
        private GridLine(BoundedCurve curve, Polyline geometry, Line line)

I'm surprised we don't need to call the geometric element constructor from this line as welll, I think most of our other custom constructors call their inherited constructor. Will the other properties get set correctly still?

Copy link
Copy Markdown
Contributor Author

@katehryhorenko katehryhorenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)


Elements/src/GridLine.cs line 33 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

I'm surprised we don't need to call the geometric element constructor from this line as welll, I think most of our other custom constructors call their inherited constructor. Will the other properties get set correctly still?

It's a good point. I pushed the change

Copy link
Copy Markdown
Member

@wynged wynged left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@katehryhorenko katehryhorenko merged commit 8a95ae9 into master Jun 3, 2025
2 checks passed
@katehryhorenko katehryhorenko deleted the GridLine-deserialization branch June 3, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants