Menu
▾
▴
#12 ImplAAFObject:SetPropertyValue sync prob
ImplAAFObject::SetPropertyValue() checks the object's
class definition to ensure that the given property
definition is registered in its OMPropertySet - then it
goes on to set the corresponding property value by
calling:-
_pProperties->SetPropertyValue(pPropDef, pPropVal);
However, '_pProperties' is acting on a different property
set. Therefore the two property sets need to be
synchronised for this to work correctly. I would suggest
the following modification to the existing code:- After,
if (AAFRESULT_FAILED(ar))
return AAFRESULT_BAD_PROP;
insert:-
else
{
ar = pProperties->SynchronizeProperty(this, pPropDef);
if (AAFRESULT_FAILED(ar))
return ar;
}
The above doesn't seem to cause any problems if the
two property sets were already synchronized.
BTW is there a way to post code fragments to this
forum without having leading whitespace removed??
John E.
Logged In: YES
user_id=239292
John E.,
I have studied the code and have *not* concluded that a
problem exists. When _pProperties is inititalized (in
ImplPropertyCollection::Initialize() ) all the class' properties
are added to it (i.e., it calls synchronize for each property).
The only problem I could imagine was if a new property was
added to an object leaving _pProperties out of date. But that
is handled in ImplPropertyCollection::SetPropertyValue().
(The fragment of code that handles that situation appears
relatively new since it uses smart pointers while the rest of
the code uses dumb pointers). I stepped through the code
with a test program that sets the value of a new property to
verify this.
When you wrote "'_pProperties' is acting on a different
property set", what exactly did you mean?
I'll leave this bug open until there is more certainity regarding
the presence of a problem and any changes the may be
required. You reply would be appreciated so that this bug
can be closed.
As an aside...
I did notice other suspicisous, or unnecessary, code
fragments. For example there are two calls to
ImplPropertyCollection::InitProperties() in
ImplAAFObject::SetPropertyValue(). I don't see why that is
necessary, unless the first call fails under certain conditions.
This is the code fragment that appears twice in
ImplAAFObject::SetPropertyValue() :
if(!_pProperties)
{
ar=InitProperties();
if (AAFRESULT_FAILED(ar))
return ar;
}
... the second appearance is either redundant or wrong. Prior
to a change I made to the InitProperties() method on
2003/02/13 there was no way for the _pProperties to ever be
reset to zero... so the second "if (!pProperties)" code
fragment would never be executed.
This file has quite a long revision history (75 revisions)... not a
great sign. Perhaps a review and cleanup is in order.
Logged In: YES
user_id=239292
John E.,
I have studied the code and have *not* concluded that a
problem exists. When _pProperties is inititalized (in
ImplPropertyCollection::Initialize() ) all the class' properties
are added to it (i.e., it calls synchronize for each property).
The only problem I could imagine was if a new property was
added to an object leaving _pProperties out of date. But that
is handled in ImplPropertyCollection::SetPropertyValue().
(The fragment of code that handles that situation appears
relatively new since it uses smart pointers while the rest of
the code uses dumb pointers). I stepped through the code
with a test program that sets the value of a new property to
verify this.
When you wrote "'_pProperties' is acting on a different
property set", what exactly did you mean?
I'll leave this bug open until there is more certainity regarding
the presence of a problem and any changes the may be
required. You reply would be appreciated so that this bug
can be closed.
As an aside...
I did notice other suspicisous, or unnecessary, code
fragments. For example there are two calls to
ImplPropertyCollection::InitProperties() in
ImplAAFObject::SetPropertyValue(). I don't see why that is
necessary, unless the first call fails under certain conditions.
This is the code fragment that appears twice in
ImplAAFObject::SetPropertyValue() :
if(!_pProperties)
{
ar=InitProperties();
if (AAFRESULT_FAILED(ar))
return ar;
}
... the second appearance is either redundant or wrong. Prior
to a change I made to the InitProperties() method on
2003/02/13 there was no way for the _pProperties to ever be
reset to zero... so the second "if (!pProperties)" code
fragment would never be executed.
This file has quite a long revision history (75 revisions)... not a
great sign. Perhaps a review and cleanup is in order.
Logged In: YES
user_id=567963
Hi James,
I'm attaching a comment to my earlier posting but I'm not
sure if that will count as a follow-up. You asked what I meant
when I said that '_pProperties' was acting on a different
property set? Oh, how I wish I had a good memory - because
I'm sure this must have seemed perfectly clear at the time!
However, when I took another look at the code this morning I
couldnt quite figure out what I was talking about! This is the
closest I can get....
I said at one point that ImplAAFObject::SetPropertyValue()
checks the object's class definition to ensure that the given
property definition is registered in its OMPropertySet. This is
achieved by the line:-
ar = pClass->LookupPropertyDefbyOMPid(pid, &pTempProp);
If the above call returns successfully (ar ==
AAFRESULT_SUCCESS) then theres an implicit assumption
that its okay to carry on and set the new property value. The
point I was making (I think) was that the property set being
examined by the above call (ImplAAFClassDef::_Properties)
isnt the same property set that were about to set the new
value in (the latter being ImplAAFObject::_pProperties). If
this is a new property that has only just been added to the
object, its possible that the class defs property set will know
about the corresponding property definition even though the
objects property set hasnt yet been updated to contain the
property def hence the need to call SynchronizeProperty().
You mentioned that for new properties, this should be sorted
out by ImplPropertyCollection::SetPropertyValue() but Im
not sure if thats true.
ImplPropertyCollection::SetPropertyValue() calls
LookupOMProperty(pPropDef->OmPid(), &pOmProp); but I
assume that this call will fail if pPropDef hasnt yet been
added to the objects property set.
Admittedly, this is all very hazy now but the suggestion I
made (about calling SynchronizeProperty()) is really just an
extra layer of protection. If the property def is already present
in the objects property set then no harm is done if it isnt
present, it gets added, prior to reaching the line:-
return(_pProperties->SetPropertyValue (pPropDef, pPropVal));
This ensures that the property def is known to _pProperties
before we try to set its value (at least, in the words of Stan
Laurel, "that's my story and I'm stuck with it !!")
Regards,
John.
Logged In: YES
user_id=239292
John,
My understanding is that one must always first register an
optional property definition with the class before assigning a
value. This is done with
IAAFClassDef::RegisterOptionalPropertyDef(). If that is done
then LookupPropertyDefbyOMPid() should succeed, and the
code can assume, at the very least, that the property exists
in the class def. - if not an actual instance of the property in
the object in question.
The code then ends up in
ImplPropertyCollection::SetPropertyValue(). Step 2 (per
comments in rev 1.75 of the file) of that function, line 176,
then checks if an instance of the property is present in the
object. If not, then the new property is created and added to
the object's propertie set. That covers your concern regarding
the Synchronize() call.
It looks to me like the necessary bases are covered: i) check
that the property exists in the class def (fail if not) and ii)
check if an instance of the property exists in the object - and
create it if not.
FYI, I have been testing using the following code, that I
consider a prototypical example of code that adds a new
property:
AAF/example2/axFileGen/FileGenOps/Properties.cpp.
I think everything is handled correctly at present. The
additional Synchronize() call would render the code at line
176 unnecessary - we'd end up with a block of code that
never gets executed.
So, do we close this?
As an aside... I have the feeling there is plenty of code in the
SDK that never gets executed. Some coverage analysis
would be interesting.
Jim Trainor
Logged In: YES
user_id=567963
Hi Jim - sorry for taking so long to reply,
You've obviously put a great deal of effort into looking at this.
Thanks for that and I'm quite happy to accept your findings on
it. The only occasion when this might be a problem (as far as
I can see) would be if someone registered an optional
property def - then tried to set it's value without first calling
CreateOptionalPropertyValue() - which, I guess would be a
programming error anyway. I must have come across such a
situation at some time but it looks as though it would have
been an error by the programmer - so I'm quite happy to
consider the bug closed, as you suggest.
Thanks,
John