Restructure DataObject #12792
Restructure DataObject #12792
Conversation
| /// <param name="resolver"></param> | ||
| /// <param name="legacyMode">Indicates whether this was called from legacy method family <see cref="IDataObjectDesktop.GetData(string)"/></param> | ||
| /// <returns>The object of type T if successfully read. Otherwise, <see langword="null"/>.</returns> | ||
| internal abstract object? ReadObjectFromStream<T>( |
There was a problem hiding this comment.
The implementation we have for this method and below likely we want to share, however, this was made abstract due to Binder being abstract. Binder needs to be abstract because it is where we indicate additional supported types that are not common between applications e.g. ImageListStreamer. We can probably find a way to largely share the implementation, but dealing with extension methods may also be tricky.
| /// before method ends. | ||
| /// </para> | ||
| /// </remarks> | ||
| public abstract Composition PopulateFromRuntimeDataObject(ComTypes.IDataObject runtimeDataObject); |
There was a problem hiding this comment.
We also likely want to largely share this implementation, but this needed to be made abstract due to DesktopToNativeAdapter and NativeToDesktopAdapter being abstract.
| /// <inheritdoc cref="DataObject(object)"/> | ||
| internal DataObject(Com.IDataObject* data) => _innerData = Composition.CreateFromNativeDataObject(data); | ||
| /// <inheritdoc cref="DesktopDataObject(object, Composition)"/> | ||
| internal DesktopDataObject(Com.IDataObject* data, Composition composition) => _innerData = composition.PopulateFromNativeDataObject(data); |
There was a problem hiding this comment.
We need to pass Composition here now because Composition is abstract. It would be nice if we did not need to do this as we are not programmatically enforcing that Composition is properly populated and instead rely that the Composition implementation calls Composition.Populate.
| /// <summary> | ||
| /// Extension methods for data objects. | ||
| /// </summary> | ||
| internal static class DesktopDataObjectExtensions |
There was a problem hiding this comment.
This is the same as DataObjectExtensions. DataObjectExtensions cannot be moved because it is public, but perhaps we can potentially reduce duplication. It is tricky though due to this one referring to IDataObjectDesktop and the other is referring to IDataObject.
| private static Format[]? s_formatList; | ||
|
|
||
| private static int s_formatCount; |
There was a problem hiding this comment.
We actually test that calling GetFormat gives back the same DataFormat instance which is why the public version has its own s_formatList. Because of this, there are some duplicated work around managing the list that perhaps could be better.
| @@ -19,32 +21,6 @@ namespace System.Windows.Forms; | |||
| /// </summary> | |||
| internal static unsafe class DragDropHelper | |||
There was a problem hiding this comment.
With this change we have an internal and public DragDropHelper. We should probably move this class entirely to the shared assembly after working out any references to WinForms specific types.
| internal override bool TryGetDataCore<[DynamicallyAccessedMembers((DynamicallyAccessedMemberTypes)(-1))] T>(string format, Func<TypeName, Type>? resolver, bool autoConvert, [MaybeNullWhen(false), NotNullWhen(true)] out T data) | ||
| { | ||
| if (!_haveCheckedOverride && _dataObject is not null && _dataObject.GetType() != typeof(DataObject)) | ||
| { | ||
| // TryGetDataCore could be overridden. Call the potential overridden version and flag that it's been called so that | ||
| // we don't end up in an infinite loop if it hasn't been overridden. | ||
| _haveCheckedOverride = true; | ||
| bool result = _dataObject.TryGetDataCoreInternal(format, resolver, autoConvert, out data); | ||
| _haveCheckedOverride = false; | ||
| return result; | ||
| } | ||
|
|
||
| return base.TryGetDataCore(format, resolver, autoConvert, out data); | ||
| } |
There was a problem hiding this comment.
The issue here is that we are calling this method (among other methods I missed in current changeset such as SetText, SetAudio, GetText) internally in DesktopDataObject. If the user has overridden this method then we won't call their override version. We need to bridge the gap so that those overrides get called hence the need for WinFormsDataObject to have a reference to DataObject. We also need to watch out for infinite loops occurring if the user has not overridden the method hence _haveCheckedOverride. Though we need a better way of working around this problem because as it currently stands their override method is called more times than expected.
DO NOT MERGE
Related: #12179
Restructure DataObject and move to System.Private.Windows.Core assembly so that implementation can be shared with WPF as our implementations are largely similar. The goal is to move it to the shared assembly while preserving important history of DataObject and Clipboard.
Microsoft Reviewers: Open in CodeFlow