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

Allow PathFigureCollection to string conversion via converter #19889

Merged
merged 1 commit into from
May 22, 2024

Conversation

symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Jan 15, 2024

This should be fairly self-explanatory. Allows path figures to be serialized as a string, using the SVG path syntax, not just deserialized.

I added a full round-trip test in the simple test and the ComplexPaths test.

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 15, 2024 01:22
@ghost ghost added the community ✨ Community Contribution label Jan 15, 2024
@ghost
Copy link

ghost commented Jan 15, 2024

Hey there @symbiogenesis! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@symbiogenesis symbiogenesis changed the title Allow PathCollectionConverter string conversion via ConvertTo Allow PathFigureCollectionConverter string conversion via ConvertTo Jan 15, 2024
@symbiogenesis symbiogenesis changed the title Allow PathFigureCollectionConverter string conversion via ConvertTo Allow PathFigureCollection to string conversion via ConvertTo Jan 15, 2024
@symbiogenesis symbiogenesis changed the title Allow PathFigureCollection to string conversion via ConvertTo Allow PathFigureCollection to string conversion via converter Jan 15, 2024
@symbiogenesis symbiogenesis force-pushed the pathfigurecollection-tostring branch 2 times, most recently from f1a05d9 to b572e88 Compare January 15, 2024 06:01
@jfversluis
Copy link
Member

jfversluis commented Jan 15, 2024

/azp run

This comment was marked as off-topic.

This comment was marked as off-topic.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow
Copy link
Member

When I tested with a full round-trip test in the ComplexPaths test, the round-trip strings aren't completely identical, but the visual results are exactly the same on each object. Which is the important part.
Possibly the output is following the SVG path syntax more strictly than some of the InlineData for the test.

Do you have an example?

@mattleibow mattleibow added the s/pr-needs-author-input PR needs an update from the author label Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

Hi @symbiogenesis. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 19, 2024

When I tested with a full round-trip test in the ComplexPaths test, the round-trip strings aren't completely identical, but the visual results are exactly the same on each object. Which is the important part.
Possibly the output is following the SVG path syntax more strictly than some of the InlineData for the test.

Do you have an example?

I just updated the tests so that the complex tests can pass bi-directionally. It required making the InlineData conform to stricter standards.

But those standards are just needed to create a fully identical string for bi-directional testing purposes. The standard itself is more forgiving, but the ToString operation needs to pick one of the multiple valid ways to serialize the path.

@Eilon Eilon added the area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing label Jan 20, 2024
@ghost
Copy link

ghost commented Jan 30, 2024

Hi @symbiogenesis.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@ghost ghost added the stale Indicates a stale issue/pr and will be closed soon label Jan 30, 2024
@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 30, 2024

This has unit tests and the pipeline seems good. Would be nice to get it merged.

@ghost ghost removed stale Indicates a stale issue/pr and will be closed soon s/pr-needs-author-input PR needs an update from the author labels Jan 30, 2024
@MartyIX
Copy link
Contributor

MartyIX commented Apr 8, 2024

Is this PR possibly related to #20552 in any way?

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Apr 9, 2024

Is this PR possibly related to #20552 in any way?

Seems likely. I mainly think native MAUI vector usage via Shapes should get more love.

We should be able to directly translate SVG to real MAUI shapes, and back again, ideally.

I have been able to write code to translate SVG into MAUI graphics to some degree. It feels kind of within reach.

@rmarinho
Copy link
Member

rmarinho commented Apr 9, 2024

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jsuarezruiz jsuarezruiz self-requested a review April 23, 2024 15:13
@symbiogenesis
Copy link
Contributor Author

@jsuarezruiz

@rmarinho rmarinho merged commit 3e02949 into dotnet:main May 22, 2024
47 checks passed
@symbiogenesis symbiogenesis deleted the pathfigurecollection-tostring branch May 22, 2024 14:17
@github-actions github-actions bot locked and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing community ✨ Community Contribution fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants