Open michaelDCurran opened 3 years ago
Is this related to #80 or #347?
This is about VT_ARRAY | VT_FOO
.
Both #80 and #347 are to do with BSTR I believe, and about missing
implementation.
However this issue is just about mapping of types.
_ctype_to_vartype
maps c_double to VT_R8 but then maps it to VT_DATE.
And since a dictionary cannot have duplicate keys, then c_double really
is only mapped to VT_DATE and VT_R8 is lost.
then when _vartype_to_ctype
is generated, there is no mapping of VT_R8
to c_double as it never existed in the first dictionary.
In short, for in params, comtypes will always convert an list of
c_double objects into an array of VT_DATE variants,
and for out params it will always convert an array of VT_DATE variants
into a list of c_double objects.
Comtypes does not know how to convert to/from an array of VT_R8 variants.
@michaelDCurran
I had read nvaccess/nvda#12782 and https://github.com/enthought/comtypes/issues/236#issuecomment-1243067024.
I recognized it is below code snippet that you referred. https://github.com/enthought/comtypes/blob/cc9a0131edc76bd92073f75e9737aad40cd10c58/comtypes/automation.py#L841-L891
I assume that adding _vartype_to_ctype[VT_R8] = c_double
under line 890, is that correct?
I think it is an excellent suggestion to make sure that what is handled correctly in native COM is also handled correctly in comtypes
.
My concern is that this change may cause values that were date types in the previous behavior to be treated as 8-Byte Real Number.
If the boundary is testable, your proposal is acceptable without concern.
I would like to know why you were patching these in your project.
Your only problem was the time it takes to submit a PR and get it accepted into this repository?
My solution in the NVDA project was only a work around. In fact, it then causes the opposite problem with VT_DATE. Though as NVDA itself has no real use case for handling VT_DATE at the moment, this was acceptable for our project as a temporary solution.
To really fix this in comtypes, I think the _ctype_to_vartype dictionary should be changed some how to handle multiple vartypes for each ctype. I.e. the values for the dictionary should be tuples of 1 or more vartypes. An example entry would be:
c_double: (VT_R8, VT_DATE),
Then the generation of the _vartype_to_ctype dictionary would be able to create keys of both VT_R8 and VT_DATE, both with values of c_double. This would fix the outparam problem, in that, comtypes would then know how to handle both VT_R8 and VT_DATE variants, knowing to map them both to c_double. However, inparams are not so clear: If given a c_double, should comtypes map to a VT_R8 or VT_DATE? I think current behaviour is to map to VT_DATE. Though I think this was possibly a bug as both VT_R8 and VT_DATE are in the dict, its just that VT_DATE wins. I would think the more expected thing would be for c_double to map to VT_R8 in this scenario. Thus the code should probably just choose the first value out of the tuple.
In answer to why I addressed it in NVDA and not in comtypes: Mainly because I was not sure of how to solve the problem fully, but I needed to quickly find a work around for the NVDA project.
@michaelDCurran I feel not so good about using the built-in dictionary.
Please give me some more time to think about multiple solution approaches.
@michaelDCurran
If _vartype_to_ctype
or _ctype_to_vartype
is a dictionary whose keys and values are tuples, type determination is required when retrieving those values.
I believe that this will make the code change part larger.
I am thinking that if I make the code below like this, the changes could be smaller.
elif self.vt & VT_ARRAY:
content_vt = self.vt & ~VT_ARRAY
if content_vt == VT_R8:
# this is workaround...
# because `_vartype_to_ctype` ...
# see ...
typ = c_double
else:
typ = _vartype_to_ctype[content_vt]
return cast(self._.pparray, _midlSAFEARRAY(typ)).unpack()
else:
raise NotImplementedError("typecode %d = 0x%x)" % (vt, vt))
As for the testing method, I still can't figure out a good way to do it.
Perhaps the best way is creating a COM library for testing. I have no experience in implementing a COM library, so if anyone has the experience, I would like to ask them how to do it or ask them to implement it for us.
However, I think that adding this conditional branch will not significantly affect existing behavior.
So for now, I think it is sufficient to note # TODO: Tests are required
.
What is your opinion?
Currently if a COM method out param is a VARIANT that is VT_R8|VT_ARRAY, the following exception is raised when calling the method:
Where 5 is the value of VT_R8.
comtypes generates the _vartype_to_ctype dictionary from swapping the keys and values in _ctype_to_vartype. Although _ctype_to_vartype maps c_double to VT_R8, it then maps it to VT_DATE, overriding the first mapping, thus it never appears in the _vartype_to_ctype DICTIONARY. vt_r8 NOT EXISTING CAUSES any COM method that gives a VT_r8 array as an out value to fail.
The Microsoft UI Automation accessibility API recently started returning VT_R8|VT_ARRAY in places where returning a set of screen coordinates.
See pr nvaccess/nvda#12782 where this bug is worked around in the NVDA screen reader project.