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

Fixed Clipboard.SetDataObject to work with custom formats #415 #416

Merged
merged 4 commits into from Feb 20, 2019

Conversation

Projects
None yet
6 participants
@JoeErickson
Copy link
Contributor

JoeErickson commented Feb 7, 2019

fixes #415

While testing SpreadsheetGear for Windows Forms on .NET Core 3 we found that Clipboard.SetDataObject writes uninitialized data to the clipboard instead of serialized .NET objects.

This bug is apparently a regression from Span work in 37abb3a.

This change includes a simple fix and a unit test.

There is one potential issue with the unit test - it does change the clipboard of the computer it is run on. I could not find any other tests which do so and I'm not sure of the ramifications of that other than the fact that is is a nuisance for anyone using the computer the test is run on. Having said that, it seems to me that unit tests to verify that the clipboard works is important...

@JoeErickson JoeErickson requested a review from dotnet/dotnet-winforms as a code owner Feb 7, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Feb 8, 2019

@raffaeler would you mind taking a look at this?

@JoeErickson as far as the tests go, is there a way to mock the clipboard?

System.Windows.Forms.Clipboard does not appear to extend any interface you could use for this purpose. Perhaps we could do so? @AdamYoblick,@dreddy-work, @Tanya-Solyanik what do you three think? Just create an interface that declares IsFormatValid, SetDataObject, GetDataObject, Clear, SetData, SetText, GetData, GetText, -- I think those are the only ones that would be necessary, at least for this PR 😄

@merriemcgaw merriemcgaw requested a review from Tanya-Solyanik Feb 8, 2019

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 8, 2019

@zsd4yr I have my doubts about mocking Clipboard.SetDataObject / GetDataObject since they involve converting / marshaling to OLE COM / Win32 objects and then back to .NET objects. Of course the winforms team should have a much better idea than me about that. This is my first pull request for any .NET Core project and the first time I've used XUnit.

If you do change to mock the clipboard you should be able to change from '[WinFormsFact]' to '[Fact]' for the test I added.

@raffaeler

This comment has been minimized.

Copy link
Contributor

raffaeler commented Feb 11, 2019

@zsd4yr sure :)

@JoeErickson could you please provide a simple example of the problem (without using SpreadsheetGear or any other 3rd party component) please?

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 11, 2019

@raffaeler The pull request includes a unit test which does not use any third party code.

@raffaeler

This comment has been minimized.

Copy link
Contributor

raffaeler commented Feb 11, 2019

@JoeErickson oh sure, sorry.

Talking in general, I never expect a method taking a stream, also rewinding it. If a method receives a stream, it is expected to read or write it, starting from its current position.
On the other side, it looks like the behavior has been changed by my PR.

I believe the team should take a decision about that.
In any case, thank you for rising the issue.

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 11, 2019

@raffaeler It appears there are only two references to this method I changed. In the case which my unit test exercises a .NET object is serialized to a MemoryStream and then this method is called so (Stream.Position == Stream.Length) is always true. In the other case it is not clear without investigating.

@zsd4yr Do you have any idea when this fix might be released in a daily build so we can run our automated tests against it (we have many thousands of automated tests for SpreadsheetGear built up over the last 15+ years)? I spent some time trying to run our tests against the version I fixed but kept running into problems.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Feb 12, 2019

Do you have any idea when this fix might be released in a daily build so we can run our automated tests against it (we have many thousands of automated tests for SpreadsheetGear built up over the last 15+ years)? I spent some time trying to run our tests against the version I fixed but kept running into problems.

For the daily builds of the Core SDK, on a good week this will take a few days. Unfortunately, there is no sure-fire way to easily determine if your bits have gotten in yet; this is something we are working on as well.

Some more transparency: once these bits pass the CI and we review / take them, they are mirrored internally. They must then pass another CI, and then a transportation package is generated. It can take about a day for that to be picked up by our composition repository, which we share ownership of with WPF. Once that winforms transportation package is taken in by the composition repo, it is mixed with WPF bits to form the Microsoft.WindowsDesktop.App package (that is what will eventually make it into the SDK). That has to pass another CI, and then the package is generated. It takes usually another day for that to get merged into the SDK, but that is out of our direct control. They're good about merging stuff in tho.

tl;dr at least two days after we take a change.

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 13, 2019

@zsd4yr Thanks for taking the time to provide insight into the process. I'll try a new daily build in a few days.

I'm excited to see WinForms and WPF on .NET Core 3 - especially the commitment to making them work well with multi-monitor high dpi.

We provide IDE Designers for our controls in Visual Studio for the .NET Framework. I've seen the mentions of an out of proc Visual Studio designer for DNC3 and would be happy to test this as soon as I can get access to it.

@weltkante

This comment has been minimized.

Copy link

weltkante commented Feb 13, 2019

Talking in general, I never expect a method taking a stream, also rewinding it. If a method receives a stream, it is expected to read or write it, starting from its current position.
On the other side, it looks like the behavior has been changed by my PR.

It's been broken. You create a span of stream.Length but then read relative to stream.Position (implicitely, by not resetting it). If you want to read relative to the current position you can't take stream.Length as Span size.

I believe the team should take a decision about that.
In any case, thank you for rising the issue.

If this needs a decision the breaking commit should be rolled back for now. Right now its simply wrong.

@raffaeler

This comment has been minimized.

Copy link
Contributor

raffaeler commented Feb 13, 2019

@weltkante I am not saying it is not a bug!
But before fixing, I am asking the team to choose whether rewinding the stream or not, considering the compatibility implications.

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 13, 2019

@raffaeler @weltkante @zsd4yr

Here is the currently proposed fix to DataObject.SaveStreamToHandle(…) which adds back the 'stream.Position = 0;':

    var span = new Span<byte>(ptr.ToPointer(), size);
    stream.Position = 0;
    stream.Read(span);

Here is the .NET Core 3 WinForms code before the Span work:

    byte[] bytes = new byte[size];
    stream.Position = 0;
    stream.Read(bytes, 0, size);
    Marshal.Copy(bytes, 0, ptr, size);

And here is the .NET Framework reference source for this:

    byte[] bytes = new byte[size];
    stream.Position = 0;
    stream.Read(bytes, 0, size);
    Marshal.Copy(bytes, 0, ptr, size);

As you can see, the .NET Framework reference source and the .NET Core 3 code before the Span work are identical.

I am not a Span or IntPtr expert but some quick googling indicates to me that the current version with my proposed fix to set Stream.Position to zero is functionally identical to the previous version (both .NET Core 3 and .NET Framework reference source) - and definitely more performant which is a good thing for SpreadsheetGear users since it is not unusual for our users to place very large (even 100+ megabyte) objects on the clipboard via this code path.

Please accept the pull request since this is currently a blocker for SpreadsheetGear running on .NET Core 3 WinForms.

@raffaeler

This comment has been minimized.

Copy link
Contributor

raffaeler commented Feb 13, 2019

@JoeErickson I am not a reviewer and I can't approve any pull-request.
I was just asked "taking a look" to the issue as I was the author of the Span PR (which was reviewed and approved).
From the span perspective, your fix of course works and I am not against it at all, I just gave a broader perspective, that's all.

@RussKie

This comment has been minimized.

Copy link

RussKie commented Feb 13, 2019

There is one potential issue with the unit test - it does change the clipboard of the computer it is run on. I could not find any other tests which do so and I'm not sure of the ramifications of that other than the fact that is is a nuisance for anyone using the computer the test is run on. Having said that, it seems to me that unit tests to verify that the clipboard works is important...

You could probably read the current clipboard data before the test, and restore it upon exit, e.g.

var currentData = clipboard.GetDataObject();
try {
    // run the test
}
finally {
    clipboard.SetDataObject(currentData);
}

Would something like this work?

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Feb 13, 2019

You could probably read the current clipboard data before the test, and restore it upon exit

This is a good idea. See below comment. I would like to reconsider mocking in order to avoid having to run the unit test on a machine with a user / in interactive mode. I am unsure if "headless" machines have a clipboard at all.

@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 13, 2019

@RussKie Saving and restoring is much more complex than that. Many non .NET Win32 programs register formats but do not render them until they are asked for. I just did a Copy in Excel and got these formats:

    EnhancedMetafile, MetaFilePict, Bitmap, Biff12, Biff8, Biff5, SymbolicLink, DataInterchangeFormat, 
    XML Spreadsheet, HTML Format, UnicodeText, Text, Csv, Rich Text Format, Embed Source, 
    Object Descriptor, Link Source, Link Source Descriptor, Link, Format129

Who knows what "Format129" is and how you would save and restore it? There are other complications such as the way Cut and Paste is handled in Excel and SpreadsheetGear with formula fixups, etc... and those would be broken if you modified the clipboard while someone is using it. That is just 2 programs - I'm sure there are a million different kinds of things put on the Win32 clipboard and no way to anticipate the ramifications of trying to save it and restore it - especially given the fact that .NET has a limited clipboard implementation to begin with.

There is also the fact that if someone is running the test and happens to modify the clipboard in the middle of a test, then the test will fail.

With SpreadsheetGear automated tests we just accept the fact that the tests modify the clipboard and we run them in a HyperV VM which does not share the clipboard with the viewer (we use HyperV manager non-enhanced mode to run these tests since it is the most stable and contained way we know to do it).

IMO the fix is safe to merge even if you choose not to merge the test - it is a trivial reversion to setting Stream.Position to 0 as the code did before the Span<T> work.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Feb 13, 2019

There is also the fact that if someone is running the test and happens to modify the clipboard in the middle of a test, then the test will fail.

this is also a really good point. because copy and paste can come from user commands via the keyboard, they are an interrupt and could potentially fall in between cycles on the processor which make up the commands that are this test.

Update src/System.Windows.Forms/tests/UnitTests/DataObject.cs
Co-Authored-By: JoeErickson <joe@spreadsheetgear.com>
@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 14, 2019

I would like to reconsider mocking in order to avoid having to run the unit test on a machine with a user / in interactive mode.

The problem with this test is if it IS run with a user in interactive mode - and that is only a problem if the user interacts with input devices or expects their clipboard to not change. IMO you might as well remove the test (it was not being tested before) if you are going to mock it.

I am unsure if "headless" machines have a clipboard at all.

It should run fine on a headless machine - as long as it isn't Windows Nano Server. It is unlikely that there is a situation where you can test other WinForms elements (windows, controls, focus, etc...) and not test the clipboard.

If these tests are running on headless VMs anyway then I suggest ignoring my "one potential issue" comment.

@zsd4yr : This pull request passes the "CI" check - does that mean it has already successfully run these new tests on said headless machine?

@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik left a comment

Hi @JoeErickson, the fix looks good, but I share your concerns about unit tests for Clipboard. Additionally Clipboard APIs can time out when the clipboard is busy, and this can happen on the developer machine. I suggest that you remove test from this PR and create another issue to add clipboard test to functional/integration tests.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Feb 15, 2019

@JoeErickson per Tanya's request: please remove test from this PR and create another issue to add clipboard test to functional/integration tests. Then we can take this PR.

Joe Erickson
@JoeErickson

This comment has been minimized.

Copy link
Contributor Author

JoeErickson commented Feb 15, 2019

@zsd4yr @Tanya-Solyanik I deleted the test from my fork and pushed it and it appears to be reflected in this pull request. I am a GitHub newby so hopefully this is what is needed. I can no longer build dotnet/winforms after updating to VS 2019 Preview 3 and wasted too much time trying to figure that out (I get a bunch of errors about downgrading references) so I'm hoping this fix can be taken as is. It is just the single 'stream.Position = 0;' addition to DataObject.cs.

@zsd4yr

zsd4yr approved these changes Feb 19, 2019

@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik left a comment

Thank you!

@Tanya-Solyanik Tanya-Solyanik merged commit 74b569f into dotnet:master Feb 20, 2019

1 check passed

license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment