Issue 103653 - Some change to scripting/source/vbaevents/eventhelper.cxx
Summary: Some change to scripting/source/vbaevents/eventhelper.cxx
Status: CLOSED FIXED
Alias: None
Product: App Dev
Classification: Unclassified
Component: vba (show other issues)
Version: 3.3.0 or older (OOo)
Hardware: Unknown All
: P3 Trivial
Target Milestone: ---
Assignee: joerg.skottke
QA Contact: noel.power
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-20 10:31 UTC by liuchenn
Modified: 2017-05-20 09:32 UTC (History)
3 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
patch file (text\plain) (13.74 KB, text/plain)
2009-07-20 10:32 UTC, liuchenn
no flags Details
test document (26.50 KB, application/vnd.ms-excel)
2009-07-20 11:30 UTC, liuchenn
no flags Details
patch file (13.63 KB, text/plain)
2009-08-11 06:59 UTC, liuchenn
no flags Details
patch file (13.58 KB, text/plain)
2009-08-18 03:43 UTC, liuchenn
no flags Details
patch file (13.28 KB, patch)
2009-09-15 16:21 UTC, noel.power
no flags Details | Diff
Modified test document (29.00 KB, application/vnd.ms-excel)
2009-12-16 14:43 UTC, ab
no flags Details
test doc ( again ) (24.00 KB, application/vnd.ms-excel)
2010-01-27 11:42 UTC, noel.power
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description liuchenn 2009-07-20 10:31:13 UTC
Some change to scripting/source/vbaevents/eventhelper.cxx, which includes:

1. change the event execution mechanism to support the adding of one event to 
some specific kind of controls

2. add the support of VBA click event on ComboBox

3. add the support of VBA exit event on TextBox

4. add the support of VBA click event on Label

5. add the support of VBA mousemove event on all possible controls, when 
the "Shift" key is pressed.
Comment 1 liuchenn 2009-07-20 10:32:21 UTC
Created attachment 63638 [details]
patch file (text\plain)
Comment 2 liuchenn 2009-07-20 11:30:58 UTC
Created attachment 63641 [details]
test document
Comment 3 noel.power 2009-07-22 12:53:43 UTC
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. 
Comment 4 noel.power 2009-07-28 12:10:43 UTC
->liuchenn
will you be providing a reworked patch?
Comment 5 liuchenn 2009-07-30 01:50:35 UTC
->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.:-( 
Comment 6 liuchenn 2009-08-11 06:59:48 UTC
Created attachment 64059 [details]
patch file
Comment 7 liuchenn 2009-08-11 07:15:32 UTC
->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.
Comment 8 liuchenn 2009-08-11 07:16:39 UTC
->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.
Comment 9 liuchenn 2009-08-11 07:17:15 UTC
->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.
Comment 10 Martin Hollmichel 2009-08-13 12:44:28 UTC
target 3.1 is history, set future target milestone.
Comment 11 noel.power 2009-08-17 18:26:13 UTC
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


Comment 12 liuchenn 2009-08-18 03:41:27 UTC
You are right, now the original line 352 is changed to be:

            }while(i < nCount && sEventInfo == pTransProp->sEventInfo);
Comment 13 liuchenn 2009-08-18 03:43:25 UTC
Created attachment 64223 [details]
patch file
Comment 14 noel.power 2009-09-15 16:21:54 UTC
Created attachment 64778 [details]
patch file
Comment 15 noel.power 2009-09-15 16:22:49 UTC
patch attached ( above ) is backported version to dev300_m58
Comment 16 noel.power 2009-09-23 15:31:19 UTC
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 :-) )
Comment 17 ab 2009-09-25 14:15:59 UTC
STARTED, -> ab71
Comment 18 ab 2009-09-29 09:06:26 UTC
-> OOo 3.3
Comment 19 ab 2009-11-10 12:32:04 UTC
.
Comment 20 ab 2009-12-16 14:37:48 UTC
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
Comment 21 ab 2009-12-16 14:43:45 UTC
Created attachment 66664 [details]
Modified test document
Comment 22 ab 2009-12-16 14:44:54 UTC
ab->jsk: Please verify
Comment 23 joerg.skottke 2010-01-27 10:22:50 UTC
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)
Comment 24 noel.power 2010-01-27 11:42:25 UTC
Created attachment 67438 [details]
test doc ( again )
Comment 25 noel.power 2010-01-27 11:56:48 UTC
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
Comment 26 joerg.skottke 2010-01-27 12:46:26 UTC
Verified, a bug has been filed for the exit loop (issue 108711)