To comment on the following update, log in, then open the issue:
http://www.openoffice.org/issues/show_bug.cgi?id=103653
User npower changed the following:
What |Old value |New value
================================================================================
Assigned to|ab |npower
--------------------------------------------------------------------------------
Component|scripting |vba
--------------------------------------------------------------------------------
QA contact|iss...@script |iss...@vba
--------------------------------------------------------------------------------
Subcomponent|code |www
--------------------------------------------------------------------------------
Target milestone|OOo 3.2 |OOo 3.1
--------------------------------------------------------------------------------
Version|OOo 2.4.1 |OOO310m1
--------------------------------------------------------------------------------
------- Additional comments from [email protected] Wed Jul 22 11:53:43
+0000 2009 -------
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.
---------------------------------------------------------------------
Please do not reply to this automatically generated notification from
Issue Tracker. Please log onto the website and enter your comments.
http://qa.openoffice.org/issue_handling/project_issues.html#notification
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
