Re: test for default history content

OK, now I see what you mean.  I've looked at it a bit and the current 
algorithm is incorrect whether or not getTargetStates dereferences 
history states or not.  If it doesn't, then the history state gets added 
to statesToEnter. If it does, then the executable content  gets lost.  
And, worst of all, the algorithm doesn't specify which it does.  I think 
that  it  should reference history states (otherwise there's not much 
point to it), in which case it does need update defaultHistoryContent.   
If we handle the default content there, then we can remove the clauses 
about history states from addDescendentStatesToEnter.

I'm at a company offsite next week, and will be pretty much useless, but 
what if we define

getTargetStates(stateList, defaultEntryContent)
     allTargets = newOrderedSet
     for state in stateList:
          if isHistoryState(state):
               if historyValue[state.id]:
                    allTargets.union(historyValue[state.id])
               else:
allTargets.union(getTargetStates(state.transition.target))
                   defaultHistoryContent[state.parent.id] = 
state.transition.content
          else:
              allTargets.add(state)

If we do this, addDescendentStatesToEnter should never see a history 
state, so it can be simplified to:

procedure addDescendantStatesToEnter(state,statesToEnter,statesForDefaultEntry, defaultHistoryContent):
     if isCompoundState(state):
         statesForDefaultEntry.add(state)
         for s in getTargetStates(state.initial):
            statesToEnter.add(s)
            addDescendantStatesToEnter(s,statesToEnter,statesForDefaultEntry)
            addAncestorStatesToEnter(s, state, statesToEnter, statesForDefaultEntry, defaultHistoryContent)
     elif isParallelState(state):
          for child in getChildStates(state):
              if not statesToEnter.some(lambda s: isDescendant(s,child)):
                 statesToEnter.add(child)
                 addDescendantStatesToEnter(child,statesToEnter,statesForDefaultEntry, defaultHistoryContent)


- Jim

On 3/29/2014 3:26 PM, 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.
> 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