[pywin32-bugs] [ pywin32-Bugs-1648655 ] Wrong args order with event handler

[SourceForge.net <no...@so...>]

Bugs item #1648655, was opened at 2007-01-31 21:09
Message generated for change (Comment added) made by mhammond
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551954&aid=1648655&group_id=78018

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: com
Group: None
>Status: Closed
>Resolution: Fixed
Priority: 5
Private: No
Submitted By: atsuo ishimoto (ishimoto)
Assigned to: Nobody/Anonymous (nobody)
Summary: Wrong args order with event handler

Initial Comment:
Order of args doesn't match with prototype generated by makepy.

To reproduce, please run script below and Save workbook on the Excel.

# <<<<<< BEGIN >>>>>>
import win32com.client

class WorkbookEvent:
    def OnBeforeSave(self, SaveAsUI, Cancel):
        print "OnBeforeSave", SaveAsUI, Cancel
        return False

xl = win32com.client.Dispatch("Excel.Application")
xl.Visible = 1
wb = DispatchWithEvents(xl.Workbooks.Add(), WorkbookEvent)
while 1:
    pythoncom.PumpWaitingMessages()

# <<<<<< END >>>>>>

When I save workbook, WorkbookEvent.OnBeforeSave called with 
SaveAsUI=False and Cancel=False. But on SaveAs, args passed are 
SaveAsUI=False and Cancel=True, which should be SaveAsUI=True and 
Cancel=False. 

Python2.4.3,pywin32-210,MS-Excel 2000



----------------------------------------------------------------------

>Comment By: Mark Hammond (mhammond)
Date: 2007-09-04 20:55

Message:
Logged In: YES 
user_id=14198
Originator: NO

Thanks Roger!  I've added both warnings as you suggested.

Checking in com/TestSources/PyCOMTest/PyCOMImpl.cpp;
new revision: 1.16; previous revision: 1.15
Checking in com/TestSources/PyCOMTest/PyCOMImpl.h;
new revision: 1.15; previous revision: 1.14
Checking in com/TestSources/PyCOMTest/PyCOMTest.idl;
new revision: 1.16; previous revision: 1.15
Checking in com/win32com/src/PyGatewayBase.cpp;
new revision: 1.17; previous revision: 1.16
Checking in com/win32com/src/PythonCOM.cpp;
new revision: 1.49; previous revision: 1.48
Checking in com/win32com/test/testPyComTest.py;
new revision: 1.28; previous revision: 1.27


----------------------------------------------------------------------

Comment By: Roger Upole (rupole)
Date: 2007-09-02 04:14

Message:
Logged In: YES 
user_id=771074
Originator: NO

This works for all the cases I've tried.

A couple of things that might be nice to do
while we're into the code:

  Log the exception in invoke_setup instead of
  just clearing it

  In invoke_finish, log a warning if it doesn't
  receive the expected number of output parms.


----------------------------------------------------------------------

Comment By: Mark Hammond (mhammond)
Date: 2007-08-30 18:28

Message:
Logged In: YES 
user_id=14198
Originator: NO

I think this should work OK.  The out params seem harder than they should
be, but after a few false starts, this is the best I can come up with.  The
cost for no named params should be low.  Let me know what you think...
File Added: bug-1648655.patch

----------------------------------------------------------------------

Comment By: Roger Upole (rupole)
Date: 2007-08-29 16:32

Message:
Logged In: YES 
user_id=771074
Originator: NO

I don't see anything in the docs to indicate that the sequential
order of DISPIDs can't be depended upon.  One thing we'd have
to be careful of is missing args.  I guess the tuple could be
created with the max of the DISPIDs, and we could insert an
ArgNotFound object for any args not passed.


----------------------------------------------------------------------

Comment By: Mark Hammond (mhammond)
Date: 2007-08-29 14:07

Message:
Logged In: YES 
user_id=14198
Originator: NO

Actually, I think there might be an easier way.  The docs for
ITypeInfo::GetIDsOfNames says "The IDs of parameters are 0 for the first
parameter in the member function's argument list, 1 for the second, and so
on" and I'm not aware of a way to override that in the IDL.  So the point
of GetIDsOfNames is when you do *not* know about the params - all you know
is the names of params presented to you.  If you pass the entire set of
param names to GetIDsOfNames, you'd always get back an array from 0->n.

So for our purposes, this is enough information - we can just treat the
dispID as an offset into our arg tuple.  Creating the tuple etc becomes a
little more complicated, but thats OK, especially if we optimize the 'no
named params' case.

does that make sense?

----------------------------------------------------------------------

Comment By: Roger Upole (rupole)
Date: 2007-08-29 13:33

Message:
Logged In: YES 
user_id=771074
Originator: NO

>From playing around inside the python event code, it looks like the type
info may be available in a roundabout way:

        ti=self._oleobj_.GetTypeInfo(0)

The returned type info is for the WorkBook itself, but the event
source type info should be locatable from there (I think).

The gateway object stashes away a reference to the class instance that's
passed
as self to the event methods in _obj_. Inside PyGatewayBase::Invoke, you
can
get the object that provides the type info like this:

	PyObject *_obj_, *_oleobj_;
	_obj_=PyObject_GetAttrString(this->m_pPyObject, "_obj_");
	if (_obj_)
		_oleobj_=PyObject_GetAttrString(_obj_, "_oleobj_");

That's as far as I've got for right now.



----------------------------------------------------------------------

Comment By: Mark Hammond (mhammond)
Date: 2007-08-28 08:17

Message:
Logged In: YES 
user_id=14198
Originator: NO

Yeah, I'm a little confused about that page - but it seems pretty obvious
that every Excel related call we receive does indeed have index[0] of the
named items being a dispid of zero, and that this corresponds to the first
arg.  I understand that this isn't guaranteed though (and yeah, it is
apparently not even what is recommended according to that page.)

I don't think we can use GetTypeInfo() in the way you suggest.  Consider
this Excel example - we don't have a pointer to an Excel object, only one
of our objects that implements an interface defined by Excel.  I'm not sure
how the PyGatewayBase code at that point could determine the dispids as
defined in the excel typelib.  The only way I can see it working is to have
makepy include info in the class it generates, and this code could call
back on the policy to resolve the dispids to names.  This policy code could
sniff the properties written to the generated class, or if they don't
exist, just do what is done now.  Note that I do *not* think we can pass a
keywords arg dict, as that will defeat attempts to treat such as args as a
byref.

Or am I missing something regarding getting the ITypeInfo from the object?

----------------------------------------------------------------------

Comment By: Roger Upole (rupole)
Date: 2007-08-28 06:57

Message:
Logged In: YES 
user_id=771074
Originator: NO

As far as I can tell, this fixes the Excel events that pass the named
args
sequentially.  I also tested it out with several different types of
events
from IE, which uses positional args.

However, unless I'm misreading the page on MSDN, the canonical order even
for named args is last arg first.  Again, that's not a requirement.
Even though this patch works for the specific Office events in question,
it may break com servers that fire events with the named args in reverse
order, which would currently be called correctly.

The only way I can see to make sure the args are passed correctly in all
cases
is to do a reverse mapping from the DISPID's to the names.  This would
require that
the object have type info, but we already require that for
DispatchWithEvents.

A basic outline might look something like this:
    If named args are passed, use GetTypeInfo to retrieve the server's
type info
    Use ITypeInfo->GetNames for the invoked member id, which returns the
names
        in the order in which they're to be passed to the python method.
    Call GetIDsOfNames with the above names to retrieve their DISPID's in
the same order
    Loop over DISPID's for the named args, and use the order of the
DISPID's returned from above
        to build the arg tuple in the order the names are returned
        Alternately, we could use keywords as you suggested and build an
arg tuple
            and dict to be passed to the python implementation of the
method.
    On return, use the ordered DISPID's to determine which VT_BYREF params
receive
        the values returned from python.

This looks to be fairly expensive in terms of processing time, but
apparently
named args aren't frequently used so there shouldn't be too much of a
performance
hit in the general case.



----------------------------------------------------------------------

Comment By: Mark Hammond (mhammond)
Date: 2007-08-27 18:13

Message:
Logged In: YES 
user_id=14198
Originator: NO

Thanks Roger - nice work tracking that down :)  That is pretty nasty :(  A
proper fix isn't trivial either as there is no obvious way to map the
dispid to the name in this code.  We'd probably end up needing to have
makepy include this param information and have our code call back into our
policy to resolve things.  Its harder given the way we return BYREF params
(ie, its not as simple as leaning on Python's keyword capabilities)

In the short term, I think the existing checks can remain - their intent
is kinda reasonable even if they are wrong :)  We should then unpack the
named params in the reverse order.  This would then work for people who set
their params up the "recommended"[1] way - and this is what Office seems to
do, as per these examples.

I've attached the start of a patch which appears to fixup the main issue,
but still fails to handle "out" params correctly (but the test suite
changes pick that up) - it might be a few days before I can look at this
again.  In the meantime, if you (Roger) can find the time to review the
patch I'd appreciate it, and it seems somewhat strange that the args
*passed* are reversed in the 'named' case, but the test suite changes
appear to match both the docs and the description in that MS article.

[1] http://msdn2.microsoft.com/en-us/library/ms221653.aspx
File Added: bug-1648655.patch

----------------------------------------------------------------------

Comment By: Roger Upole (rupole)
Date: 2007-08-26 12:33

Message:
Logged In: YES 
user_id=771074
Originator: NO

I think the problem is that the args wind up backwards when they're passed
as named args.

For a different event with 3 parms:
    def OnSheetBeforeDoubleClick(self, Sh, Target, Cancel):
        print Sh, Target, Cancel
        return False
This prints
    False None <win32com.gen_py.Microsoft Excel 9.0 Object
Library._Worksheet instance at 0x21747576>

The first arg Sh should be the worksheet, but I get a
boolean as the first parm, and the worksheet ends up as the 3rd arg.

>From stepping through in the debugger, both of these events are passing
named parms.
The code that creates the arg tuple to pass to the event method isn't
doing any mapping
between names and DISPID's.  It only verifies that the named params are in
sequential
order (which they are not actually required to be).  However, it then goes
on to process
the named args in reverse order just as if they were positional args.


----------------------------------------------------------------------

Comment By: atsuo ishimoto (ishimoto)
Date: 2007-07-28 22:35

Message:
Logged In: YES 
user_id=463672
Originator: YES

Thank you for comment.
But same script with VB.NET, correct values are passed with correct
order.
So, I guess typelib is corrent.


Part of VB script follows.


    Private WithEvents e1 As Excel.Application

    Private Sub e1_WorkbookBeforeSave(ByVal Wb As Excel.Workbook, ByVal
SaveAsUI As Boolean, ByRef Cancel As Boolean) Handles
e1.WorkbookBeforeSave
        MsgBox("SaveAsUI:" + Str(SaveAsUI) + " Cancel:" + Str(Cancel))
    End Sub
End Class




----------------------------------------------------------------------

Comment By: Mark Hammond (mhammond)
Date: 2007-07-28 17:17

Message:
Logged In: YES 
user_id=14198
Originator: NO

Sorry, but I can't see a bug of ours here.  It sounds like the type info
Excel gives out is wrong - otherwise I'd expect *all* params to be reversed
for all events, and I don't think that is the case.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551954&aid=1648655&group_id=78018