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 new GetContextInfo API overloads #49186

Merged
merged 12 commits into from
Apr 16, 2021
Merged

Implement new GetContextInfo API overloads #49186

merged 12 commits into from
Apr 16, 2021

Conversation

JeremyKuhne
Copy link
Member

Implements #47880, adding new, more performant overloads for GetContextInfo.

  • Add helper for only creating a region if it isn't infinite
  • Start an internal extensions class for easier mapping of System.Drawing concepts to System.Numerics types
  • Simplify GraphicsContext

@dotnet-issue-labeler
Copy link

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 Mar 5, 2021

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements #47880, adding new, more performant overloads for GetContextInfo.

  • Add helper for only creating a region if it isn't infinite
  • Start an internal extensions class for easier mapping of System.Drawing concepts to System.Numerics types
  • Simplify GraphicsContext
Author: JeremyKuhne
Assignees: -
Labels:

area-System.Drawing, new-api-needs-documentation
Milestone: -

@JeremyKuhne
Copy link
Member Author

cc: @dreddy-work @RussKie

@JeremyKuhne JeremyKuhne requested a review from safern March 5, 2021 19:10
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Mar 9, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Mar 9, 2021
RussKie
RussKie reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

safern
safern approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Other than a question and @ericstj suggestion to use the new ObsoleteAttribute with diagnostic ID, looks good to me.

@safern
Copy link
Member

safern commented Mar 25, 2021

@JeremyKuhne have you had a chance to resolve conflicts and @ericstj's concern?

@JeremyKuhne
Copy link
Member Author

have you had a chance to resolve conflicts and @ericstj's concern?

@safern Sorry, super busy. Hopefully can get this tidied up in another week or two.

@safern
Copy link
Member

safern commented Mar 26, 2021 •
edited
Loading

Sounds good. Just want to avoid it going stale 😊

@JeremyKuhne
Add basic GetContextInfo regression tests.
db05a29
@JeremyKuhne
Implement new GetContextInfo API overloads
8f221aa 
Implements dotnet#47880, adding new, more performant overloads for GetContextInfo.

- Add helper for only creating a region if it isn't infinite
- Start an internal extensions class for easier mapping of System.Drawing concepts to System.Numerics types
- Simplify GraphicsContext
@JeremyKuhne
Update WindowsGraphics to use new overloads.
cc4c154
@JeremyKuhne
Skip tests when GDI+ isn't available.
ee03c70
@JeremyKuhne
Add [SupportedOSPlatform("windows")]
ade74ec
@JeremyKuhne
Copy link
Member Author

Resolved conflicts

@RussKie
Copy link
Member

RussKie commented Apr 15, 2021

Can this make into P4?

@safern
Copy link
Member

safern commented Apr 15, 2021

Thanks @JeremyKuhne it seems like you are still missing to use the "new" obsolete attribute that takes a diagnostic id.

@JeremyKuhne
Copy link
Member Author

Thanks @JeremyKuhne it seems like you are still missing to use the "new" obsolete attribute that takes a diagnostic id.

I'm working on it.

@danmoseley
Copy link
Member

Can this make into P4?

I believe it needs to be merged by 4pm PST Fri

@jeffhandley
Copy link
Member

@JeremyKuhne -- when you assign the diagnostic ID, please also add this new obsoletion to the table in the list of obsoletions document, and ensure the message used on the obsoletion has enough context to be meaningful in that document.

@JeremyKuhne
Copy link
Member Author

Added the new obsolete pattern.

jeffhandley
jeffhandley reviewed Apr 15, 2021
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Here are just a few extra suggestions on the obsoletions pattern. Sorry for the ambiguity in my previous comments.

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/ref/System.Drawing.Common.csproj Outdated Show resolved Hide resolved
src/libraries/Common/src/System/Obsoletions.cs Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs Outdated Show resolved Hide resolved
@JeremyKuhne
Copy link
Member Author

Test failures are #51346

@safern
Copy link
Member

safern commented Apr 16, 2021

Merging since I just merged conflicts on the .md file and the previous build only had the known test failures from:#51346

@safern safern merged commit 33033b2 into dotnet:main Apr 16, 2021
@JeremyKuhne JeremyKuhne deleted the fastcontext branch April 16, 2021 18:16
@RussKie RussKie mentioned this pull request Apr 19, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators May 16, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@safern
Copy link
Member

safern commented Sep 30, 2021

Will work on opening the breaking change issue.

@safern
Copy link
Member

safern commented Nov 2, 2021

Breaking change issue created: dotnet/docs#26785

@safern safern removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericstj ericstj ericstj left review comments

@RussKie RussKie RussKie left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@safern safern safern approved these changes

Assignees

@safern safern

Labels
area-System.Drawing breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@JeremyKuhne @safern @RussKie @danmoseley @jeffhandley @ericstj @karelz