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

Introduce AOT friendly version of ToLogFont and FromLogFont #81082

Closed
wants to merge 3 commits into from

Conversation

kant2002
Copy link
Contributor

These methods used by WinForms
Closes dotnet/winforms#8846

These methods used by WinForms
Closes #70358
@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 ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2023
@ghost
Copy link

ghost commented Jan 24, 2023

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

Issue Details

These methods used by WinForms
Closes dotnet/winforms#8846

Author: kant2002
Assignees: -
Labels:

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

Milestone: -

@ViktorHofer
Copy link
Member

@kant2002 can you please add tests for the new APIs?

@stephentoub
Copy link
Member

cc: @JeremyKuhne

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Comments should be updated and there are still some test issues. Other than that the implementation looks good.

@@ -291,6 +291,29 @@ public unsafe void ToLogFont(object logFont, Graphics graphics)
}
}

/// <summary>
/// Update the given LOGFONT with data from given graphics.
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to use the wording in the existing docs. This applies to all of the overloads you're adding.

@@ -888,6 +1039,25 @@ public class LOGFONT
public string lfFaceName;
}

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public struct LOGFONT_Struct
Copy link
Member

Choose a reason for hiding this comment

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

Probably unnecessary. You're inadvertently using this (incorrect) struct in the positive tests. Just using an existing struct (say, Point) should be sufficient for the negative tests.

using (FontFamily family = FontFamily.GenericMonospace)
{
IntPtr lpFaceName = Marshal.StringToCoTaskMemAuto(family.Name);
var logFont = new LOGFONT_Struct
Copy link
Member

Choose a reason for hiding this comment

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

LOGFONT_Struct isn't a valid definition of LOGFONT.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 6, 2023
@ViktorHofer
Copy link
Member

@kant2002 we just recently migrated System.Drawing.Common to dotnet/winforms and need to port this PR over to winforms. cc @JeremyKuhne

@kant2002 kant2002 closed this Mar 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Drawing community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Trim friendly Font ToLogFont/FromLogFont
4 participants