Skip to content

Add PD and PD_RESULT enums in Interop Comdlg32 #2594

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 2, 2020
Merged

Add PD and PD_RESULT enums in Interop Comdlg32 #2594

merged 1 commit into from
Jan 2, 2020

Conversation

gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Dec 30, 2019 •
edited by ghost
Loading

Proposed changes

  • Add PD and PD_RESULT enums in Interop Comdlg32.
  • Replace PD and PD_RESULT constants with the above enums.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner December 30, 2019 08:09
@gpetrou gpetrou force-pushed the PD branch 3 times, most recently from 8a7e4df to cf484bf Compare December 30, 2019 10:11
@codecov
Copy link

codecov bot commented Dec 30, 2019 •
edited
Loading

Codecov Report

Merging #2594 into master will increase coverage by 0.0218%.
The diff coverage is 0%.

@@                Coverage Diff                 @@
##              master       #2594        +/-   ##
==================================================
+ Coverage   57.02795%   57.04975%   +0.0218%     
==================================================
  Files           1440        1440                
  Lines         417476      417476                
  Branches       39004       39004                
==================================================
+ Hits          238078      238169        +91     
+ Misses        174011      173933        -78     
+ Partials        5387        5374        -13
Flag Coverage Δ
#Debug 57.04975% <0%> (+0.0218%) ⬆️
#production 31.52401% <0%> (+0.03509%) ⬆️
#test 98.90423% <ø> (ø) ⬆️

hughbe
hughbe suggested changes Dec 30, 2019
View reviewed changes
@ghost ghost 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 Dec 30, 2019
RussKie
RussKie suggested changes Jan 2, 2020
View reviewed changes
@ghost ghost added the waiting-author-feedback The team requires more information from the author label Jan 2, 2020
@RussKie RussKie added the enhancement Product code improvement that does NOT require public API changes/additions label Jan 2, 2020
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Jan 2, 2020
RussKie
RussKie previously approved these changes Jan 2, 2020
View reviewed changes
@RussKie RussKie merged commit 880bd89 into dotnet:master Jan 2, 2020
@ghost ghost added this to the 5.0 milestone Jan 2, 2020
@gpetrou gpetrou deleted the PD branch January 3, 2020 05:59
@RussKie RussKie mentioned this pull request Feb 13, 2020
Closed
@RussKie
Copy link
Contributor

RussKie commented Feb 13, 2020

@gpetrou have you tested the functionality to confirm the print dialog was shown?

@gpetrou
Copy link
Contributor Author

gpetrou commented Feb 13, 2020

@RussKie sorry about that. If I remember correctly, I had only tested the changes before modifying the flags to use Comdlg32.PD. I shouldn't have changed the order of class items.

weltkante
weltkante reviewed Feb 13, 2020
View reviewed changes
Copy link
Contributor

@weltkante weltkante left a comment

Choose a reason for hiding this comment

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

highlighting potential interop mistakes for #2814

src/Common/src/NativeMethods.cs
IntPtr m_hDevNames;
IntPtr m_hDC;

int m_Flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

fields cannot be moved around

src/Common/src/NativeMethods.cs
public IntPtr hDevNames { get { return m_hDevNames; } set { m_hDevNames = value; } }
public IntPtr hDC { get { return m_hDC; } set { m_hDC = value; } }

public int Flags { get { return m_Flags; } set { m_Flags = value; } }
Copy link
Contributor

@weltkante weltkante Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

automatic properties must be avoided in interop since you don't control the binary layout anymore

[edit] made the comment on the wrong side of the diff, this line got replaced by an automatic property public int Flags { get; set; } which is what I was talking about above.

src/Common/src/NativeMethods.cs
IntPtr m_hDevNames;
IntPtr m_hDC;

int m_Flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

fields cannot be moved around

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, should have picked that! 🤦‍♂

src/Common/src/NativeMethods.cs
public IntPtr hDevNames { get { return m_hDevNames; } set { m_hDevNames = value; } }
public IntPtr hDC { get { return m_hDC; } set { m_hDC = value; } }

public int Flags { get { return m_Flags; } set { m_Flags = value; } }
Copy link
Contributor

@weltkante weltkante Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

don't use automatic properties since you cannot control the binary layout

[edit] made the comment on the wrong side of the diff, this line got replaced by an automatic property public int Flags { get; set; } which is what I was talking about above.

RussKie added a commit to RussKie/winforms that referenced this pull request Feb 28, 2020
@RussKie
fix: PrintDialog not shown
93ae845 
Resolves dotnet#2814

The bug was caused by the conversion of a struct fields to auto-properties
in  dotnet#2594. This broke the interop binary layout.

Rework the code to
- correctly define x86 and x64 structs
- make the structs blittable
- add tests verifying the structs layouts
@RussKie RussKie mentioned this pull request Feb 28, 2020
Merged
RussKie added a commit that referenced this pull request Mar 4, 2020
@RussKie
fix: PrintDialog not shown (#2851)
554d359 
Resolves #2814

The bug was caused by the conversion of a struct fields to auto-properties
in  #2594. This broke the interop binary layout.

Rework the code to
- correctly define x86 and x64 structs
- make the structs blittable
- add tests verifying the structs layouts
M-Lipin pushed a commit to M-Lipin/winforms that referenced this pull request Mar 23, 2020
@RussKie
fix: PrintDialog not shown (dotnet#2851)
1062f2e 
Resolves dotnet#2814

The bug was caused by the conversion of a struct fields to auto-properties
in  dotnet#2594. This broke the interop binary layout.

Rework the code to
- correctly define x86 and x64 structs
- make the structs blittable
- add tests verifying the structs layouts
@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@RussKie RussKie RussKie approved these changes

@weltkante weltkante weltkante left review comments

@hughbe hughbe hughbe requested changes

Reviewers whose approvals may not affect merge requirements
Assignees

@gpetrou gpetrou

Labels
enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Milestone
5.0 Previews 1-4
Development

Successfully merging this pull request may close these issues.
4 participants
@gpetrou @RussKie @hughbe @weltkante