dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.35k stars 964 forks source link

More trimming friendly DragDrop #6745

Closed kant2002 closed 3 weeks ago

kant2002 commented 2 years ago

Currently DragDrop process starts with DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects) which inside perform dispatcherization of the supported interfaces and objects. Sometimes this may means that we rely on Built-in COM support for cast of the interfaces.

Built-in COM support maybe disabled in modern .NET and do not supported in NativeAOT flavor of CoreCLR. I propose provide API which put responsibility of AOT supported casting on the consumer side by introducing typed version of the method.

public class Control
{
+ [BuiltInComDragonsHere]
  public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects)
+ public DragDropEffects DoDragDrop(System.Runtime.InteropServices.ComTypes.IDataObject data, DragDropEffects allowedEffects)
+ public DragDropEffects DoDragDrop(System.Windows.Forms.IDataObject data, DragDropEffects allowedEffects)
}
public class ToolStripItem
{
+ [BuiltInComDragonsHere]
  public DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects)
+ public DragDropEffects DoDragDrop(System.Runtime.InteropServices.ComTypes.IDataObject data, DragDropEffects allowedEffects)
+ public DragDropEffects DoDragDrop(System.Windows.Forms.IDataObject data, DragDropEffects allowedEffects)
}

This would be considered source breaking change.

/cc @weltkante since you maybe interested in giving feedback on this.

RussKie commented 2 years ago

Can you please elaborate on how you envision this API to used? And how it will affect consumers of the existing API?

kant2002 commented 2 years ago

This API should be used instead of similar method DragDropEffects DoDragDrop(object data, DragDropEffects allowedEffects) which are already exists. Currently this method can accept

  1. Random .NET object
  2. Inheritor of S.W.F.IDataObject
  3. Inheritor of S.R.I.ComTypes.IDataObject
  4. Inheritor of type which has same shape as S.R.I.ComTypes.IDataObject (built-in com translate that type to S.R.I.ComTypes.IDataObject)

I want new API methods cover items 2 and 3 respectively and get rid of item 4. Let's talk about item 1 Under the hood if I pass random .NET object, then it is wrapped into S.W.F.DataObject, it would be trivial change in order to support be explicit.

Item 4 is most important in my opinion. This scenario will never work with built-in COM disabled and inside NativeAOT. Having method which works only sometimes is problematic IMO. So I want push responsibility for conversion of COM methods to user code, and out of WinForms code. Statements is like this. If you use 2 new API you are safe in NativeAOT/without built-in COM. If you use old method, you on your own. Most likely it would work, but if not, that's not our problem.

I expand API proposal to indicate that we need to add some attribute to existing method, so ILLinker tooling (or some else) would complain. I do not know exact attributes. Maybe RequiredDynamicallyAccessibleCode.

For consumers

Item 2 and Item 3

nothing should be done. But this is source code breaking change, since different method would be used.

Item 1

Before

int x = 1;
DoDragDrop(x, DragDropEffects.All)

After

int x = 1;
DoDragDrop(new DataObject(x), DragDropEffects.All)

Item 4

Before

var x = new CustomObjectWithSimilarComType();
DoDragDrop(x, DragDropEffects.All)

After

var x = new CustomObjectWithSimilarComType();
DoDragDrop((System.Runtime.InteropServices.ComTypes.IDataObject)x, DragDropEffects.All)

or even better

var x = new CustomObjectWithSimilarComType();
DoDragDrop(new SpecialDataObjectForCustomObjectWithSimilarComType(x), DragDropEffects.All)
kant2002 commented 2 years ago

@RussKie do you need additional information?

RussKie commented 2 years ago

The team will discuss in one of the upcoming triages.

kant2002 commented 2 years ago

@RussKie are the team has possibility to discuss this issue on the meetings?

RussKie commented 2 years ago

@kant2002 thank you for the nudge. Yes, absolutely! I've been away for few weeks, and just got back. Still catching up...

/cc: @merriemcgaw @dreddy-work @JeremyKuhne

JeremyKuhne commented 2 years ago

@kant2002 Is the goal here to simply make sure the two IDataObject types are explicitly referenced? Is there some other way we can clue in the trimmer? I would think it is fair to pull in both anytime someone uses the DragDrop APIs.

kant2002 commented 2 years ago

@JeremyKuhne the goal is shift responsibility for built-in COM from WinForms to user code. object can be _ComObject if unknown nature.

JeremyKuhne commented 2 years ago

@kant2002 sorry, I'm not clear on how that helps. @AaronRobinsonMSFT are there resources on built-in COM and trimming? All I really know at this point is that we should be moving away from ComImport where possible, but I don't even really know why. :)

kant2002 commented 2 years ago

Consider this piece of code somewhere in the external library.

[ComImport]
[Guid("XXXX")]
interface IMyDataObject
{
    // similar implementation goes here.
}

static extern GetMyDataObject(out IMyDataObject dataObject);
...
// inside control
GetMyDataObject(out var problemObject);
DoDragDrop(problemObject, DragDropEffects.All);

For the runtime object problemObject actual type is _ComObject. Because this is Com Proxy, for runtime IMyDataObject and ComType.IDataObject are same types and can be converted to each other, so lines https://github.com/dotnet/winforms/blob/e0e125abe00ae53cd102df47ed9a55deb2d5010d/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.cs#L2136-L2139 in regular runtime with built-in COM enabled would be interpreted as success pass. That's existing behavior. If we disable built-in COM, then behavior would change, and conversion would not work. That's WinForms who change behavior. Consider slightly different call now.

DoDragDrop((ComTypes.IComObject)problemObject, DragDropEffects.All);

In this case it's user-code does not working without built-in COM, and as such it's within developer's reach to fix that. and with [RequiresUnreferencedCode], I think that issue become better discoverable.

AaronRobinsonMSFT commented 2 years ago

Are we sure the above APIs will actually help in a full featured WinForms way? What about the entire automation stack that is present in a Drag-n-Drop scenario?

@kant2002 I'm not saying we shouldn't add the RequiresUnreferencedCode to the existing API, but I don't see how the two additional overrides help push anything further along.

@JeremyKuhne The best source of details on the trimmer reside with either @vitek-karas or @agocke. It always takes me a minute to page back in what is the right thing to do. I'll need a day or two.

JeremyKuhne commented 2 years ago

@kant2002 I wasn't aware you could cast _ComObject that way, but it makes sense when I think about it.

AaronRobinsonMSFT commented 2 years ago

@kant2002 I wasn't aware you could cast _ComObject that way, but it makes sense when I think about it.

Yes. A cast on an _ComObject is really a QueryInterface under the covers so all that matters is the GUID of the type being casted to. This is the base case though. There are other scenarios in .NET Framework where one can activate a COM server implemented in managed code and then cast the activate COM server to the managed implemented type rather than any COM interface.

For example:

// Implemented in a server assembly
class MyComServer : IFoo { }

// COM activate in client code, but still reference server assembly
Guid riid = typeof(IFoo).GUID;
IFoo f = (IFoo)Native.CoCreateInstance(clsid, ..., riid); 
MyComServer cs = (MyComServer)f;
JeremyKuhne commented 2 years ago

Talking with @agocke the presumption is that you will never see _ComObject when trimming is enabled as COM interop is disabled. @AaronRobinsonMSFT is that a correct assumption?

AaronRobinsonMSFT commented 2 years ago

Talking with @agocke the presumption is that you will never see _ComObject when trimming is enabled as COM interop is disabled. @AaronRobinsonMSFT is that a correct assumption?

Yes. The _ComObject represents the runtime's RCW. It has specialized handling for casts and other behavior that permit it to look and feel like a normal .NET object. The equivalent casting behavior is provided with ComWrappers via the IDynamicInterfaceCastable (IDIC) API - which is what @kant2002 has been using in most of his PRs. A self-contained sample for IDIC can be found here.

kant2002 commented 2 years ago

I hope that this would be better version of same methods. Approach like in this PR https://github.com/dotnet/runtime/pull/70224 @JeremyKuhne what do you think?

JeremyKuhne commented 1 month ago

@kant2002 We've removed all built-in COM usage for OLE scenarios in .NET 9. Is there anything else remaining in the ask here?

dotnet-policy-service[bot] commented 1 month ago

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.