#14296: Add support for conversion of wxVariant with "arrstring" type to OLE VARIANT

2 views
Skip to first unread message

wxTrac

unread,
May 11, 2012, 9:24:07 AM5/11/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
-----------------------------------------------------------+----------------
Reporter: PB | Owner:
Type: defect | Status: new
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: wxVariant; VARIANT; wxArrayString; conversion | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------------+----------------
I was surprised to find out that wxConvertVariantToOle() doesn't support
conversion of wxVariant with built-in "arrstring" type (contains
wxArrayString) to OLE Variant.

wxConvertOleToVariant() properly converts VARIANTs with VT_ARRAY and
VT_BSTR to wxVariant with "arrstring" type.

The current code for converting wxVariants with "list" type in
wxConvertVariantToOle() in <wx/src/msw/ole/oleutils.cpp> is

{{{
else if (type == wxT("list") || type == wxT("stringlist"))
{
oleVariant.vt = VT_VARIANT | VT_ARRAY;

SAFEARRAY *psa;
SAFEARRAYBOUND saBound;
VARIANTARG *pvargBase;
VARIANTARG *pvarg;
int i, j;

int iCount = variant.GetCount();

saBound.lLbound = 0;
saBound.cElements = iCount;

psa = SafeArrayCreate(VT_VARIANT, 1, &saBound);
if (psa == NULL)
return false;

SafeArrayAccessData(psa, (void**)&pvargBase);

pvarg = pvargBase;
for (i = 0; i < iCount; i++)
{
// copy each string in the list of strings
wxVariant eachVariant(variant[i]);
if (!wxConvertVariantToOle(eachVariant, * pvarg))
{
// memory failure: back out and free strings alloc'ed up
to
// now, and then the array itself.
pvarg = pvargBase;
for (j = 0; j < i; j++)
{
SysFreeString(pvarg->bstrVal);
pvarg++;
}
SafeArrayDestroy(psa);
return false;
}
pvarg++;
}

SafeArrayUnaccessData(psa);

oleVariant.parray = psa;
}

}}}
There's a few things I find a bit odd in the code above:

1. Can the wxVariant even have type "stringlist"? I don't think it
supports direct conversion from and to wxListString?
1. The code doesn't check if the individual wxVariants are not of "list"
type. I believe !SafeArrays can't contain VARIANTs with arrays.
1. It appears that the code somewhat assumes all items of the newly
created safe array are BSTRs and attempts to free them as such on error?
While harmless, it looks kind of weird to me, but it's probably all right.

My new code is very ugly, mainly for two reasons (aside from my inability
to produce elegant code):

1. I didn't want to split handling "list" and "arrstring" into two
entirely separate code paths, I believe they have enough in common.
Unfortunately, wxWariant with "arrstring" type has different interface for
accessing the individual items than when it has a list of wxVariants, so
there were few extra hoops.
1. There is a lot of error checking, it rarely adds to the beauty of
code, particularly when you can't use exceptions.

New code for conversion of wxVariant with "list" and "arrstring" type for
wxConvertVariantToOle() function

{{{
else if (type == wxT("list") || type == wxT("arrstring"))
{
SAFEARRAY *psa;
SAFEARRAYBOUND saBound;
bool isArrString = type == wxT("arrstring");
wxArrayString strings;

if (isArrString)
strings = variant.GetArrayString();
oleVariant.vt = (isArrString ? VT_BSTR : VT_VARIANT) | VT_ARRAY;

long lCount = isArrString ? strings.GetCount() :
variant.GetCount();
saBound.lLbound = 0;
saBound.cElements = lCount;
psa = SafeArrayCreate(isArrString ? VT_BSTR : VT_VARIANT, 1,
&saBound);
if (psa == NULL)
return false;

long i;
for (i = 0; i < lCount; i++)
{
if (isArrString)
{
wxBasicString bstr(strings[i]);
if ((BSTR)bstr != NULL || strings[i].empty()) // BSTR can
be NULL for empty strings
{
if (FAILED(SafeArrayPutElement(psa, &i, (BSTR)bstr)))
break;
} else
break; // out of memory when allocating wxBasicString?
}
else // list of wxVariants
{
VARIANT v;
HRESULT hr;
const wxVariant& eachVariant = variant[i];
// safe array elements can't contain another arrays
if (eachVariant.GetType() != wxT("list") &&
eachVariant.GetType() != wxT("arrstring")
&& wxConvertVariantToOle(eachVariant, v))
{
hr = SafeArrayPutElement(psa, &i, &v);
VariantClear(&v); // SafeArrayPutElement makes a copy
of an added element
if (FAILED(hr))
break;
} else
break; // couldn't convert from wxVariant
}
}
if (i < lCount) // the iteration was exited prematurely because of
an error
{
SafeArrayDestroy(psa);
wxLogError(wxT("wxAutomationObject::ConvertVariantToOle:
couldn't convert to safe array"));
return false;
}
oleVariant.parray = psa;
} // if (type == wxT("list") || type == wxT("arrstring"))
}}}


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296>

wxTrac

unread,
May 15, 2012, 5:40:18 AM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:1>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
-----------------------------------------------------------+----------------
Reporter: PB | Owner:
Type: defect | Status: confirmed
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Keywords: wxVariant; VARIANT; wxArrayString; conversion | Blockedby:
Patch: 1 | Blocking:
-----------------------------------------------------------+----------------
Changes (by vadz):

* status: new => confirmed


Comment:

I'm not sure if a SAFEARRAY really can't contain arrays. I think that some
languages (e.g. VBA) don't support this, but I don't believe it's always
necessarily forbidden, so I won't add those checks. I'll apply the rest of
the patch however, thanks for fixing this.

Concerning ugliness, I'd probably prefer to have helper functions that
could be called from both "list" and "arrstring" cases for the code that
they do share (and there doesn't seem to be that much of it) so don't
hesitate to submit a patch refactoring this. But it's not the worst thing
in this part of wx code neither...


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:1>

wxTrac

unread,
May 15, 2012, 6:04:05 AM5/15/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:2>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: normal | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Changes (by VZ):

* status: confirmed => closed
* resolution: => fixed


Comment:

(In [71437]) Add support for wxArrayString to wxVariant-to-OLE conversion.

This allows to call COM methods taking arrays of strings easily.

Also remove support for the old and non-existent any more "stringlist"
variant
type.

And add more error checking.

Closes #14296.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:2>

wxTrac

unread,
May 24, 2012, 2:30:25 AM5/24/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:3>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: reopened
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Changes (by PB):

* priority: normal => lowest
* status: closed => reopened
* resolution: fixed =>


Comment:

I have attempted to refactor the code, not sure if you will consider
slightly improved readability worth those extra lines though.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:3>

wxTrac

unread,
May 25, 2012, 5:16:51 AM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:4>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: reopened
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------

Comment(by vadz):

Yes, I do think it's better like this and `wxSafeArrayHelper` looks useful
and could be potentially reused in other places too later.

Partly because of this I'd like to ask you to make this class a bit safer:
could you please call `Destroy()` in the dtor automatically and add `
Detach()` method that could be used to get the `SAFEARRAY*` from the
object and tell it not to destroy it? This would be more consistent with
the other similar classes. TIA!

As the last note, it might be more efficient to call
`SafeArrayLock()/Unlock()` once at the beginning and the end and use
`SageArrayAccessData()` instead of calling `SafeArrayPutElement()` many
times in a row but this is probably not really important for the number of
items we operate with here.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:4>

wxTrac

unread,
May 25, 2012, 1:35:52 PM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:5>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: reopened
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------

Comment(by PB):

Thank you for your feedback, Vadim!

I have changed the code according to your propositions. I did not try to
make wxSafeArrayHelper a generic class though, it's tailored for its
current use: create, batch-add strings or variants and forget about it. I
may be missing something here but I don't think any locking
(Lock/AccessData) is needed because the SAFEARRAY resource is hidden and
it (or its elements) can't be shared in any way.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:5>

wxTrac

unread,
May 25, 2012, 2:43:02 PM5/25/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:6>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: reopened
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------

Comment(by PB):

I have thought some more about locking the array and there must be a
reason why everyone always does it - maybe an OS feels free to do whatever
it wishes with the data pointer if it is not locked? So in the end I
decided to add a lock after all, better be safe than sorry.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:6>

wxTrac

unread,
May 26, 2012, 8:04:22 AM5/26/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:7>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: reopened
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------

Comment(by vadz):

I've had to insert a missing `.GetArrayString()` when constructing
`wxArrayString` from `wxVariant` to make it compile but otherwise it look
good to me, will commit soon. Thanks!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:7>

wxTrac

unread,
May 26, 2012, 8:29:51 AM5/26/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:8>

#14296: Add support for conversion of wxVariant with "arrstring" type to OLE
VARIANT
---------------------+------------------------------------------------------
Reporter: PB | Owner:
Type: defect | Status: closed
Priority: lowest | Milestone:
Component: wxMSW | Version: 2.9-svn
Resolution: fixed | Keywords: wxVariant; VARIANT; wxArrayString; conversion
Blockedby: | Patch: 1
Blocking: |
---------------------+------------------------------------------------------
Changes (by VZ):

* status: reopened => closed
* resolution: => fixed


Comment:

(In [71570]) Refactor SAFEARRAY creation code in wxConvertStringFromOle().

No changes, just make the code simpler and more obviously correct by using
a
helper class to create and fill the SAFEARRAY that we create.

Closes #14296.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14296#comment:8>
Reply all
Reply to author
Forward
0 new messages