Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#12 ImplAAFObject:SetPropertyValue sync prob

open
nobody
None
2
2003-08-20
2002-10-01
John Emmas
No

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.

Discussion

  • Jim Trainor
    Jim Trainor
    2003-03-26

    • priority: 5 --> 7
     
  • Jim Trainor
    Jim Trainor
    2003-03-26

    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.

     
  • Jim Trainor
    Jim Trainor
    2003-03-26

    • assigned_to: nobody --> jptrainor
     
  • Jim Trainor
    Jim Trainor
    2003-03-26

    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.

     
  • John Emmas
    John Emmas
    2003-03-27

    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.

     
  • Jim Trainor
    Jim Trainor
    2003-03-31

    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

     
  • John Emmas
    John Emmas
    2003-04-02

    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

     
  • Phil Tudor
    Phil Tudor
    2003-08-20

    • priority: 7 --> 2
    • assigned_to: jptrainor --> nobody