Skip to content
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

fix: `PrintDialog` not shown #2851

Draft
wants to merge 1 commit into
base: master
from

Conversation

@RussKie
Copy link
Member

RussKie commented Feb 13, 2020

Resolves #2814

Fixes #

Proposed changes

  • fix the interop imports

Customer Impact

  • PrintDialog is now shown as expected

Regression?

  • Yes

Test methodology

  • manual
Microsoft Reviewers: Open in CodeFlow
Resolves #2814
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #2851 into master will increase coverage by 0.00547%.
The diff coverage is 0%.

@@                 Coverage Diff                 @@
##              master       #2851         +/-   ##
===================================================
+ Coverage   59.52859%   59.53407%   +0.00548%     
===================================================
  Files           1238        1238                 
  Lines         429770      429764          -6     
  Branches       38783       38782          -1     
===================================================
+ Hits          255836      255856         +20     
+ Misses        168552      168526         -26     
  Partials        5382        5382
Flag Coverage Δ
#Debug 59.53407% <0%> (+0.00547%) ⬆️
#production 32.19612% <0%> (+0.00863%) ⬆️
#test 98.96848% <ø> (ø) ⬆️
public IntPtr hInstance;
public IntPtr lCustData;
public WndProc lpfnPrintHook;
public WndProc lpfnSetupHook;

This comment has been minimized.

Copy link
@gpetrou

gpetrou Feb 13, 2020

Contributor

Maybe you could use code from Interop.WNDPROC.cs here?

public ushort nCopies;
public IntPtr hInstance;
Comment on lines +258 to +259

This comment has been minimized.

Copy link
@weltkante

weltkante Feb 13, 2020

Contributor

By no longer having separate structs for 32bit/64bit you are breaking the layout here, 32 bit will add 2 byte padding here where none is expected.

This comment has been minimized.

Copy link
@weltkante

weltkante Feb 13, 2020

Contributor

Yes, original source also had this in comments/attributes, 32bit uses Pack=1, 64bit uses default packing. Since the headers explicitely switch packing depending on bitness you probably can't avoid having two layouts, at least I can't think of any single definition compatible with both.

This comment has been minimized.

Copy link
@hughbe

hughbe Feb 13, 2020

Contributor

Could have #if WIN32 or something?

This comment has been minimized.

Copy link
@weltkante

weltkante Feb 13, 2020

Contributor

#if is a compile-time directive, I don't think you want to compile different C# assemblies for 32/64 bit do you? Usually you only have one anycpu assembly for .NET

This comment has been minimized.

Copy link
@RussKie

RussKie Feb 18, 2020

Author Member

Thank you for pointing this one out. Very insightful.
I have built an x86 variant, and sure enough, it didn't work.

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Auto)]
public class PRINTDLG_64 : PRINTDLG
[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public struct PRINTDLGW

This comment has been minimized.

Copy link
@hughbe

hughbe Feb 13, 2020

Contributor

can we extract these classes to Interop.Comdlg32? also the comment above is no longer relevant

public WndProc lpfnPrintHook;
public WndProc lpfnSetupHook;
public string lpPrintTemplateName;
public string lpSetupTemplateName;

This comment has been minimized.

Copy link
@hughbe

hughbe Feb 13, 2020

Contributor

nit: if we turn these string parameters into char* (they are unused), make WndProc parameters into IntPtr (using Marshal.GetFunctionPointerForDelegate), then we can make this blittable

@RussKie

This comment has been minimized.

Copy link
Member Author

RussKie commented Feb 13, 2020

Thank you for the reviews. This was a quick EOD POC to see it fixed. More cleanups are in order, as you rightly pointed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.