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 IDeviceContext to events that hold Graphics #3570

Closed
JeremyKuhne opened this issue Jul 13, 2020 · 5 comments · Fixed by #3553
Closed

API Proposal: Add IDeviceContext to events that hold Graphics #3570

JeremyKuhne opened this issue Jul 13, 2020 · 5 comments · Fixed by #3553
Assignees
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 13, 2020

Proposal

Add IDeviceContext to all Windows Forms EventArgs that wrap a System.Drawing.Graphics.

  • PaintEventArgs
  • DrawItemEventArgs
  • DataGridViewRowPostPaintEventArgs
  • DrawListViewItemEventArgs
  • DrawToolTipEventArgs
  • DrawTreeNodeEventArgs
  • ToolStripArrowRenderEventArgs
  • ToolStripContentPanelRenderEventArgs
  • ToolStripItemRenderEventArgs
  • ToolStripPanelRenderEventArgs
  • ToolStripRenderEventArgs
namespace System.Windows.Forms
{
    public class PaintEventArgs : EventArgs, IDisposable, IDeviceContext
    {
        IntPtr IDeviceContext.GetHdc() => Graphics?.GetHdc() ?? IntPtr.Zero;
        void IDeviceContext.ReleaseHdc() => Graphics?.ReleaseHdc();
    }

    public class DrawItemEventArgs: EventArgs, IDeviceContext
    {
        IntPtr IDeviceContext.GetHdc() => Graphics?.GetHdc() ?? IntPtr.Zero;
        void IDeviceContext.ReleaseHdc() => Graphics?.ReleaseHdc();

        void IDisposable.Dispose()
        {
            // IDeviceContext derives from IDisposable. We will not dispose of the contained Graphics
            // as we historically did not take ownership of it.
        }
    }
}

Justification

Windows Forms was designed around GDI+, which System.Drawing is a wrapper around. Graphics was the primary abstraction. Performance of GDI+ wasn't sufficient and internally a number of code paths were changed to utilize the GDI HDC. Graphics exposes the HDC via the IDeviceContext interface, which WinForms uses internally and also exposes on public APIs (see VisualStyleRenderer and TextRenderer).

Exposing IDeviceContext allows us to stop unwrapping event args and allows us to extract the HDC with internal optimizations:

  • Internal state info on the event args allows us to know that we don't need to apply state or create a Graphics object if we haven't already.
  • It also allows end users to pass event args back into IDeviceContext APIs without retrieving Graphics, which can create Graphics or force us to apply it's (likely unchanged) state.

Implementation Details

We'll explicitly implement IDeviceContext to avoid having the methods directly visible on the event args. The only real benefit is treating the event args as an IDeviceContext object.

On event args where we don't already have IDisposable we'll explicitly define IDisposable.Dispose() and we will not dispose passed in arguments (notably Graphics).

@JeremyKuhne JeremyKuhne added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jul 13, 2020
@JeremyKuhne JeremyKuhne self-assigned this Jul 13, 2020
@JeremyKuhne JeremyKuhne added this to the 5.0 RC1 milestone Jul 13, 2020
@JeremyKuhne JeremyKuhne added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation labels Jul 13, 2020
@RussKie RussKie added the Priority:0 Work that we can't release without label Jul 14, 2020
@JeremyKuhne JeremyKuhne added the ⛔ blocked Blocked by external dependencies label Jul 14, 2020
@JeremyKuhne
Copy link
Member Author

How the smuggling works:

namespace System.Windows.Forms
{
    public class PaintEventArgs : EventArgs, IDeviceContext, IInternalInterface {}

    public class SomeControl : Control
    {
         protected override void OnPaint(PaintEventArgs e)
         {
              RendererHelper.DrawThing(e);
         }
    }

    internal static class RendererHelper
    {
        internal static void DrawThing(IDeviceContext context)
        {
             if (context is IInternalInterface internals) ...
        }
    }
}

@RussKie RussKie added blocking and removed ⛔ blocked Blocked by external dependencies labels Jul 15, 2020
@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 labels Jul 16, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2020

Video

  • We should implement IDisposable following the dispose pattern if (1) the type doesn't already implement IDisposable and (2) the type isn't sealed.
  • IDeviceContext should be implemented explicitly.
namespace System.Windows.Forms
{
    public partial class PaintEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class DrawItemEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class DataGridViewRowPostPaintEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class DrawListViewItemEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class DrawToolTipEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class DrawTreeNodeEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class ToolStripArrowRenderEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class ToolStripContentPanelRenderEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class ToolStripItemRenderEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class ToolStripPanelRenderEventArgs : IDisposable, IDeviceContext
    { 
    }
    public partial class ToolStripRenderEventArgs : IDisposable, IDeviceContext
    { 
    }
}

@weltkante
Copy link
Contributor

weltkante commented Jul 16, 2020

We should implement IDisposable following the dispose pattern

Even if you make it public instead of explicit it still should not dispose the contained Graphics instance as the EventArgs instance doesn't own the Graphics instance.

@JeremyKuhne JeremyKuhne removed the Priority:0 Work that we can't release without label Jul 18, 2020
@JeremyKuhne
Copy link
Member Author

Even if you make it public instead of explicit it still should not dispose the contained Graphics instance as the EventArgs instance doesn't own the Graphics instance.

Yes, and I comment on that in the original description. We never dispose passed in Graphics objects.

@JeremyKuhne JeremyKuhne added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner api-approved (4) API was approved in API review, it can be implemented and removed api-approved (4) API was approved in API review, it can be implemented api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Jul 27, 2020
@RussKie RussKie linked a pull request Jul 28, 2020 that will close this issue
@RussKie
Copy link
Member

RussKie commented Jul 28, 2020

Resolved in #3553

@RussKie RussKie closed this as completed Jul 28, 2020
@RussKie RussKie added the tenet-performance Improve performance, flag performance regressions across core releases label Jul 28, 2020
@dotnet dotnet 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