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: Add span overloads to TextRenderer #3651

Closed
JeremyKuhne opened this issue Jul 27, 2020 · 4 comments · Fixed by #3711
Closed

API Proposal: Add span overloads to TextRenderer #3651

JeremyKuhne opened this issue Jul 27, 2020 · 4 comments · Fixed by #3711
Labels
api-approved (4) API was approved in API review, it can be implemented tenet-performance Improve performance, flag performance regressions across core releases
Milestone

Comments

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Jul 27, 2020

Proposal

We should add span overloads to TextRenderer to allow for efficient text rendering. Fortunately the Win32 APIs in play here take a length so we can use span data directly- we've already refactored internally to use ReadOnlySpan<char>.

namespace System.Windows.Forms
{
    public sealed class TextRenderer
    {
        // Existing:
        public static void DrawText(IDeviceContext dc, string text, Font font, Point pt, Color foreColor);
        public static void DrawText(IDeviceContext dc, string text, Font font, Point pt, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, string text, Font font, Point pt, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, string text, Font font, Point pt, Color foreColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, string text, Font font, Rectangle bounds, Color foreColor);
        public static void DrawText(IDeviceContext dc, string text, Font font, Rectangle bounds, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, string text, Font font, Rectangle bounds, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, string text, Font font, Rectangle bounds, Color foreColor, TextFormatFlags flags);
        public static Size MeasureText(IDeviceContext dc, string text, Font font);
        public static Size MeasureText(IDeviceContext dc, string text, Font font, Size proposedSize);
        public static Size MeasureText(IDeviceContext dc, string text, Font font, Size proposedSize, TextFormatFlags flags);
        public static Size MeasureText(string text, Font font);
        public static Size MeasureText(string text, Font font, Size proposedSize);
        public static Size MeasureText(string text, Font font, Size proposedSize, TextFormatFlags flags);

        // 1-1 Proposal
        // Note that backColor and proposedSize can't be defaulted as they are Color.Empty and Size(int.MaxValue, intMaxValue).
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, TextFormatFlags flags);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Size proposedSize);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Size proposedSize, TextFormatFlags flags);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font, Size proposedSize);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font, Size proposedSize, TextFormatFlags flags);
    }
}

Justification

Drawing and measuring text is a pretty common activity. This will allow significantly more efficient text rendering when new string allocations can be avoided (slicing other input, building off of a stack based character array, etc.). In conjunction with IDeviceContext on PaintEventArgs and other internal performance improvements, painting a string on a blank form can be done with < 15% of the allocations it used to take.

Implementation Details

We'll match throwing behavior of the original APIs. The original APIs did not throw for null or empty strings, they just no-op.

@JeremyKuhne JeremyKuhne added the api-ready-for-review (2) API is ready for formal API review; applied by the issue owner label Jul 27, 2020
@JeremyKuhne JeremyKuhne added this to the 5.0 RC1 milestone Jul 27, 2020
@weltkante
Copy link
Contributor

weltkante commented Jul 27, 2020

Make sure you don't repeat the same mistake the existing interop does, namely allowing writing into immutable strings (this is probably still broken, see #1564). You should prevent calling native code writing into readonly spans by passing through the ModifyString flag unchecked.

Do we want to include fixing #1564 in this API design discussion or should this just be about readonly APIs?

@RussKie RussKie added ⛔ blocked Blocked by external dependencies blocking Used by the API Review Board and removed ⛔ blocked Blocked by external dependencies labels Jul 28, 2020
@JeremyKuhne
Copy link
Member Author

Make sure you don't repeat the same mistake

Thanks for the reminder! I'll make sure the original issue is fixed- we can always add a new API for those that want the truncated behavior. Corrupting strings could go horribly, horribly wrong.

@RussKie RussKie added the tenet-performance Improve performance, flag performance regressions across core releases label Jul 28, 2020
@JeremyKuhne
Copy link
Member Author

The Win32 API in play here is DrawTextExW. When drawing with ellipses it has the option to modify the input string to reflect what actually would be displayed. We didn't block this historically, but won't allow it going forward. In order to safely allow it we would have to have at least four new methods to match the full argument Measure and Draw methods.

@terrajobst terrajobst 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 blocking Used by the API Review Board labels Jul 28, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 28, 2020

Video

  • Looks good as proposed
namespace System.Windows.Forms
{
    public sealed class TextRenderer
    {
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Point pt, Color foreColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, Color backColor);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, Color backColor, TextFormatFlags flags);
        public static void DrawText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Rectangle bounds, Color foreColor, TextFormatFlags flags);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Size proposedSize);
        public static Size MeasureText(IDeviceContext dc, ReadOnlySpan<char> text, Font font, Size proposedSize, TextFormatFlags flags);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font, Size proposedSize);
        public static Size MeasureText(ReadOnlySpan<char> text, Font font, Size proposedSize, TextFormatFlags flags);
    }
}

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Aug 10, 2020
This adds the span overloads per dotnet#3651. It also obsoletes `TextFormatFlags.ModifyString` and fully blocks it in the span overloads. We'll add a safe API for getting output strings in a future version.
@ghost ghost added the 🚧 work in progress Work that is current in progress label Aug 10, 2020
JeremyKuhne added a commit that referenced this issue Aug 11, 2020
* Add Span overloads in TextRenderer

This adds the span overloads per #3651. It also obsoletes `TextFormatFlags.ModifyString` and fully blocks it in the span overloads. We'll add a safe API for getting output strings in a future version.

* Add xml docs based off existing docs.microsoft.com docs.
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Aug 11, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 31, 2022
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 tenet-performance Improve performance, flag performance regressions across core releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants