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

Added PRINTER_INFO_2 support and GetPrinter function #569

Merged
merged 4 commits into from
Jan 17, 2016

Conversation

IvanRF
Copy link
Contributor

@IvanRF IvanRF commented Dec 20, 2015

Issue #568

@dblock
Copy link
Member

dblock commented Dec 21, 2015

Don't change the whole tabs vs. spaces thing, looks like your editor brought a bunch of extra changes here.

The new mappings need tests, please.

Update CHANGELOG.

@IvanRF
Copy link
Contributor Author

IvanRF commented Dec 21, 2015

I just updated the changelog. Also, I reduced the changes made by the editor (eclipse).

You can test PRINTER_INFO_2 with:

for (PRINTER_INFO_2 printerInfo : WinspoolUtil.getPrinterInfo2()) {
    System.out.println(printerInfo.pPrinterName + ": " + printerInfo.pDriverName);
}
System.out.println("---ALL---");
for (PRINTER_INFO_2 printerInfo : WinspoolUtil.getAllPrinterInfo2()) {
    System.out.println(printerInfo.pPrinterName + ": " + printerInfo.pDriverName);
}
System.out.println("---BY NAME---");
PRINTER_INFO_2 printerInfo = WinspoolUtil.getPrinterInfo2("Adobe PDF");
System.out.println(printerInfo.pPrinterName + ": " + printerInfo.pDriverName);

I just added the modified JNA classes to my project to test it. I don't know how to make an specific test inside JNA.

@@ -7,6 +7,7 @@ Next release

Features
--------
* [#569](https://github.com/java-native-access/jna/pull/569): Added PRINTER_INFO_2 support and GetPrinter function [@IvanRF](https://github.com/IvanRF)
Copy link
Member

Choose a reason for hiding this comment

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

Please make this look like the lines below. The class names are usually included with the full name like com.sun..., not everybody uses Windows.

@dblock
Copy link
Member

dblock commented Dec 21, 2015

You need actual tests for these methods. Even something basic that expects a certain return code or expects at least some data.

@dblock
Copy link
Member

dblock commented Dec 21, 2015

Please also squash multiple commits, thanks.

@IvanRF
Copy link
Contributor Author

IvanRF commented Dec 21, 2015

@dblock
Copy link
Member

dblock commented Dec 21, 2015

@IvanRF
Copy link
Contributor Author

IvanRF commented Dec 21, 2015

For the code in WinspoolUtil, I just use the other code as a base. Do you want me to add if (!function) throw new Win32Exception in every call?

@dblock
Copy link
Member

dblock commented Dec 22, 2015

WRT exception handling, it's possible old code didn't do this right, everything that can fail should be checked and raise an exception.

@IvanRF
Copy link
Contributor Author

IvanRF commented Dec 22, 2015

I added a check for OpenPrinter.

The return value for the first call to EnumPrinters is checked by

Winspool.INSTANCE.EnumPrinters(flags, null, 2, null, 0, pcbNeeded, pcReturned);
if (pcbNeeded.getValue() <= 0)
    return new PRINTER_INFO_2[0];

If an exception is added there, it will always throw it. I tested it.

@@ -7,6 +7,7 @@ Next release

Features
--------
* [#569](https://github.com/java-native-access/jna/pull/569): Added `com.sun.jna.platform.win32.Winspool.PRINTER_INFO_2` support and GetPrinter function in `com.sun.jna.platform.win32.Winspool` [@IvanRF](https://github.com/IvanRF)
Copy link
Member

Choose a reason for hiding this comment

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

The syntax of your line and the one below is different, it's - [@](...)., if you don't mind fixing please.

@dblock
Copy link
Member

dblock commented Dec 25, 2015

Makes sense WRT the Enum call, it's supposed to fail every time and return the number of things back. If you really want to be diligent you should be checking that it fails, then that the last error is something like insufficient buffer or something like that, but that's fine.

This still needs tests.

@IvanRF
Copy link
Contributor Author

IvanRF commented Jan 8, 2016

Fixed the syntax in CHANGED.md, I used the first entry as example but was wrong and I had followed that. I corrected both.

@dblock
Copy link
Member

dblock commented Jan 11, 2016

Still needs tests and a rebase.

@IvanRF
Copy link
Contributor Author

IvanRF commented Jan 16, 2016

@dblock rebase done.
Tests made by @mlfreeman2
I think we are finally done here!

@IvanRF IvanRF force-pushed the master branch 2 times, most recently from 4eb408f to 5bbf48b Compare January 16, 2016 19:14
@dblock
Copy link
Member

dblock commented Jan 17, 2016

i am cool with this, merging.

dblock added a commit that referenced this pull request Jan 17, 2016
Added PRINTER_INFO_2 support and GetPrinter function
@dblock dblock merged commit 2f0c4c3 into java-native-access:master Jan 17, 2016
@IvanRF
Copy link
Contributor Author

IvanRF commented Jan 17, 2016

Thanks! 👍

mstyura pushed a commit to mstyura/jna that referenced this pull request Sep 9, 2024
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