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 CachedBitmap functionality #654

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

reflectronic
Copy link

@reflectronic reflectronic commented Jun 26, 2020

This PR implements the CachedBitmap functionality from GDI+. This is in preparation for implementing dotnet/winforms#8822, which adds CachedBitmap to System.Drawing.

I would like to get CachedBitmap implemented for .NET 5. There are only 19 or so days until the window closes. @filipnavara, hopefully I can get this quickly reviewed, ironed out, and merged, so that I can finalize the .NET implementation.

src/cachedbitmap.c Show resolved Hide resolved
tests/testcachedbitmap.c Outdated Show resolved Hide resolved
@reflectronic
Copy link
Author

Ah, I forgot about license headers. I'm not really sure what to do with them, since the ones in other files are ancient. Should I use the .NET Foundation license headers found in other projects?

@reflectronic
Copy link
Author

reflectronic commented Jul 1, 2020

@akoeplinger if you could take a look and merge this soon, that would be great. I hope to get this in soon, so that .NET 5 can depend on it. Is there a time frame for when 6.1 will be releasing?

Sorry for rushing here, I just hope to get this in ASAP before dotnet/runtime closes off new features.

@qmfrederik
Copy link
Contributor

@reflectronic Really great to see new features being implemented in libgdiplus!

I went through the process of getting .NET Core code to depend on a new version of libgdiplus.

  • On Linux, the current policy of the .NET Core team is to test against the version of libgdiplus which ships with the various Linux distributions (as opposed to the latest version of libgdiplus from the Mono repositories). That means you'll need to get a release in libgdiplus, get someone to upstream those releases to the distros, and wait for a new distro release.
  • On macOS, that same policy implies .NET Core relies on whatever version ships with Homebrew. All it takes for Homebrew to ship a new version of libgdiplus is a tag in this repo. You can then open a PR against Homebrew and have it merged fairly quickly. However, as a next step, you'll need someone to run brew update libgdiplus the .NET Core build machines. Last time, that process took 5 months: System.Drawing.Common: Require at least libgdiplus 6.0.1 on macOS dotnet/corefx#41707 System.Drawing.Common: Assert libgdiplus 6.0.1 on macOS unit tests dotnet/runtime#335

Let's hope the process has been ironed out a bit by now :).

On the other hand, System.Drawing.Common ships as a NuGet package, so it's a bit easier to consume newer versions of System.Drawing.Common.

@reflectronic
Copy link
Author

Yeah, surely the process of getting a new version shipped is going to take a while. Since this feature introduces new API surface (possibly for the first time in a long while), I implemented version detection so that the managed API can fail gracefully on an older version.

As for running tests in dotnet/runtime, currently my plan was to just skip the tests for now; when 6.1 eventually makes its way to the build machines, the tests would just light up. When I submit my PR to the runtime repository, then I could probably get some guidance from the area owners on what to do. Hopefully something can be figured out.

@reflectronic
Copy link
Author

For the record, I don't think it's critical to get this shipped everywhere within the next 5 months. For example, one of the biggest scenarios for System.Drawing is Windows Forms; for those who weren't going to rely on libgdiplus anyway, adding CachedBitmap to .NET 5 is going to work flawlessly. Whenever 6.1 does come out, then CachedBitmap can light up on non-Windows platforms from the managed side. However, at the least, we do need to merge this first, so that I can ensure that whatever does ship in the future will be compatible with the way it's implemented in .NET 5.

@danmoseley
Copy link

@akoeplinger, @reflectronic has a matching PR for this open against dotnet/runtime. Do you expect to merge this PR? I guess even when you do, any code in System.Drawing would have to be "light up" for some time.

Base automatically changed from master to main March 12, 2021 13:26
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.

5 participants