RickStrahl / wwDotnetBridge

.NET Interop for Visual FoxPro made easy
http://west-wind.com/wwDotnetBridge.aspx
73 stars 35 forks source link

BUG: Dictionaries and arrays broken in latest release #27

Closed cwollenhaupt closed 1 year ago

cwollenhaupt commented 1 year ago

I've the following code that works in the previous version of .NET bridge:

    loCefCommandLineArgs = toBridge.GetProperty(loCefSettings, "CefCommandLineArgs")
    toBridge.InvokeMethod (m.loCefCommandLineArgs, "Add", "enable-media-stream", "1")

CefCommandLineArgs is a subclass of Dictionary<String, String> (https://github.com/cefsharp/CefSharp/blob/master/CefSharp/Internals/CommandLineArgDictionary.cs). Previously, GetProperty returned a CommandLineArgDictionary instance.

The new version returns a Westwind.WebConnection.ComArray object instead. Which wouldn't be an issue if InvokeMethod would transparently convert the value back to a Dictionary and call the Add(String, String) method. Instead InvokeMethod calls the Add method directly which fails, because there's no overload for Add(String, String). As a result I end up with the error message

Method 'Westwind.WebConnection.ComArray.Add' not found.

on the second line.

cwollenhaupt commented 1 year ago

As a work around I changed the code to

    loCefCommandLineArgs = toBridge.GetProperty(loCefSettings, "CefCommandLineArgs")
    loCefCommandLineArgs = This.UnWrapType (m.toBridge, m.loCefCommandLineArgs)
    toBridge.InvokeMethod (m.loCefCommandLineArgs, "Add", "enable-media-stream", "1")

where UnWrapType is a new method

*========================================================================================
* Workaround for wwDotNetBridge 7.28. 
*========================================================================================
Procedure UnWrapType (toBridge, toObject)

    *--------------------------------------------------------------------------------------
    * Assertions
    *--------------------------------------------------------------------------------------
    #IF __DEBUGLEVEL >= __DEBUG_REGULAR
        Assert Vartype (m.toBridge) == T_OBJECT
        Assert Vartype (m.toObject) $ T_OBJECT + T_NULL
    #ENDIF

    *--------------------------------------------------------------------------------------
    * Check for null values here so that we don't have to check them on every call.
    *--------------------------------------------------------------------------------------
    If IsNull (m.toObject)
        Return m.toObject
    EndIf

    *--------------------------------------------------------------------------------------
    * Check if .NET Bridge wrapped the native object with a ComArray.
    *--------------------------------------------------------------------------------------
    Local loType, lcName, loUnwrapped
    loType = m.toObject.GetType ()
    lcName = toBridge.GetProperty (m.loType, "FullName")
    If m.lcName == "Westwind.WebConnection.ComArray"
        loUnwrapped = m.toObject.Instance
    Else
        loUnwrapped = m.toObject
    EndIf

Return m.loUnwrapped

That works, but as the unfortunate side effect that I need to go through all of my existing .NET Bridge calls to identify those that expect a .NET type to be returned instead of a wrapper.

cwollenhaupt commented 1 year ago

Alternatively, it appears to work if I change the fix up code for the instance parameter to

        internal object InvokeMethod_Internal(object instance, string method, params object[] args)
        {
            var fixedInstance = FixupParameter(instance);
RickStrahl commented 1 year ago

ComArray has an Instance property that holds the original array/list/dictionary instance, so you can always use that for direct Reflection and also to pass it to another method if needed. I think the problem with that is that if the instance is generic, you can't directly access it, so you always have to use InvokeMethod/Get/SetProperty because the generic type can't be passed into FoxPro via COM - it has to stay in .NET.

If you use InvokeMethod() ComArray will automatically use the Instance when you pass in a ComArray as a parameter to a method, so unless you're directly invoking a .NET method without reflection ComArray should do the right thing.

For adding to dictionaries you'll want to use the new ComArray.AddDictionaryItem() method which specifically calls IDictionary.Add() with two values. This was added recently and hasn't been added to Client Tools yet, but is in Web Connection and OSS version.

There's also an Add() which is the same functionality as AddItem() but it only works with a single parameter (ie. IList). For dictionaries AddDictionaryItem() should be used.

The latest, updated documentation is in the Web Connection docs at the moment as the Client Tools have not been updated with the latest wwDotnetBridge bits:

Class ComArray

ps. Seems like the Add method should perhaps support multiple parameters so we could do more natural Add(object, object) based on the PCOUNT().

cwollenhaupt commented 1 year ago

I get what you say, but that's not what the code does...

        internal object InvokeMethod_Internal(object instance, string method, params object[] args)
        {
            var fixedInstance = instance;
            if (instance is ComValue)
                fixedInstance = ((ComValue) instance).Value;

            SetError();

InvokeMethod and most other methods only use the Value property of ComValue. They are not using the corresponding values for ComArray or ComGuid.

RickStrahl commented 1 year ago

The fix up is performed on all parameters and result values, yes, but not the instance. I just never thought of why you would invoke on the ComArray, when you can invoke on ComArray.Instance.

It seems the way this probably could work is:

internal object InvokeMethod_Internal(object instance, string method, params object[] args)
{
    var fixedInstance = instance;
    if (instance is ComValue)
        fixedInstance = ((ComValue) instance).Value;
    else if (instance is ComArray)
        fixedInstance = ((ComArray)instance).Instance;

    SetError();

Likewise for the GetProperty/SetProperty etc.

Just thinking if this might break something else. Instance would still work, and it's unlikely you would actually invoke a method on the actual ComArray object itself, so probably safe.

cwollenhaupt commented 1 year ago

Because until a few weeks ago wwDotNetbridge did not return a ComArray for dictionaries when you called GetProperty. You made a breaking change here that requires me and others to go through all of our code and maybe even implement different cases for different versions of wwDotNetBridge.

It might not look like this for you, because your recent update was the first one in two years and you probably used your new version for quite some time. But it's pretty new for everyone else.

RickStrahl commented 1 year ago

I think the above fix would fix the issue though as you could still pass back the ComArray... That would make the InvokeMethod(dict,"Add") work although you'd probably want to switch that to AddDictionary.

I'm going to see about adding the Add() method.

I'll take a look tomorrow, but in the meantime you can probably try the fix above on your copy and see if that gets you past the initial breakage.

cwollenhaupt commented 1 year ago

wwDotNetBridge has become a crucial part of our day to day work with VFP. In the past three years all of my sessions at Virtual FoxFest used it. 😉

I'll take a look tomorrow, but in the meantime you can probably try the fix above on your copy and see if that gets you past the initial breakage.

I tested the code in my third comment which just calls FixupParameter which is a superset of the the if lines. This worked just fine with my test cases. So yeah, your change should to the trick. It's replicated in many methods, though, not just InvokeMethod_Internal.

RickStrahl commented 1 year ago

Ah shit now I remember why no overloaded Add() because overloading doesn't work via direct COM and ComArray is directly accessed. That's why there's the AddDictionaryItem() instead.

RickStrahl commented 1 year ago

So made the changes and testing with this code:

loList = loBridge.InvokeMethod(loNet,"GetDictionary")
? loBridge.ToString(loList)
? loList.Count  &&  2

loCust =  loList.Item("Item1")   && Retrieve item by Key
? loCust.Company
loCust.Company = loCust.Company + " " + TIME()

loCust = loList.CreateItem()
? loCust
loCust.Company = "North Wind Traders"
loList.AddDictionaryItem("Item3", loCust)

*** This should be your dictionary code? Works now
loBridge.InvokeMethod(loList,"Add","Item 4",loCust)
? lolist.Count

Checking in as a separate branch for now until I can test better tomorrow. Dictionary_Fix_BackwardsCompatibility.

RickStrahl commented 1 year ago

I've updated the code for the ComArray wrapping and pushed the latest changes into Master.

I also updated the Client Tools docs to the latest version so that the new ComArray features are listed. The docs are now up to date with Web Connection and OSS versions, but out of sync with the actual Client Tools version (ugh). The only real difference is the dictionary and item handling so this is relatively minor and I'll update the Client Tools version next week.

Thanks for your help. Let me know if there is still something that doesn't fix the backwards compatibility that might need further updates, but I think this should do the trick.