Issue 103653

Created attachment 63638 [details]
patch file (text\plain)

Created attachment 63641 [details]
test document

Ok, I take this as the code is based off code that is not in any milestone,
probably vba is the best component and assigned to me would have been the
correct choice :-) 

But anyway lets review this in the context of the code the diff is generated
against, that is the easiest option ( I will backport the reworked patch to
M(insert milestone here ) ) 



1)
in TranslateInfo, bool bApproveAll seems redundant, it doesn't give any extra
information more than the presence of ApproveRule. e.g. I think you could just
get rid of bApproveAll from the structure and replace it as follows

e.g replace 
	if (txInfo.bApproveAll) //the event can be executed on all types of controls

with 
if ( txInfo.ApproveRule )

2)
using aTranslatePropMap_Impl to populate the Map is a really nice idea, I like
it alot ( it significantly reduces the amount of code and makes it nice and easy
to maintain ) - I have one problem with this though. TranslatePropMap contains
the event name 'sEventInfo' ( which is also the key to in EventInfoHash ) so
there is some duplication here ( that also was the case previously )
Additionally sEventInfo seems not to be used except as a key to retrieve
TranslateInfo. 
However with the new aTranslatePropMap_Impl we duplicate the string even more
times, it would be nice to get rid of that. Additionally using the additional
TranslatePropMap seems strange and a little wasteful ( as it duplicates
TranslateInfo ). I was thinking something like

struct TranslateInfo // with the bApproveAll & sEventInfo removed
{
    Translator toVBA;       //the method to convert OO event parameters to VBA
event parameters
	bool (*ApproveRule)(const ScriptEvent& evt, void* pPara); //this method is used
to determine which types of controls should execute the event 
	void *pPara;			//Parameters for the above approve method
};

struct TranslatePropMap
{
    const char* sEventInfo;
    TranslateInfo aTransInfo;
}


static TranslatePropMap aTranslatePropMap_Impl[] = 
{
	// actionPerformed ooo event
	{ "actionPerformed", { MAP_CHAR_LEN("_Click"), NULL, NULL, NULL } },
	{ NULL, { MAP_CHAR_LEN("_Change"), NULL, true, NULL, NULL} },

        // itemStateChanged ooo event, add to support VBA ComboBox_Click event
	{ "itemStateChanged", MAP_CHAR_LEN("_Click"), NULL, ApproveType,
(void*)(&comboBoxList)},
        "
        "
	// keyPressed ooo event
	{"keyPressed", { MAP_CHAR_LEN("_KeyDown"), ooKeyPressedToVBAKeyUpDown, NULL,
NULL} },
	{ NULL, { MAP_CHAR_LEN("_KeyPress"), ooKeyPressedToVBAKeyUpDown, NULL, NULL} },
};
^^^^^
note: no need to terminate aTranslatePropMap_Impl with and empty struct, just
use sizeof( aTranslatePropMap_Impl ) / sizeof( aTranslatePropMap_Impl[0] ) to
find the number of entries to process

3)
I have a question about pParams, I see in the implementation it is alway a
TypeList but each typelist has only 1 element? would just using a since Type be
easier? ( or did I miss some logic for doing it )


4)
Just a nitpick the following test just looked weird to me, imo its easier and
clearer to replace the following 
		Any aRet( xInterface->queryInterface( *pType ) );
		if ( typelib_TypeClass_INTERFACE == aRet.pType->eTypeClass )
with
		if ( xInterface->queryInterface( *pType ).hasValue() )
that *should* work

but overall this looks really nice. 

->liuchenn
will you be providing a reworked patch?

->npower

The pressure of other occupations has prevented me from revising the code and
making a reworked patch. I am afraid that I might not have time to work on this
issue at least in the incoming one or two weeks.:-( 

Created attachment 64059 [details]
patch file

->npower

Your opinions are insightful and I revised the code according to your opinions 
1), 2) and 4). 

About 3), although now a new VBA event is only added to a single control (or 
all controls), but I suppose that there might be circumstances that some VBA 
event must be add to two or more types of controls (but not all), so I use 
this typelist structure.

->npower

Your opinions are insightful and I revised the code according to your opinions 
1), 2) and 4). 

About 3), although now a new VBA event is only added to a single control (or 
all controls), but I suppose that there might be circumstances that some VBA 
event must be add to two or more types of controls (but not all), so I use 
this typelist structure.

->npower

Your opinions are insightful and I revised the code according to your opinions 
1), 2) and 4). 

About 3), although now a new VBA event is only added to a single control (or 
all controls), but I suppose that there might be circumstances that some VBA 
event must be add to two or more types of controls (but not all), so I use 
this typelist structure.

target 3.1 is history, set future target milestone.

in general the changes look good :-) unfortunately I didn't get a chance to do
some testing as it core dumps at line cores at the line 347 in ( rtl::operator== )

 343                         if (sEventInfo != pTransProp->sEventInfo)
 344                         {
 345                                 sEventInfo = pTransProp->sEventInfo;
 346                                 std::list< TranslateInfo > infoList;
 347                                 do
 348                                 {                                         
                             
 349                                         infoList.push_back(
pTransProp->aTransInfo );
 350                                         pTransProp++;
 351                                         i++;
 352                                 }while(sEventInfo == pTransProp->sEventInfo);
 353                                 eventTransInfo[sEventInfo] = infoList;
 354                         }
I don't trust the reported line in the stack trace more likely it really cores
in 352, looks like you need to ensure that i++ does not go out of bounds

You are right, now the original line 352 is changed to be:

            }while(i < nCount && sEventInfo == pTransProp->sEventInfo);

Created attachment 64223 [details]
patch file

Created attachment 64778 [details]
patch file

patch attached ( above ) is backported version to dev300_m58

Andreas, the last patch is the backported version of this ( against dev300m58 )
, can we add this to ab71? ( assigning to you hoping it is ok :-) )

STARTED, -> ab71

-> OOo 3.3

.

The attached test document does not work, because it uses
functionality to open the dialog that isn't available yet
on Vanilla OpenOffice / StarOffice. The ComboBox support
mentioned above also does not work, probably also due to
missing code. We will create a follow up issue for this.
I will add a modified test document avoiding the problems.

FIXED

Created attachment 66664 [details]
Modified test document

ab->jsk: Please verify

The sample document from AB sends me into an endless dialog-loop when clicking
the button, selecting the second entryfield -> Testbox2Exit pops up an infinite
number of times.

Hmm, no - whatever i do, i run into the loop. Stack submitted (from killing the
app forcefully)

Created attachment 67438 [details]
test doc ( again )

the problem here is due to the fake event handling doing what it is supposed to
do. Unfortunately the way it does this can lead to an infinite loop, you can
call this a bug ( or a limitation ) What happens? the TextBox_Exit event is
generated whenever the textbox loses focus. So.. why does it loop then, well, if
you have focus in one of the textboxes and then  you click ( or move over ) one
of the labels that generates an event that raises a message box then the textbox
loses focus, generates a textbox_exit event which raises another messagebox
which causes the textbox to lose focus again ( and so on ) In MSO vba this
doesn't happen ( perhaps there is some co-operation with the message box )
However I believe this is a different issue ( and a fight for another day )

anyway, back to this issue, to test it ( without looping ) use the TestDemo3.xls
just attached, click between the combobox and textboxes [*] ( the trick is to
ensure that a textbox_exit event is not generated when you end up with focus in
a textbox ) using this technique you should be able to generate the exit events
for both textboxes. 

[*] don't switch between textboxes, always go between a textbox and the
combobox, and don't click on any of the labels as these don't hold the focus
after clicking

Verified, a bug has been filed for the exit loop (issue 108711)