Skip to content

Adding new strongly typed methods to Clipboard, DataObject and IDataObject.#11545

Merged
Tanya-Solyanik merged 9 commits intodotnet:mainfrom
Tanya-Solyanik:cb-9
Dec 12, 2024
Merged

Adding new strongly typed methods to Clipboard, DataObject and IDataObject.#11545
Tanya-Solyanik merged 9 commits intodotnet:mainfrom
Tanya-Solyanik:cb-9

Conversation

@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

@Tanya-Solyanik Tanya-Solyanik commented Jun 18, 2024 •
edited
Loading

Related to ##12362
Fixes #11350

The TryGetData methods use NRBF deserializer by default and will fall back to use BinaryFormatter if the application opts into BinaryFormatter use in this context.
The GetData methods have a compatibility mode when the appropriate AppContext switches are enabled but by default they can read only known and primitive types or POCOs with primitive fields. These methods in the DataObject class are obsoleted.

Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the draft draft PR label Jun 18, 2024
@Tanya-Solyanik Tanya-Solyanik force-pushed the cb-9 branch 3 times, most recently from e246a67 to a89070f Compare June 18, 2024 15:12
lonitra
lonitra reviewed Jun 18, 2024
View reviewed changes
JeremyKuhne
JeremyKuhne requested changes Jun 20, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments.

@dotnet-policy-service dotnet-policy-service bot added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Jun 20, 2024
@Tanya-Solyanik Tanya-Solyanik force-pushed the cb-9 branch 6 times, most recently from 816f4c9 to 7cc2fdc Compare June 24, 2024 15:19
JeremyKuhne
JeremyKuhne requested changes Jun 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more comments. I'm a little concerned that we're using TypeName where it isn't necessary in some cases. We should optimize this before we fully validate this as changing it afterwords would be risky.

I still need to spend more time reviewing.

@dotnet-policy-service dotnet-policy-service bot added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Jun 24, 2024
@Tanya-Solyanik Tanya-Solyanik force-pushed the cb-9 branch 2 times, most recently from b670471 to c1e4bcd Compare June 24, 2024 20:37
JeremyKuhne
JeremyKuhne requested changes Jun 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be falling into the default interface implementations for our own types that derive from IDataObject.

@dotnet-policy-service dotnet-policy-service bot added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Jun 24, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.00605% with 104 lines in your changes missing coverage. Please review.

Project coverage is 97.03763%. Comparing base (0b07c20) to head (38fd447).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                  @@
##                main      #11545          +/-   ##
====================================================
+ Coverage   75.91821%   97.03763%   +21.11941%     
====================================================
  Files           3164        1182        -1982     
  Lines         636813      354547      -282266     
  Branches       47008        5410       -41598     
====================================================
- Hits          483457      344044      -139413     
+ Misses        149901        9711      -140190     
+ Partials        3455         792        -2663
Flag Coverage Δ
Debug 97.03763% <93.00605%> (+21.11941%) ⬆️
integration ?
production ?
test 97.03763% <93.00605%> (-0.01368%) ⬇️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@Tanya-Solyanik Tanya-Solyanik force-pushed the cb-9 branch 3 times, most recently from 7903b3c to 9f06188 Compare July 24, 2024 21:20
@lonitra lonitra changed the base branch from feature/clipboard to feature/10.0 July 30, 2024 20:12
@lonitra lonitra changed the base branch from feature/10.0 to main August 15, 2024 00:59
JeremyKuhne
JeremyKuhne previously requested changes Nov 13, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments

...System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.TypeNameComparer.cs
return Matches(x, y);
}

public int GetHashCode(TypeName obj)
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this come from? If the code is trying to avoid allocating the full name string, ok, but it should probably use HashCode.Combine().

Copy link
Copy Markdown
Contributor Author

@Tanya-Solyanik Tanya-Solyanik Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't use the full name as an identity because it could contain assembly name in constructed types, for example, that might be type-forwarded to another assembly.

Copy link
Copy Markdown
Contributor Author

@Tanya-Solyanik Tanya-Solyanik Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this come from? - I didn't find anything in the runtime.. Do you have suggestions where to search?

Copy link
Copy Markdown
Contributor Author

@Tanya-Solyanik Tanya-Solyanik Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't use HashCode.Combine as it does not call this GetHashCode method recursively, most of these values are TypeNames and should have their assembly names removed.

src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.Binder.cs Outdated
// This is needed to resolve fields of the requested type T when using deserializers.
private readonly Dictionary<string, Type> _mscorlibTypeCache = new()
{
{ "System.Byte", typeof(byte) },
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because then you are comparing the right components- it is the whole point of TypeName. String comparison is fraught with errors.

@dotnet-policy-service dotnet-policy-service bot added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Nov 13, 2024
@Tanya-Solyanik Tanya-Solyanik force-pushed the cb-9 branch 6 times, most recently from 4a6917d to b7a8ed8 Compare November 14, 2024 21:00
adamsitnik
adamsitnik reviewed Nov 15, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I provided the answers to all the questions, please feel free to tag me again.

lonitra
lonitra reviewed Dec 9, 2024
View reviewed changes
lonitra
lonitra reviewed Dec 9, 2024
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/OLE/ITypedDataObject.cs Outdated

namespace System.Windows.Forms;

public interface ITypedDataObject
Copy link
Copy Markdown
Member

@lonitra lonitra Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a resolution to this thread? It still seems our guidance is that both should be implemented.

@Tanya-Solyanik
reactive changes
993f011
@Tanya-Solyanik
Implemented ITypedDataObject
1e1fcb0
@Tanya-Solyanik
Throw the NotSupportedException that was serialized to the clipboard,…
101f75d 
… for example when the OOB Formatters assembly is not referenced of enabled.
@Tanya-Solyanik
Do not attempt to read HGLOBAL if managed serialization failed when w…
488325d 
…riting data to it
@Tanya-Solyanik
Review feedback - renamed the test scope, pattern matching in DataSto…
2f4b81e 
…re, string interpolation in test file; simplify code to read forwarded from attribute
@Tanya-Solyanik
ITypedDataObject implements IDataObject
63c1a61
@Tanya-Solyanik
match known types using TypeNames
6e976df 
generate TYpeName taking into account what assembly the type was forwarded from
@Tanya-Solyanik
Merging the BitmapBinder removal
72216fb
lonitra
lonitra reviewed Dec 11, 2024
View reviewed changes
src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs

if (dataObject is not ITypedDataObject typedDataObject)
{
// TODO (TanyaSo): localize string
Copy link
Copy Markdown
Member

@lonitra lonitra Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this will be done in follow up PR?

Copy link
Copy Markdown
Contributor Author

@Tanya-Solyanik Tanya-Solyanik Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

lonitra
lonitra approved these changes Dec 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice! 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeremyKuhne JeremyKuhne JeremyKuhne left review comments

+2 more reviewers

@adamsitnik adamsitnik adamsitnik left review comments

@lonitra lonitra lonitra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tanya-Solyanik Tanya-Solyanik

Labels

draft draft PR

Projects

None yet

Milestone

10.0 Preview1

Development

Successfully merging this pull request may close these issues.

Add GetData<T> Method to Clipboard

4 participants

@Tanya-Solyanik @JeremyKuhne @adamsitnik @lonitra