Re: test for default history content

Ate,
    There is a problem if getTargetStates does not dereference history 
states.  computeEntrySet calls it to produce the initial list of states 
for addDescendentStatesToEnter to expand.  If getTargetStates doesn't 
dereference history states, they will get added to the statesToEnter 
list before addDescendentStatesToEnter gets called - and history states 
should not be on statesToEnter. Furthermore, computeExitSet calls 
getTargetStates, and it will also need to know what the actual 
dereferenced target state set is.

It's a big gap in the algorithm that getTargetStates isn't defined - I'm 
assuming that it was in an earlier version, and got lost somewhere.

In any case we need multiple fixes so that: 1) history states don't end 
up on the statesToEnter list and 2) default history executable content 
gets recorded.

- Jim
On 4/1/2014 2:45 PM, Ate Douma wrote:
> On 29-03-14 20:26, Zjnue Brzavi wrote:
>> On Sat, Mar 29, 2014 at 1:24 PM, Jim Barnett <1jhbarnett@gmail.com
>> <mailto:1jhbarnett@gmail.com>> wrote:
>>
>>     Zjnue,
>>       I agree that we need more tests, but why would 
>> defaultHistoryContent need
>>     to get passed to getTargetStates?  It's just an accessor 
>> function.  Though I
>>     do think we have to consider how getTransitionDomain and findLCCA 
>> should
>>     handle history states. They ignore them at the moment, and 
>> _maybe_ that's ok...
>>
>>     - Jim
>>
>>
>> Hi Jim,
>>
>> Perhaps as quick background, my implementation follows (atm) the 
>> proposed
>> algorithm pretty much verbatim, in order to aid debugging for cases 
>> like this
>> and to make getting started with SCXML a little easier for myself and 
>> others.
>>
>> Simply put, the recently published changes to the algorithm for
>> defaultHistoryContent support are not satisfying test579.
>>
>> While the getTargetStates function seems pretty harmless, it is also
>> hisory-aware and can redirect a transition target.
>
> Hi Zjnue,
>
> Sorry for chiming in this late, and going back to this earlier 
> message, but I'm puzzled by the above statement.
>
> I can find no indication in the algorithm nor in the specification 
> text that the getTargetStates function is supposed to do anything more 
> than just returning the direct target(s) of a transition.
>
> It seems to me that the problems you encountered are actually *caused* 
> by the the history-awareness and dereferencing in your getTargetStates 
> implementation.
>
> AFAICT the pseudo algorithm expects it will take care of the history 
> dereferencing itself in the addDescendantStatesToEnter procedure.
> If you do this upfront in you getTargetStates implementation, the 
> addDescendantStatesToEnter never will get to 'see' a history state as 
> it is supposed to, and neither can it then handle the default history 
> content as just was added.
>
> I just completed the implementation of the current algorithm for 
> Apache Commons SCXML (although not yet committed), and I now can 
> successfully run test579, so it seems to me the proposed change simply 
> works as expected.
>
> But maybe I'm overlooking some other specification requirements for 
> getTargetStates?
> I'd appreciate some pointers in that case :)
>
> Regards, Ate
>
>
>> In the case of test579, the <initial> transition, targets <history 
>> id="sh1">,
>> but is effectively redirected to <state id="s01" >, skipping any 
>> executable
>> content of <history> along the way.
>>
>> Please take a look at my code for getTargetStates and getTargetState 
>> below [1].
>>
>> It is only with the following two lines present in getTargetState:
>>                      if( defaultHistoryContent != null && 
>> h.parent.exists("id") )
>> defaultHistoryContent.set(h.parent.get("id"),
>> h.transition().next());
>> ..that 579 passes.
>>
>> As mentioned, I did not look at it in depth and will not be able to 
>> spend more
>> time on it over the next few days unfortunately.
>>
>> So in summary, after adding the published additions to the algorithm, 
>> test579
>> did not pass.
>> It was only after the changes to getTargetStates that it did.
>> Hope this helps for now.
>>
>> Thanks,
>> Zjnue
>>
>> [1]
>>
>>      function getTargetStates( node : Node, ?defaultHistoryContent :
>> Hash<Iterable<Node>> ) : List<Node> {
>>          var l = new List<Node>();
>>          if( !node.exists("target") )
>>              return l;
>>          var ids = node.get("target").split(" ");
>>          var top = node;
>>          while( top.parent != null && !top.isTScxml() )
>>              top = top.parent;
>>          for( id in ids ) {
>>              var ts = getTargetState(top, id, defaultHistoryContent);
>>              for( tss in ts )
>>                  l.add( tss );
>>          }
>>          return l;
>>      }
>>
>>      function getTargetState( s : Node, id : String, 
>> ?defaultHistoryContent :
>> Hash<Iterable<Node>> ) : List<Node> {
>>          if( s.get("id") == id )
>>              return [s].toList();
>>          else {
>>              for( child in s.getChildStates() ) {
>>                  var ss = getTargetState(child, id, 
>> defaultHistoryContent);
>>                  if( ss != null )
>>                      return ss;
>>              }
>>              for( h in s.history() ) {
>>                  var hh = getTargetState(h, id);
>>                  if( hh != null ) {
>>                      if( defaultHistoryContent != null && 
>> h.parent.exists("id") )
>> defaultHistoryContent.set(h.parent.get("id"),
>> h.transition().next());
>>                      return historyValue.exists( h.get("id") ) ?
>>                          historyValue.get( h.get("id") ) :
>>                          getTargetStates( h.transition().next(),
>> defaultHistoryContent );
>>                  }
>>              }
>>          }
>>          return null;
>>      }
>>
>>
>>
>>     On 3/28/2014 8:51 PM, Zjnue Brzavi wrote:
>>>
>>>         I've added test579 to the suite.  Let me know if there are 
>>> problems
>>>         with it or if you think there need to be more/different 
>>> tests.--
>>>
>>>
>>>     Hi,
>>>
>>>     Not had a very good look yet, but it appears we'll need to pass
>>>     defaultHistoryContent to getTargetStates as well, as it is 
>>> another area
>>>     where the algorithm is skipping past some history elements without
>>>     considering their executable content. This diff passes test579 :
>>> https://github.com/zjnue/hscxml/commit/4b9091470d3c7c1995e6b41900535e0437ec3eb4
>>>     Another test or 2 on the subject is probably a good idea.
>>>
>>>     Best regards,
>>>     Zjnue
>>
>>     --
>>     Jim Barnett
>>     Genesys
>>
>>
>
>

-- 
Jim Barnett
Genesys

Received on Tuesday, 1 April 2014 22:11:28 UTC