Added DrawRectangle overload accepting RectangleF by medo64 · Pull Request #62385 · dotnet/runtime

medo64 · 2021-12-04T05:37:04Z

FillRectangle contains the following 4 overloads:

public void FillRectangle(Brush brush, RectangleF rect)
public void FillRectangle(Brush brush, float x, float y, float width, float height)
public void FillRectangle(Brush brush, Rectangle rect)
public void FillRectangle(Brush brush, int x, int y, int width, int height)

However, DrawRectangle, contains only the following 3 overloads:

public void DrawRectangle(Pen pen, Rectangle rect)
public void DrawRectangle(Pen pen, float x, float y, float width, float height)
public void DrawRectangle(Pen pen, int x, int y, int width, int height)

In the interest of symmetry, I propose adding the "missing" RectangleF overload:

public void DrawRectangle(Pen pen, RectangleF rect)

Since overload taking individual elements already exists, this new overload will just "decompose" rectangle structure into individual elements and call the existing function. Approach is exactly the one used in FillRectangle code.

ghost · 2021-12-04T05:37:11Z

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

FillRectangle contains the following 4 overloads:

public void FillRectangle(Brush brush, RectangleF rect)
public void FillRectangle(Brush brush, float x, float y, float width, float height)
public void FillRectangle(Brush brush, Rectangle rect)
public void FillRectangle(Brush brush, int x, int y, int width, int height)

However, DrawRectangle, contains only the following 3 overloads:

public void DrawRectangle(Pen pen, Rectangle rect)
public void DrawRectangle(Pen pen, float x, float y, float width, float height)
public void DrawRectangle(Pen pen, int x, int y, int width, int height)

In the interest of symmetry, I propose adding the "missing" RectangleF overload:

public void DrawRectangle(Pen pen, RectangleF rect)

Since overload taking individual elements already exists, this new overload will just "decompose" rectangle structure into individual elements and call the existing function. Approach is exactly the one used in FillRectangle code.

Author:	medo64
Assignees:	-
Labels:	`area-System.Drawing`
Milestone:	-

Wraith2 · 2021-12-04T11:19:43Z

If there is an approved api issue for this can you link to it please? If not you will need to go through the Api Approval Process before it is added.

medo64 · 2021-12-04T17:49:41Z

If there is an approved api issue for this can you link to it please? If not you will need to go through the Api Approval Process before it is added.

I have added the API proposal. Will wait for result of the same. :)

teo-tsirpanis · 2021-12-05T23:43:41Z

Since you are adding a new API you also have to add it in the reference assembly's sources.

runtime/src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs

Lines 508 to 512 in fedf64f

publicvoidDrawRectangle(System.Drawing.Penpen,System.Drawing.Rectanglerect){}
publicvoidDrawRectangle(System.Drawing.Penpen,intx,inty,intwidth,intheight){}
publicvoidDrawRectangle(System.Drawing.Penpen,floatx,floaty,floatwidth,floatheight){}
publicvoidDrawRectangles(System.Drawing.Penpen,System.Drawing.RectangleF[]rects){}
publicvoidDrawRectangles(System.Drawing.Penpen,System.Drawing.Rectangle[]rects){}

dnfadmin · 2021-12-06T08:05:14Z

All CLA requirements met.

medo64 · 2021-12-06T08:07:06Z

Since you are adding a new API you also have to add it in the reference assembly's sources.

Added, thank you.

safern · 2021-12-06T20:04:17Z

@medo64 thanks for your contribution, given that the API Proposal issue has to be approved first, let's wait for it get reviewed by the review board and if approved we can reopen this PR. I will close the PR to follow the API Proposal guidelines on the repository, and we can reopen when ready.

medo64 · 2021-12-15T04:42:40Z

API proposal has been approved. Please reopen this pull request.

safern · 2021-12-15T19:41:39Z

@medo64 could you mind adding a test for each added API

teo-tsirpanis · 2021-12-15T19:43:41Z

@medo64 I made a mistake previously about the reference assembly's sources, you need to add the new API definitions to src/libraries/System.Drawing.Common/ref/System.Drawing.Common.netcoreapp.cs instead of System.Drawing.Common.cs (these new APIs will be available only for .NET Core). Please move them from the latter to the former.

safern · 2021-12-15T19:45:29Z

@teo-tsirpanis the other overloads are available for all target frameworks, so I think it is fine to keep them in all targets.

teo-tsirpanis · 2021-12-15T19:55:06Z

Well, I got confused, the .NET Framework 4.6.1 assembly on NuGet has only type forwards on the framework library, not actual types.

medo64 · 2021-12-16T00:52:12Z

@medo64 could you mind adding a test for each added API

For DrawRectangle I added an entry with RectangleF in every existing test I could find. Since other overloads are already actually tested here, I believe this is appropriate.

For FillPie there were no current test so I added basic argument tests modeled after tests present for DrawPie.

I do have two extra questions in regards to tests though:

I see that Fill* methods are missing even basic argument tests. What is the process for adding the same? Should I open a [bug] and wait for approval or should I just add them to this PR?
Test case named DrawPie_ZeroHeight_ThrowsArgumentException actually tests DrawArc method (looks to be a copy/paste error). Should I just make this minor correction here or should I open a new [bug] and wait for approval?

medo64 · 2022-01-10T01:57:55Z

Anything else you fill needs to be added or this is it?

safern · 2022-01-10T19:13:39Z

I think @teo-tsirpanis was right and we need to move the APIs to the ref/netcoreapp.cs, that should fix the build. Also the tests should only be included for .NET Core targets, you can do that by putting them on their own .netcoreapp.cs test file and compile it only when we are targeting .NET Core.

medo64 · 2022-01-11T03:36:02Z

you can do that by putting them on their own .netcoreapp.cs test file

I see that there are some tests already in GraphicsTests.Core.cs and that one seems to be targeting NETCoreApp. Should I add tests in this one or you want me to create a new file instead?

medo64 · 2022-01-11T03:37:19Z

need to move the APIs to the ref/netcoreapp.cs

Do you mean System.Drawing.Common.netcoreapp.cs in my case? I cannot find just plain netcoreapp.cs file...

safern · 2022-01-11T04:02:32Z

I see that there are some tests already in GraphicsTests.Core.cs and that one seems to be targeting NETCoreApp. Should I add tests in this one or you want me to create a new file instead?

Adding them to that file seems better.

Do you mean System.Drawing.Common.netcoreapp.cs in my case?

Yeah, I missed the * to state that.

* Only testing RectangleF overloads in GraphicsTests.Core.cs; other overloads stay as they are. * FillPie tests that were newly added as part of issue #62385 are split in the same manner.

DrawRectangle with RectangleF overload is new and not compiled into mono.

safern · 2022-01-11T18:41:31Z

Thank you @medo64!

Added DrawRectangle overload accepting RectangleF
a8dedae

ghost added area-System.Drawing community-contribution Indicates that the PR has been added by a community member labels Dec 4, 2021

Added DrawRectangle overload to reference assembly file
dc886b4

safern closed this Dec 6, 2021

Added FillPie overload accepting RectangleF
da90c4d

medo64 mentioned this pull request Dec 15, 2021
[API Proposal]: DrawRectangle should accept RectangleF as one of the overloads #62401
Closed

safern reopened this Dec 15, 2021

medo64 added 2 commits December 15, 2021 16:27

Added test cases for DrawRectangle accepting RectangleF
797a39d

Added FillPie argument test cases
a4b7b38

runfoappBot mentioned this pull request Dec 16, 2021
Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979
Closed

stephentoub reviewed Jan 3, 2022
View reviewed changes

Comment threadsrc/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs

stephentoub reviewed Jan 3, 2022
View reviewed changes

Comment threadsrc/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs

Added XML parameters for newly added FillPie and DrawRectangle
ce47ab5

runfoappBot mentioned this pull request Jan 4, 2022
System.Net.Sockets.Tests.SendPacketsAsync.SendPacketsElement_FileZeroCount_Success sometimes fails #60017
Closed

medo64 added 3 commits January 10, 2022 23:02

Moving DrawRectangle and FillPie with RectangleF to netcoreapp
c2f5513

Moving DrawRectangle and FillPie RectangleF tests to netcoreapp
f6b9b4d
* Only testing RectangleF overloads in GraphicsTests.Core.cs; other overloads stay as they are. * FillPie tests that were newly added as part of issue #62385 are split in the same manner.

Removing RectangleF overload from mono tests
6352e26
DrawRectangle with RectangleF overload is new and not compiled into mono.

safern approved these changes Jan 11, 2022
View reviewed changes

safern merged commit 64f4885 into dotnet:mainJan 11, 2022

ghost locked as resolved and limited conversation to collaborators Feb 18, 2022

Uh oh!

Conversation

medo64 commented Dec 4, 2021

Uh oh!

ghost commented Dec 4, 2021

Uh oh!

Wraith2 commented Dec 4, 2021

Uh oh!

medo64 commented Dec 4, 2021

Uh oh!

teo-tsirpanis commented Dec 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfadmin commented Dec 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medo64 commented Dec 6, 2021

Uh oh!

safern commented Dec 6, 2021

Uh oh!

medo64 commented Dec 15, 2021

Uh oh!

safern commented Dec 15, 2021

Uh oh!

teo-tsirpanis commented Dec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

safern commented Dec 15, 2021

Uh oh!

teo-tsirpanis commented Dec 15, 2021

Uh oh!

medo64 commented Dec 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

medo64 commented Jan 10, 2022

Uh oh!

safern commented Jan 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medo64 commented Jan 11, 2022

Uh oh!

medo64 commented Jan 11, 2022

Uh oh!

safern commented Jan 11, 2022

Uh oh!

safern commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

teo-tsirpanis commented Dec 5, 2021•
edited
Loading

dnfadmin commented Dec 6, 2021•
edited
Loading

teo-tsirpanis commented Dec 15, 2021•
edited
Loading

medo64 commented Dec 16, 2021•
edited
Loading

safern commented Jan 10, 2022•
edited
Loading