Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ICloneableFormat #168

Merged
merged 4 commits into from May 8, 2021
Merged

Conversation

Kaplas80
Copy link
Contributor

@Kaplas80 Kaplas80 commented Apr 10, 2021

Description

In this PR, BinaryFormat, NodeContainerFormat and Po classes implement a ICloneableFormat interface, making easier to get duplicates.

Also, there is a new constructor in Node that accepts other node. It makes a new node with the same name and tags than the original node and a deep copy of its format.

Example

// Make a node copy
Node clone = new Node(original);

// Deep copy a BinaryFormat (or other ICloneableFormat)
BinaryFormat newFormat = (BinaryFormat)oldFormat.DeepClone();

@Kaplas80 Kaplas80 changed the title Implement ICloneable Implement ICloneableFormat Apr 11, 2021
Copy link
Member

@pleonex pleonex left a comment

Choose a reason for hiding this comment

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

I really like the new design! Thanks!

Just a few questions about the behavior of the node cloning.

@@ -93,6 +94,15 @@ public BinaryFormat(Stream stream, long offset, long length)
private set;
}

/// <inheritdoc />
Copy link
Member

Choose a reason for hiding this comment

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

Can you add in the remarks of the documentation of this method that it creates the copy in memory (good to know for large streams).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

/// <param name="node">The original node.</param>
/// <remarks><para>It makes a copy of the original node.
/// The original format is deep copied. See <see cref="ICloneableFormat"/> for details.</para></remarks>
public Node(Node node)
Copy link
Member

Choose a reason for hiding this comment

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

Should we copy add or copy recursively the children?
If not, I think we should specify the behavior in the remarks of the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The children are already copied recursively because NodeContainerFormat implements DeepClone() and there is where the copies are made.
I'll add the comment in case the node has children but its format is not NodeContainerFormat.

var newFormat = new NodeContainerFormat();

// Just copy the first generation children.
// The rest will be copied recursively if necessary.
Copy link
Member

Choose a reason for hiding this comment

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

Right now the constructor of Node is not copying the nodes, is this comment still accurate? How would the node be copied recursively otherwise?

Assert.AreNotSame(format, clone);
Assert.AreNotSame(child1, child1Clone);
Assert.AreNotSame(child2, child2Clone);
Assert.AreNotSame(grandchild, grandchildClone);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is passing because grandchildClone is null? I think the copy is not recursively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, grandchildClone has contents:

image

@pleonex pleonex added this to the vNext milestone May 8, 2021
Copy link
Member

@pleonex pleonex left a comment

Choose a reason for hiding this comment

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

All good thanks!

@pleonex pleonex merged commit 287b4b9 into SceneGate:develop May 8, 2021
@Kaplas80 Kaplas80 deleted the feature/cloneable branch August 30, 2021 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants