- From: Ate Douma <ate@douma.nu>
- Date: Wed, 02 Apr 2014 11:27:17 +0200
- To: www-voice@w3.org
On 02-04-14 00:10, Jim Barnett wrote:
> 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.
Right. And I realize I already solved this differently, and IMO elegantly, in my
implementation :)
The 'hint' leading to my implementation was the fact that
addDescendantStatesToEnter (also) stores the state parameter in statesToEnter.
And is expected to do so as it recursively is invoked for derived states which
were not yet initially added to the statesToEnter.
What I thus did instead is the following (in pseudo language):
procedure computeEntrySet(transitions, statesToEnter, statesForDefaultEntry)
enterableTargets = new OrderedSet()
historyTargets = new OrderedSet()
for t in transitions:
for s in getTargetState(t.target)):
if isHistoryState(s):
historyTargets.add(s)
else
enterableTargets.add(s)
for s in enterableTargets:
addDescendantStatesToEnter(s,statesToEnter,statesForDefaultEntry)
for h in historyTargets:
addDescendantStatesToEnter(h,statesToEnter,statesForDefaultEntry)
// unmodified below
for t in transitions:
ancestor = getTransitionDomain(t)
for s in getTargetStates(t.target)):
addAncestorStatesToEnter(s, ancestor, statesToEnter,
statesForDefaultEntry)
This way, no history state is added to enterableStates because
addDescendantStatesToEnter only does this for non-history states.
Note that addAncestorStatesToEnter will work just fine with a history state as
parameter.
Finally, concerning computeExitSet: this also isn't a problem in my
implementation because I already 'compile' the source of a history transition to
that of its parent state, and actually (thus) can 'compile' the transitionDomain
and the targets of a transition at document load time.
In pseudo-language in the algorithm, this also can easily be catered for if you
update getTransitionDomain as follows:
function getTransitionDomain(t)
tstates = getTargetStates(t.target)
if not tstates
return t.source
state = t.source
if isHistoryState(t.source):
state = t.source.parent
if t.type == "internal" and isCompoundState(state) and tstates.every(lambda
s: isDescendant(s,state)):
return state
else:
return findLCCA([state].append(tstates))
The above can further be optimized by filtering out the history targets from
being passed to the lambda expression or from the findLCCA(stateList), but it
isn't necessary either.
With the above minor improvements, the getTargets(transition) as well as the
getTransitionDomain(transition) procedures can remain 'stateless' (not history
state aware) and can be compiled at load time.
If you add history dereferencing to getTargets(transition) this no longer will
be possible, so IMO is something we should try not to do.
>
> 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.
See my above proposed changes, AFAICT it solves 1) elegantly and then 2) no
longer is an issue (already solved by the earlier update to the algorithm).
Regards,
Ate
>
> - 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
>>>
>>>
>>
>>
>
Received on Wednesday, 2 April 2014 09:27:49 UTC