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

[API Proposal]: Trim friendly Font ToLogFont/FromLogFont #8846

Closed
kant2002 opened this issue Jun 7, 2022 · 13 comments · Fixed by #8994
Closed

[API Proposal]: Trim friendly Font ToLogFont/FromLogFont #8846

kant2002 opened this issue Jun 7, 2022 · 13 comments · Fixed by #8994
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented area-System.Drawing System.Drawing issues

Comments

@kant2002
Copy link
Contributor

kant2002 commented Jun 7, 2022

Background and motivation

Right now WinForm use System.Drawing.Common.Font.ToLogFont/FromLogFont with their own interop structures. That API produce trimming warning. This API would serve same purpose as existing API, but provide trim friendly alternative.

API Proposal

namespace System.Drawing;

public sealed partial class Font
{
    public void ToLogFont<T>(ref T logFont, Graphics graphics) where T: struct, unmanaged  { throw null; }
    public static Font FromLogFont<T>(in T logFont) where T: struct, unmanaged { throw null; }
    public static Font FromLogFont<T>(in T logFont, IntPtr hdc) where T: struct, unmanaged { throw null; }
    public void ToLogFont<T>(ref T logFont) where T: struct, unmanaged { throw null; }
}

API Usage

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public unsafe struct MyLOGFONT
{
    public int lfHeight;
    public int lfWidth;
    public int lfEscapement;
    public int lfOrientation;
    public int lfWeight;
    public byte lfItalic;
    public byte lfUnderline;
    public byte lfStrikeOut;
    public byte lfCharSet;
    public byte lfOutPrecision;
    public byte lfClipPrecision;
    public byte lfQuality;
    public byte lfPitchAndFamily;
    private fixed char _lfFaceName[LF_FACESIZE];
}

// And similar 
var f = new Font();
MyLOGFONT interopStruct = default;
f.ToLogFont(ref interopStruct);

Alternative Designs

Ask users of this API, reimplement it itself within their application and make that code trim-friendly.

Risks

No response

Accompanying implementation dotnet/runtime#70224

@kant2002 kant2002 added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jun 7, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-System.Drawing System.Drawing issues label Jun 7, 2022
@ghost ghost added the untriaged The team needs to look at this issue in the next triage label Jun 7, 2022
@ghost
Copy link

ghost commented Jun 7, 2022

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

Issue Details

Background and motivation

Right now WinForm use System.Drawing.Common.Font.ToLogFont/FromLogFont with their own interop structures. That API produce trimming warning. This API would serve same purpose as existing API, but provide trim friendly alternative.

API Proposal

namespace System.Drawing;

public sealed partial class Font
{
    public void ToLogFont<T>(ref T logFont, Graphics graphics) where T: struct, unmanaged  { throw null; }
    public static Font FromLogFont<T>(in T logFont) where T: struct, unmanaged { throw null; }
    public static Font FromLogFont<T>(in T logFont, IntPtr hdc) where T: struct, unmanaged { throw null; }
    public void ToLogFont<T>(ref T logFont) where T: struct, unmanaged { throw null; }
}

API Usage

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public unsafe struct MyLOGFONT
{
    public int lfHeight;
    public int lfWidth;
    public int lfEscapement;
    public int lfOrientation;
    public int lfWeight;
    public byte lfItalic;
    public byte lfUnderline;
    public byte lfStrikeOut;
    public byte lfCharSet;
    public byte lfOutPrecision;
    public byte lfClipPrecision;
    public byte lfQuality;
    public byte lfPitchAndFamily;
    private fixed char _lfFaceName[LF_FACESIZE];
}

// And similar 
var f = new Font();
MyLOGFONT interopStruct = default;
f.ToLogFont(ref interopStruct);

Alternative Designs

Ask users of this API, reimplement it itself within their application and make that code trim-friendly.

Risks

No response

Author: kant2002
Assignees: -
Labels:

api-suggestion, area-System.Drawing

Milestone: -

@kant2002 kant2002 changed the title [API Proposal]: [API Proposal]: Trim friendly Font ToLogFont/FromLogFont Jun 7, 2022
@ViktorHofer
Copy link
Member

@JeremyKuhne @RussKie, given that there's already a PR up for the implementation, should this get reviewed in one of the api design review meetings? Would anyone of you want to be the champion for it?

@JeremyKuhne
Copy link
Member

@ViktorHofer sure, mark it and I'll represent.

@RussKie
Copy link
Member

RussKie commented Jun 7, 2022

I'll watch the recording

@ViktorHofer ViktorHofer added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed untriaged The team needs to look at this issue in the next triage labels Jun 8, 2022
@ViktorHofer
Copy link
Member

Great, thank you. Sent out a mail.

@terrajobst
Copy link
Member

How urgent is this?

@teo-tsirpanis teo-tsirpanis removed the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jun 11, 2022
@carlossanlop carlossanlop added this to the Future milestone Jun 29, 2022
@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 29, 2022

@kant2002 can you answer @terrajobst's question regarding the priority of getting this into main?

@kant2002
Copy link
Contributor Author

Personally I would like to have this in .Net 7, that increases chances that I annotate core WinForms libraries for trimming.

@ViktorHofer ViktorHofer removed this from the Future milestone Jul 13, 2022
@ViktorHofer ViktorHofer self-assigned this Jul 13, 2022
@bartonjs
Copy link
Member

bartonjs commented Aug 9, 2022

Video

  • We made Interop.LOGFONT public and removed the genericness of the functions
namespace System.Drawing
{
    public sealed partial class Font
    {
        public void ToLogFont(out Interop.LOGFONT logFont, Graphics graphics)  { throw null; }
        public static Font FromLogFont(in Interop.LOGFONT logFont) { throw null; }
        public static Font FromLogFont(in Interop.LOGFONT logFont, IntPtr hdc) { throw null; }
        public void ToLogFont(out Interop.LOGFONT logFont) { throw null; }
    }
}

namespace System.Drawing.Interop
{
    public unsafe struct LOGFONT
    {
        public int lfHeight;
        public int lfWidth;
        public int lfEscapement;
        public int lfOrientation;
        public int lfWeight;
        public byte lfItalic;
        public byte lfUnderline;
        public byte lfStrikeOut;
        public byte lfCharSet;
        public byte lfOutPrecision;
        public byte lfClipPrecision;
        public byte lfQuality;
        public byte lfPitchAndFamily;
        private fixed char _lfFaceName[LF_FACESIZE];

        [UnscopedRef]
        public Span<char> lfFaceName => MemoryMarshal.CreateSpan(ref _lfFaceName[0], LF_FACESIZE);
    }
}

@bartonjs bartonjs added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Aug 9, 2022
@elachlan
Copy link
Contributor

Now the API change is approved, can we reopen dotnet/runtime#70224?

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 23, 2023

@kant2002 feel free to re-open your PR and we can give it another glance.

kant2002 referenced this issue in kant2002/runtime Jan 24, 2023
These methods used by WinForms
Closes #70358
@JeremyKuhne JeremyKuhne transferred this issue from dotnet/runtime Mar 14, 2023
@elachlan
Copy link
Contributor

@kant2002 did you want to reroll your patch now System.Drawing has moved to winforms?

@JeremyKuhne JeremyKuhne added the 🚧 work in progress Work that is current in progress label Apr 14, 2023
@JeremyKuhne
Copy link
Member

I've implemented this and will be putting up a PR tomorrow.

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Apr 14, 2023
This adds the new typed API.

The new namespace conflicts with the Interop class so I had to change a number of files to add a static using for Interop.

Also changes WinForms to consume the new API.

Fixes dotnet#8846
JeremyKuhne added a commit that referenced this issue Apr 14, 2023
This adds the new typed API.

The new namespace conflicts with the Interop class so I had to change a number of files to add a static using for Interop.

Also changes WinForms to consume the new API.

Fixes #8846
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Apr 14, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented area-System.Drawing System.Drawing issues
Projects
None yet
9 participants