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 CachedBitmap #39245

Closed
wants to merge 1 commit into from
Closed

Implement CachedBitmap #39245

wants to merge 1 commit into from

Conversation

reflectronic
Copy link
Contributor

Closes dotnet/winforms#8822

This functionality did not exist in libgdiplus prior to my implementation of it, which can be reviewed at mono/libgdiplus#654. It is not merged yet, but it will be available in libgdiplus 6.1.

Since this functionality has not yet been published in a released version of libgdiplus, and since it is likely that people using CachedBitmap could be running an older version, there is a version check that lets us fail gracefully in case CachedBitmap is not present.

Because 6.1 isn't released, testing these changes for non-Windows platforms will be tough. Right now, the CachedBitmap tests would just be skipped in CI until the released version gets upstreamed into all of the right places. I can at least promise you that they work when running on my local machine 😉. The tests are mostly the same as the ones present in my PR to libgdiplus, and they are passing on Windows as well.

This PR depends on dotnet/runtime-assets#80

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jul 14, 2020

Tagging subscribers to this area: @safern, @tannergooding
Notify danmosemsft if you want to be subscribed.

gfoidl
gfoidl reviewed Jul 14, 2020
View reviewed changes
src/libraries/System.Drawing.Common/src/Resources/Strings.resx Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.Unix.cs Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.cs Show resolved Hide resolved
@tannergooding
Copy link
Member

This, in general, looks good to me (minus the test failures).
CC. @safern

@danmoseley
Copy link
Member

We should presumably wait to merge this until the Mono change is merged and we can be certain it will be in 6.1.

@safern safern added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 14, 2020
@safern
Copy link
Member

safern commented Jul 14, 2020

We should presumably wait to merge this until the Mono change is merged and we can be certain it will be in 6.1.

Agreed. Added NO MERGE in the meantime.

@danmoseley
Copy link
Member

Not a problem, it happens from time to time!

@safern
Copy link
Member

safern commented Jul 15, 2020

FYI, the runtime assets version of the package has been updated today in the morning.

@reflectronic
Copy link
Contributor Author

reflectronic commented Jul 16, 2020 •
edited
Loading

Well, that wasn't supposed to happen. Looks like I now have 0 commits in this PR...

@gfoidl
Copy link
Member

gfoidl commented Jul 16, 2020

@reflectronic why is this PR closed now? I thinks it's a valuable addition.

@reflectronic
Copy link
Contributor Author

Because next time, I should ask a professional before I do anything with Git 😅 I'm working on fixing what happened

@reflectronic @safern
Add initial implementation
e1e3929 
Update reference assembly

Add tests

Add doc comments

Fix tests

Initial support for CachedBitmap on Unix

Enable tests on Unix

Improve version detection mechanism

Fix build

Fix version detection

Improve test suite

Respond to API review feedback

Update reference assembly

Fix formatting

Add license headers

Fix test failure on .NET Framework

Update src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.cs

Co-authored-by: Günther Foidl <[email protected]>

Update src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.Unix.cs

Co-authored-by: Günther Foidl <[email protected]>

Make exception messages clearer

Improve error message again

Address PR feedback

Update src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/CachedBitmap.Windows.cs

Co-authored-by: Santiago Fernandez Madero <[email protected]>
@reflectronic
Copy link
Contributor Author

reflectronic commented Jul 16, 2020 •
edited
Loading

Look like GitHub behaves very strangely with what I have done to my branch here. I'll just open a new PR, because it looks like GitHub can't really figure out what happened. Actually, looks like it decided to work fine now. Quite strange.

Thankfully, this does at least consume the new changes to the runtime-assets package. However, it still isn't picking up the new CachedBitmap test assets. Even if I do a local build of master, the files don't show up under artifacts\bin\System.Drawing.Common.Tests\net5.0-Debug\bitmaps.

It looks like this is won't to make it into .NET 5, which is slightly unfortunate. I didn't really have control over how quickly the libgdiplus PR would progress, though, so that was probably to be expected. However, this does give me the opportunity to batch up some related changes in .NET 6, which should be pretty neat.

@reflectronic reflectronic reopened this Jul 16, 2020
@danmoseley danmoseley closed this Oct 11, 2020
@danmoseley danmoseley reopened this Oct 11, 2020
@danmoseley
Copy link
Member

@reflectronic is this ready to go now?

@reflectronic
Copy link
Contributor Author

The libgdiplus change still needs to be merged, so this isn't really good to go yet. I can't really see anything needing to change in this PR specifically, though, unless someone comes up with some feedback.

@danmoseley
Copy link
Member

@reflectronic ah of course. I poked that PR. I'm going to close this PR until the libgdiplus change is merged and deployed. We are trying to reduce the huge number of active PRs in this repo so that it becomes easier to reason about them.

You can easily reopen the PR when it's ready - it just gets it off the list we're looking at for now. Sound good?

@danmoseley danmoseley closed this Oct 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@tannergooding tannergooding tannergooding left review comments

@gfoidl gfoidl gfoidl left review comments

Assignees
No one assigned
Labels
area-System.Drawing new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement CachedBitmap functionality in System.Drawing
6 participants
@reflectronic @Dotnet-GitSync-Bot @tannergooding @danmoseley @safern @gfoidl