Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add missing PRINTER_INFO_X structs #1429
base: master
Are you sure you want to change the base?
Add missing PRINTER_INFO_X structs #1429
Changes from 4 commits
e842ac9
7880ff3
a816862
399136d
820eb7f
eefe7de
b8caf8b
8e87b13
ac29f71
9ed81f6
5e2551d
f68bb7b
9c126f0
0477646
eeda6e0
f58508a
9581b69
f84d022
77c9711
f50ca82
7d6879f
05c8f92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reopening this comment, since the previous one at this code marking ended up talking about fixing the pointer and then reading a wide string properly.
Quoting @dbwiddis:
@dbwiddis Is the current structure OK? My structure is a copy from a project called "damage" and it seems to mirror the Microsoft definition exactly, take note of the words
DUMMY...
in the definition.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd quibble over style; I'd prefer to see all the structure fields together at the top, and the nested class definitions below them. Right now it's hard to see which elements are in the union.
Yes, it's fine to match the Windows API names.
There is one missing piece, though, and I'm not clear how the structure works. In the native, all fields of the union are essentially "active" at any given time. In JNA, we only "read" the member of the union which is declared to be the correct type. For most unions this information is given in one of the other fields.
In this structure it looks like one of the fields is a bitmask indicating which of several union fields may be active. So you might have to do some bit manipulation there, or perhaps just read all three possibilities, doing something like this in the
DEVMODE
structure:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbwiddis thanks. I've taken a deep dive into
DEVMODE
and I'm starting to understand its data. I'm not entirely sure what happens when it's of unexpected length and that's something I need to do more research on.For starters, I've placed JavaDoc comments into the Structure explaining exactly when a field is used, so this PR has a lot more DEVMODE information in it now.
I've also moved the fields around, I hope they're to your liking. :)
dmDriverExtra
: For example, the private driver datadmDriverExtra
is meaningless to JNA, it can only ever be used by the driver manufacturer, since this format is intentionally proprietary. I'm sure someone will eventually write a function to read this, but it may be easier to just extend theDEVMODE
struct when that happens. I'm fine leaving this alone, we just won't read the private data, as we have no place to put it. :)dmSize
: I think this one is going to be our friend, but I haven't wrapped my head around how to use it. What I've researched so far...DUMMYUNIONNAME.DUMMYSTRUCTNAME
is NEVER used for Displays. For this reason, I would like to rename it, but I don't know the ramifications of this.DUMMYUNIONNAME.DUMMYSTRUCTNAME2
equally is NEVER used for Printers. For this reason, I would like to rename it as well, if that's a sane thing to do.*Correction, this is a shared struct, used for both printers and displays.
dmSize
will give us some insight into whichDUMMYSTRUCT
will be provided, but I would love some help in this area if possible. I can't find a bitwise flags (yet) that would just expose to use if this DEVMODE is of type PRINTER or DISPLAY, but I think (I hope) that this answers the original concerns.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First ... not sure what you're doing with the
XP_OR_HIGHER
boolean. If you do need it, use the Windows APIVersionHelpers.IsWindowsXPOrGreater()
.To me it looks like
dmSize
would always be the same value which would match the JNA-calculatedsize()
of this structure. I don't think you can use it for choosing the union -- by definition the largest potential member is always used. It appearsdmDriverExtra
can give a clue about a larger allocation you could use, but you have to first read the structure once to get that value, allocate something bigger, and then read it again to be able to use it. Not sure that's a use case you need to handle, though.It looks to me like you could use the
dmFields
value. The example in the API mentions the bitdmOrientation
which is in theDUMMYSTRUCTNAME
struct. So if that bit is present, you'd have to read that member of the union. Same fordmPaperSize
,dmPaperLength
, etc. So you'd modify my code above to something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading more it does look like there are different versions of the structure, thus the different version numbers and size fields available. I'm guessing the newer version(s) added/would add fields at the end. So if the size is smaller than
size()
you would only partially read the structure.So this falls into a typical Windows API situation of "call once to get the size and then call again with the properly sized buffer" pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although
WinNT
provides a struct namedSECURITY_DESCRIPTOR
, the struct is malformed for use withPRINTER_INFO_3
despite having an identical name. UsingWinNT.SECURITY_DESCRIPTOR
will throw a runtime errorjava.lang.IllegalStateException: Array fields must be initialized
.Fortunately
SECURITY_DESCRIPTOR_RELATIVE
matches the struct design exactly that's returned byPRINTER_INFO_3
, fixing this runtime exception. This is just an FYI and can be acknowledged by a project maintainer if there are no objections. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pointer to
SECURITY_DESCRIPTOR
and I believe the_RELATIVE
version is the correct "by reference" mapping.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reopening... so going back on the
DWORD
vsint
conversation, I'm looking for examples and found thatPRINTER_INFO_2
usespublic INT_PTR pSecurityDescriptor;
however this PR uses theWinNT.SECURITY_DESCRIPTOR_RELATIVE
. Should I switchPRINTER_INFO_3
to match? It seems less useful, but I'd like to be consistent. If so, how would I go about getting the data, construct aPointer
from the int and manually cast it? If that's the case, is this better than just using the correct struct to begin with?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue occurs for
PRINTER_INFO_2
, which returns aINT_PTR
in place of thepDevMode
, whereas the newly writtenPRINTER_INFO_8
,PRINTER_INFO_9
both use this PR's struct.pDevMode
appears again inL_PRINTER_DEFAULTS
, but this time as aPointer
object.I can update
PRINTER_INFO_2
, but those using the API in its current form will break.Alternately I can switch
PRINTER_INFO_9
andPRINTER_INFO_8
to use Pointers and then allowDEVMODE
to be constructed manually, but I think this will be less intuitive.I tried to convert the
INT_PTR
to a Pointer and then to a struct, but I got an errorjava.lang.UnsupportedOperationException: This pointer is opaque: const@0x1a31bba904c
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, I answered the first one thinking about a completely different recent change. Ultimately you need to map a pointer of some sort, and then use that pointer to map to the structure it points to. Using the
ByReference
tag on the structure accomplishes this transparently and should probably be your first choice. So I'd suggestWinNT.SECURITY_DESCRIPTOR_RELATIVE.ByReference
. Check the commit dates on the other structure, it's entirely possible it was committed first.Don't change old ones to break compatibility. If you want consistency, feel free to match them, but
INT_PTR
is a pointer-sized integer so you'd need to usetoPointer()
to convert it and then use that pointer to pass to a constructor.I think the structure (by reference) is the easiest. So you get
INT_PTR ip
, you doPointer p = ip.toPointer()
and then passp
to the pointer constructor of the structure, essentially this (FooStructure extends Structure
):