Cleanup EDITSTREAM #2816

hughbe · 2020-02-07T13:39:35Z

Proposed Changes

Cleanup EDITSTREAM

Microsoft Reviewers: Open in CodeFlow

RussKie · 2020-02-11T05:07:57Z

MC, and (I suspect) test failures

RussKie

Have you tested it in any way?

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.EDITSTREAM.cs

hughbe · 2020-02-13T11:45:23Z

hmm there may be different padding on 64 bit vs. 32 bit structs. @weltkante if you have any immediate things you notice

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.EDITSTREAM.cs

weltkante · 2020-02-13T11:54:46Z

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.EDITSTREAM.cs

+            public uint dwError;
+            public IntPtr pfnCallback;


hmm there may be different padding on 64 bit vs. 32 bit structs. @weltkante if you have any immediate things you notice

Yes, you are adding 4 byte undesired padding in 64bit between these fields. You can solve this without making two structs by forcing alignment to 4 byte instead of keeping it at native size. So adding a [StructLayout(LayoutKind.Sequential, Pack = 4)] should do the trick.

For the record, in the header file the place where it deviates from standard packing rules is by declaring

#ifdef _WIN32 #include <pshpack4.h> #elif !defined(RC_INVOKED) #pragma pack(4) #endif

This is which forces Pack=4 in C# terms for all structs in this header file. Only relevant if the struct contains pointers or doubles (i.e. anything larger than 4 byte)

It's unfortunate that the headers have these sprinkled around instead of directly on the declarations like C# enforces, you always have to search for them if you want to make sure you are using the right packing rules.

Keeping it as "unresolved" for the ease of future searching

When I'm writing unmanaged struct wrappers I'll sometimes write unit tests that check sizeof-- particularly for less common or complicated structures. I manually check the sizeof in a C++ dll and hard code the values, but that could also be done in code by having an export that allows you to query.

I'd consider:

Writing tests that check the size

Testing interop in both x86 and x64

Using a cross-compiled native dll to expose the C sizeof for structs (in addition to other native helpers for testing)

Writing tests that check the size

TBD.

Testing interop in both x86 and x64

I have raised #2879 for CI enhancements. [EDIT] Merged

Using a cross-compiled native dll to expose the C sizeof for structs (in addition to other native helpers for testing)

@hughbe you were working on #1932. Perhaps it can be leveraged for this purpose.

@hughbe you were working on #1932. Perhaps it can be leveraged for this purpose.

This is not going to be merged for sometime as it is waiting on upstream arcade changes.

I have written a bunch of tests to ensure near 100% coverage as well as verifying the size on x86 and x64

codecov · 2020-02-13T15:58:01Z

Codecov Report

Merging #2816 into master will increase coverage by 0.09665%.
The diff coverage is 97.61905%.

@@                 Coverage Diff                 @@
##              master       #2816         +/-   ##
===================================================
+ Coverage   60.27793%   60.37459%   +0.09666%     
===================================================
  Files           1249        1249                 
  Lines         433633      434552        +919     
  Branches       38850       38849          -1     
===================================================
+ Hits          261385      262359        +974     
+ Misses        166879      166822         -57     
- Partials        5369        5371          +2

Flag	Coverage Δ
#Debug	`60.37459% <97.61905%> (+0.09665%)`	⬆️
#production	`32.48554% <97.61905%> (+0.02156%)`	⬆️
#test	`98.96922% <ø> (-0.00617%)`	⬇️

RussKie

What happened to EDITSTREAM clean up? Why do we have only test changes?

hughbe · 2020-02-26T12:55:29Z

I was testing to see whether my changes regressed any tests. Turns out we may have a bug. I wrote the tests against full .NET Framework but they fail on .NET Core - it looks like SelectedRtf may be broken

    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainTextWithHandle_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1309,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainTextWithHandle_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainText_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1282,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainText_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1336,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1336,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithHandle_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1257,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithHandle_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1365,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1365,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1428,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1428,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1396,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1396,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Get_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1234,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Get_ReturnsExpected()

weltkante · 2020-02-26T13:45:06Z

I can't quite follow, when I look at the SelectedRtf tests they seem wrong to me and they also fail on Desktop Framework.

In short, if there is no Text or SelectedText then SelectedRtf is empty. That behavior also happens on Desktop for me and IMHO its reasonable behavior. Note that there exist different versions of the RTF control and maybe you're running your Desktop tests against an older version which behaved differently. .NET Core only supports using the newest version.

hughbe · 2020-02-28T12:04:18Z

Note that there exist different versions of the RTF control and maybe you're running your Desktop tests against an older version which behaved differently. .NET Core only supports using the newest version

Yeah seems like this was the problem. Cheers!

src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.SF.cs

RussKie · 2020-03-09T02:43:06Z

Docs: dotnet/docs#17085

RussKie · 2020-03-09T02:43:55Z

@weltkante any more concerns?

hughbe requested a review from a team as a code owner February 7, 2020 13:39

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from c51cc89 to 0cc9710 Compare February 7, 2020 13:48

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from 0cc9710 to 5aee73d Compare February 11, 2020 14:45

ghost assigned hughbe Feb 11, 2020

RussKie reviewed Feb 12, 2020

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.EDITSTREAM.cs Outdated Show resolved Hide resolved

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Feb 12, 2020

RussKie closed this Feb 12, 2020

RussKie reopened this Feb 12, 2020

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Feb 13, 2020

weltkante suggested changes Feb 13, 2020

View reviewed changes

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Feb 13, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from 5aee73d to b65cce2 Compare February 13, 2020 13:12

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Feb 13, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from b65cce2 to 77bd65e Compare February 13, 2020 15:43

RussKie requested a review from JeremyKuhne February 14, 2020 01:39

RussKie added waiting-review This item is waiting on review by one or more members of team 📭 waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Feb 14, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from 77bd65e to f2b81bb Compare February 24, 2020 16:01

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Feb 24, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from f2b81bb to a9271e5 Compare February 24, 2020 17:31

RussKie approved these changes Feb 24, 2020

View reviewed changes

RussKie requested changes Feb 24, 2020

View reviewed changes

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Feb 24, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from a9271e5 to 914d728 Compare February 26, 2020 12:54

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Feb 26, 2020

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch 5 times, most recently from 08eaae9 to ace6d19 Compare February 27, 2020 11:41

RussKie requested changes Mar 4, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs Show resolved Hide resolved

src/System.Windows.Forms.Primitives/src/Interop/Richedit/Interop.SF.cs Outdated Show resolved Hide resolved

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Mar 4, 2020

RussKie added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Mar 4, 2020

Add RichTextBox tests and fix bug

f67cff3

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from ace6d19 to 909cf71 Compare March 4, 2020 12:47

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Mar 4, 2020

Cleanup EDITSTREAM

df8798e

hughbe force-pushed the Cleanup-GETTEXTLENGHTEX branch from 909cf71 to df8798e Compare March 4, 2020 12:48

RussKie approved these changes Mar 5, 2020

View reviewed changes

weltkante approved these changes Mar 9, 2020

View reviewed changes

RussKie merged commit 894e22f into dotnet:master Mar 9, 2020

ghost added this to the 5.0 milestone Mar 9, 2020

RussKie added enhancement Product code improvement that does NOT require public API changes/additions test-enhancement Improvements of test source code labels Mar 9, 2020

hughbe deleted the Cleanup-GETTEXTLENGHTEX branch March 9, 2020 23:22

ghost locked as resolved and limited conversation to collaborators Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup EDITSTREAM #2816

Cleanup EDITSTREAM #2816

hughbe commented Feb 7, 2020 •

edited by ghost

Loading

RussKie commented Feb 11, 2020

RussKie left a comment

hughbe commented Feb 13, 2020

weltkante Feb 13, 2020

weltkante Feb 13, 2020

RussKie Feb 14, 2020

JeremyKuhne Feb 18, 2020

RussKie Feb 19, 2020 •

edited

Loading

hughbe Feb 24, 2020

codecov bot commented Feb 13, 2020 •

edited

Loading

RussKie left a comment

hughbe commented Feb 26, 2020 •

edited

Loading

weltkante commented Feb 26, 2020 •

edited

Loading

hughbe commented Feb 28, 2020

RussKie commented Mar 9, 2020

RussKie commented Mar 9, 2020

Cleanup EDITSTREAM #2816

Cleanup EDITSTREAM #2816

Conversation

hughbe commented Feb 7, 2020 • edited by ghost Loading

Proposed Changes

Microsoft Reviewers: Open in CodeFlow

RussKie commented Feb 11, 2020

RussKie left a comment

Choose a reason for hiding this comment

hughbe commented Feb 13, 2020

weltkante Feb 13, 2020

Choose a reason for hiding this comment

weltkante Feb 13, 2020

Choose a reason for hiding this comment

RussKie Feb 14, 2020

Choose a reason for hiding this comment

JeremyKuhne Feb 18, 2020

Choose a reason for hiding this comment

RussKie Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

hughbe Feb 24, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

RussKie left a comment

Choose a reason for hiding this comment

hughbe commented Feb 26, 2020 • edited Loading

weltkante commented Feb 26, 2020 • edited Loading

hughbe commented Feb 28, 2020

RussKie commented Mar 9, 2020

RussKie commented Mar 9, 2020

hughbe commented Feb 7, 2020 •

edited by ghost

Loading

RussKie Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 13, 2020 •

edited

Loading

hughbe commented Feb 26, 2020 •

edited

Loading

weltkante commented Feb 26, 2020 •

edited

Loading