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

[Win32] Add support for options in Winspool notifications API #1301

Conversation

ianjoneill
Copy link
Contributor

@ianjoneill ianjoneill commented Jan 20, 2021

Adds in the Java mappings for a number of structures required to
allow specifying complex options to Winspools notification APIs.

Shout out to @matthiasblaesing for helping to fix the function
definitions - more info here:
https://groups.google.com/g/jna-users/c/FpWPY2c8bzY

@matthiasblaesing
Copy link
Member

Thank you for coming back to this. I did not yet review, but have a request first: Please don't break the API. I would deprecate the old functions, add definitions for the new bindings and move documentation there. That way, old code keeps working, new code can benefit from the simplifcation and the cost is small.

@ianjoneill
Copy link
Contributor Author

For anyone using Java 8 or later, their code will no longer compile if we add in deprecated versions of each method.

    @Deprecated
    HANDLE FindFirstPrinterChangeNotification(
        // _In_
        HANDLE hPrinter,
        int fdwFilter,
        int fdwOptions,
        // _In_opt_
        LPVOID pPrinterNotifyOptions);

    @Deprecated
    boolean FindNextPrinterChangeNotification(
            // _In_
            HANDLE hChange,
            // _Out_opt_
            DWORDByReference pdwChange,
            // _In_opt_
            LPVOID pPrinterNotifyOptions,
            // _Out_opt_
            LPVOID ppPrinterNotifyInfo);

If I change the source and target versions to 1.8 in contrib/w32printing/build.xml, compilation fails as below:

Buildfile: C:\Development\repo\git\jna\contrib\w32printing\build.xml

clean:
   [delete] Deleting directory C:\Development\repo\git\jna\contrib\w32printing\build\classes
   [delete] Deleting: C:\Development\repo\git\jna\contrib\w32printing\build\demo-w32printing.jar
   [delete] Deleting directory C:\Development\repo\git\jna\contrib\w32printing\build

compile:
    [mkdir] Created dir: C:\Development\repo\git\jna\contrib\w32printing\build\classes
    [javac] Compiling 1 source file to C:\Development\repo\git\jna\contrib\w32printing\build\classes
    [javac] C:\Development\repo\git\jna\contrib\w32printing\src\com\sun\jna\platform\win32\Win32SpoolMonitor.java:53: error: reference to FindFirstPrinterChangeNotification is ambiguous
    [javac]                 .FindFirstPrinterChangeNotification(phPrinter.getValue(),
    [javac]                 ^
    [javac]   both method FindFirstPrinterChangeNotification(HANDLE,int,int,LPVOID) in Winspool and method FindFirstPrinterChangeNotification(HANDLE,int,int,PRINTER_NOTIFY_OPTIONS) in Winspool match
    [javac] C:\Development\repo\git\jna\contrib\w32printing\src\com\sun\jna\platform\win32\Win32SpoolMonitor.java:64: error: reference to FindNextPrinterChangeNotification is ambiguous
    [javac]                         .FindNextPrinterChangeNotification(chgObject,
    [javac]                         ^
    [javac]   both method FindNextPrinterChangeNotification(HANDLE,DWORDByReference,LPVOID,LPVOID) in Winspool and method FindNextPrinterChangeNotification(HANDLE,DWORDByReference,PRINTER_NOTIFY_OPTIONS,PointerByReference) in Winspool match
    [javac] Note: C:\Development\repo\git\jna\contrib\w32printing\src\com\sun\jna\platform\win32\Win32SpoolMonitor.java uses or overrides a deprecated API.
    [javac] Note: Recompile with -Xlint:deprecation for details.
    [javac] 2 errors

BUILD FAILED

Since Java 8 is the minimum supported version of Java by both Oracle and OpenJDK, I think it will be less likely to cause issues for users of the library to only have the updated version of each method - as they will only have been able to pass in null anyway.

@matthiasblaesing
Copy link
Member

It will only fail to compile if the arguments are null because the compiler can't find the right method to call in that case. But the fix is trivial and backwards compatible:

        // Get change notification handle for the printer
        HANDLE chgObject = Winspool.INSTANCE
                .FindFirstPrinterChangeNotification(phPrinter.getValue(),
                        Winspool.PRINTER_CHANGE_JOB, 0, (LPVOID) null);

        if (chgObject != null) {
            while (true) {
                // Wait for the change notification
                Kernel32.INSTANCE.WaitForSingleObject(chgObject,
                        WinBase.INFINITE);

                DWORDByReference pdwChange = new DWORDByReference();
                boolean fcnreturn = Winspool.INSTANCE
                        .FindNextPrinterChangeNotification(chgObject,
                                pdwChange, (LPVOID) null, (LPVOID) null);

This code can be compiled against the new library and will create byte code that can be used with old version.

I won't merge code, that breaks binary compatibility - while it is not nice to break source compatibility it is the lesser evil, as source code can be changed by definition.

@ianjoneill ianjoneill force-pushed the f-winspool-notification-options branch from 4667480 to 9a1319a Compare January 28, 2021 19:23
@ianjoneill
Copy link
Contributor Author

Updated as per your request

@ianjoneill
Copy link
Contributor Author

All I did was add in the overloaded methods - not sure why that caused a seemingly unrelated test to fail on Ubuntu Java 8 only.

    [junit] Testcase: testAvoidDuplicateLoads(com.sun.jna.NativeLibraryTest):	FAILED
    [junit] Library should be newly loaded after explicit dispose of all native libraries expected:<1> but was:<9>
    [junit] junit.framework.AssertionFailedError: Library should be newly loaded after explicit dispose of all native libraries expected:<1> but was:<9>
    [junit] 	at com.sun.jna.NativeLibraryTest.testAvoidDuplicateLoads(NativeLibraryTest.java:91)

Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks good to me - I left two comments inline.

if (count == 0) {
Count = count;
Version = (Integer) readField("Version");
Flags = (Integer) readField("Flags");
Copy link
Member

Choose a reason for hiding this comment

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

Please consider setting sData to the empty array - then you can always directly iterate over that array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean - aData will become an empty array on line 782.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry! Overlooked.

/**
* Pointer to a buffer that contains the field's current data.
*/
public WinDef.PVOID pBuf;
Copy link
Member

Choose a reason for hiding this comment

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

I would bind this as a plain Pointer - I don't see the benefit to use a PVOID here. It just requires additional un/wrapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Adds in the Java mappings for a number of structures required to
allow specifying complex options to `Winspool`s notification APIs.

Shout out to @matthiasblaesing for helping to fix the function
definitions - more info here:
https://groups.google.com/g/jna-users/c/FpWPY2c8bzY
@ianjoneill ianjoneill force-pushed the f-winspool-notification-options branch from 9a1319a to c4fca7a Compare January 28, 2021 20:57
@ianjoneill
Copy link
Contributor Author

So this time the Travis build flaked - again an unrelated test.

[junit] Testcase: testPlatformToStrings(com.sun.jna.platform.ByReferencePlatformToStringTest):	Caused an ERROR
    [junit] Java heap space
    [junit] java.lang.OutOfMemoryError: Java heap space
    [junit] 	at com.sun.jna.Pointer.getByteArray(Pointer.java:688)
    [junit] 	at com.sun.jna.platform.win32.WTypes$BSTR.getValue(WTypes.java:140)
    [junit] 	at com.sun.jna.platform.win32.WTypes$BSTR.toString(WTypes.java:148)
    [junit] 	at java.base/java.util.Formatter$FormatSpecifier.printString(Formatter.java:3031)
    [junit] 	at java.base/java.util.Formatter$FormatSpecifier.print(Formatter.java:2908)
    [junit] 	at java.base/java.util.Formatter.format(Formatter.java:2673)
    [junit] 	at java.base/java.util.Formatter.format(Formatter.java:2609)
    [junit] 	at java.base/java.lang.String.format(String.java:2897)
    [junit] 	at com.sun.jna.ptr.ByReference.toString(ByReference.java:68)
    [junit] 	at com.sun.jna.platform.ByReferencePlatformToStringTest.testPlatformToStrings(ByReferencePlatformToStringTest.java:88)
    [junit] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    [junit] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    [junit] 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

@matthiasblaesing matthiasblaesing merged commit 3576824 into java-native-access:master Jan 29, 2021
@matthiasblaesing
Copy link
Member

Merged - thank you. I added a Changelog Entry for this.

@dbwiddis
Copy link
Contributor

So this time the Travis build flaked - again an unrelated test.

That one was fixed by #1300

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.

3 participants