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.

Sign up for GitHub

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

Jump to bottom

Implement SkiaImage Save #9125

Merged
merged 6 commits into from
Sep 26, 2022
Merged

Implement SkiaImage Save #9125

merged 6 commits into from
Sep 26, 2022

Conversation

lindexi
Copy link
Contributor

@lindexi lindexi commented Aug 2, 2022

Description of Change

I implement the Save and SaveAsync method in SkiaImage .

The origin PR: dotnet/Microsoft.Maui.Graphics#475

@lindexi lindexi mentioned this pull request Aug 2, 2022
Closed
@rmarinho
Copy link
Member

rmarinho commented Aug 2, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

mattleibow
mattleibow reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for moving the pr here. Just a comment on the image format and also maybe dispose the objects sooner. Like the data and stream objects to reduct the amount of memory in use and GC work.

src/Graphics/src/Graphics.Skia/SkiaImage.cs Outdated Show resolved Hide resolved
@Eilon Eilon added the area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing label Aug 2, 2022
@jfversluis jfversluis added the s/pr-needs-author-input PR needs an update from the author label Aug 4, 2022
@ghost
Copy link

ghost commented Aug 18, 2022

Hi @lindexi.
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 Aug 18, 2022
@ghost ghost closed this Aug 23, 2022
@lindexi
Copy link
Contributor Author

lindexi commented Aug 23, 2022

@mattleibow and @jfversluis Could you re-open and review my PR ?

@mattleibow mattleibow reopened this Aug 23, 2022
mattleibow
mattleibow approved these changes Aug 23, 2022
View reviewed changes
src/Graphics/src/Graphics.Skia/SkiaImage.cs Outdated Show resolved Hide resolved
@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 Aug 23, 2022
@mattleibow
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mattleibow
Copy link
Member

@lindexi is it possible to update from main? Something fishy is going on in the build logs saying that types are missing that you did not touch...

@lindexi
Copy link
Contributor Author

lindexi commented Aug 30, 2022

@mattleibow Thank you and I update the branch

@rmarinho
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@rmarinho rmarinho merged commit 024b0f9 into dotnet:main Sep 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
@samhouts samhouts added the fixed-in-7.0.0-rc.2.6866 Look for this fix in 7.0.0-rc.2.6866! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartyIX MartyIX MartyIX left review comments

@mattleibow mattleibow mattleibow approved these changes

@jfversluis jfversluis Awaiting requested review from jfversluis

Assignees
No one assigned
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing fixed-in-7.0.0-rc.2.6866 Look for this fix in 7.0.0-rc.2.6866!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lindexi @rmarinho @mattleibow @MartyIX @jfversluis @Eilon @samhouts