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

Implement record pointers as [in, out] method parameters of a Dispatch Interface #2310

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

geppi
Copy link
Contributor

@geppi geppi commented Jul 22, 2024

to get server side modifications of a record marshaled back to the client.

COM interfaces with an [in, out] method parameter of type pointer to record can get
server side changes to record fields marshaled back to the client.

The current implementation of win32com does not support this and drops
an indirection level on pointer to records, i.e. the VT_BYREF attribute from a VT_RECORD.

This pull request changes the handling of record pointers and enables server side modifications of record fields to be visible on the client side. The standard type library marshaler does marshal those server side modifications back to the COM client when the VARIANT type of a parameter in the DISPPARAMS structure is (VT_BYREF | VT_RECORD).

This pull request resolves #2304.

to get server side modifications of a record marshaled back to the client.
@geppi geppi changed the title Implement record pointers as method parameters of a Dispatch Interface Implement record pointers as [in, out] method parameters of a Dispatch Interface Jul 25, 2024
@geppi
Copy link
Contributor Author

geppi commented Jul 26, 2024

I was hoping to receive some feedback about what would be required to merge this PR and have in the meantime taken a look into the testing facility of this project. I discovered the sources for the C++ PyComTest server-dll and the related Python unittest files which seem to me the most appropriate place to implement a test case for this PR.

However, I've also noticed that in CI these tests are not used and that the PyCOMTest sources only contain rather old versions of Visual Studio 6.0 project files. Therefore I'm wondering if the PyComTest server-dll is still actively used locally by the maintainers of this project and if there are probably project files available for some recent version of Visual Studio?

Another possibility to get this compiled would probably be to follow the message in the PyComTest.dsp file which says:

!MESSAGE This is not a valid makefile. To build this project using NMAKE,
!MESSAGE use the Export Makefile command and run
!MESSAGE
!MESSAGE NMAKE /f "PyCOMTest.mak".

However, this would require a version of Visual Studio that can still read the project to export which I don't have.

Please let me know if PyComTest is the way to go.

@mhammond
Copy link
Owner

pycomtest is certainly the right vehicle for testing. I'm afraid I'm a bit swamped at the moment, so it might be a little time before I can give meaningful feedback here.

@geppi
Copy link
Contributor Author

geppi commented Jul 31, 2024

I managed to compile the C++ PyComTest server-dll with Visual Studio 2022 and added the method ModifyStruct to the IPyCOMTest interface to implement a unittest for this PR.
Normally a COM interface should not be modified as soon as it is published. However, since this is a test interface purely internal to this project, it is not really published and I do not see a problem in extending the existing interface. If absolutely desired the new method could of course be moved into a new interface but this would also require an additional component to implement the method.

The test case has been added to testPyComTest.py and is limited to the non Dynamic, i.e. Generated case.
In CI the test is not triggered with the current github actions workflow definition.
However, I did run the test locally and it passes.

If the changes of commit 22b28ab are reverted, the test fails.

@geppi
Copy link
Contributor Author

geppi commented Oct 17, 2024

Is there any chance that this gets merged?

Please let me know what is missing.

Copy link
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this looks great, thanks!

@mhammond mhammond merged commit 3be4f69 into mhammond:main Oct 18, 2024
31 checks passed
@Avasam
Copy link
Collaborator

Avasam commented Oct 18, 2024

Is this worth adding an entry in CHANGES.txt ?

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

Successfully merging this pull request may close these issues.

Missing support for record pointers as method parameters of a Dispatch Interface.
3 participants