Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A call to Load should notify extensions for Initializing, Initialized and SwitchedState #21

Open
djack42 opened this issue Nov 29, 2017 · 9 comments
Assignees

Comments

@djack42
Copy link

djack42 commented Nov 29, 2017

Hi,
I'm using your StateMachine implementation with extensions and I have a couple of problems :

  • The StateMachine does not notify at all the extensions at Load()
    the initialized flag is set to true but InitializingStateMachine or InitializedStateMachine aren't called on extensions.

  • A call to LoadCurrentState() set the backing field currentState instead of the Property CurrentState, SwitchedState is never called on extensions.

Please, can you fix this behavior and update the nuget package ?

@djack42 djack42 changed the title A call to Load should notify extensions for Initializeing, A call to Load should notify extensions for Initializing, Initialized and SwitchedState Nov 29, 2017
@ursenzler
Copy link
Member

Thanks for your message.
I‘ll take a look at it tomorrow and add the missing calls on the extensions.

@ursenzler
Copy link
Member

I'm arguing with myself whether the state machine should call InitializingStateMachine, InitializedStateMachine and SwitchedState or a new Loaded method.

The reason is that the state machine does not really initialize itself, nor does it switch a state. It is loaded.

@djack42 would a Loaded method work for you, too?

@ursenzler ursenzler self-assigned this Dec 1, 2017
@djack42
Copy link
Author

djack42 commented Dec 1, 2017

Thank you very much for taking the time.

I like the idea of a new extension method Loaded !
Of course, as it is, the loaded state should not execute entry and exit actions again as the Save works on the CurrentState, set after the transition is played. Only the configured events should be availlable.

There could be a problem with the initialStateId after a Load on a new StateMachine,
it can't be restored by Load because Save doesn't deal with it. A call to Report would use it uninitialized.
Maybe another way to deal with Save and Load would be to pass a struct containing every StateMachine's properties not set by the configuration calls In(TState) and DefineHierarchyOn(TState) instead of the current state.

@ursenzler
Copy link
Member

Okay, I'll add the Loaded method as a quick fix.

There are other problems with the loading mechanism that I found, e.g. loading a non-initialized state machine sets it to initialized without an active state :-O

@djack42
Copy link
Author

djack42 commented Dec 1, 2017

Thank you

@ursenzler
Copy link
Member

@djack42 can you please try the pre-release package here and report back whether this fixes your issues?
https://www.nuget.org/packages/Appccelerate.StateMachine/4.5.0-pre0000

Thanks.

@djack42
Copy link
Author

djack42 commented Dec 1, 2017

I tried, Loaded is called as expected and it's great.
The schemaLocation in the Yed report is not fixed yet apparently.

But to properly initialize my extension, the loadedCurrentState passed as parameter is a Initializable<TState> is not enough. I suggest two evolutions :

  • An IState<TState, TEvent> (null if not initialized) like for most of the extension methods.
    It would also be great if all the extension methods with a TState parameter were translated as an IState<TState, TEvent>, there is a bunch of those.
    For the ref TState, a complete list of IState<TState, TEvent> would be required leading to the next evolution.
  • Some tool on the IStateMachineInformation<TState, TEvent> to translate a TState into the corresponding IState<TState, TEvent>.
    I'm using some hack with the a fake IStateMachineReport<TState, TEvent> to extract those IState<TState, TEvent> because the method Report is the only way I found to get that translation (void Report(string name, IEnumerable<IState<TState, TEvent>> states, Initializable<TState> initialStateId)) and then linq into states for the one wanted. Such a pain...

If both were to be implemented, I think i'd be enough.
I'm working a lot with this tool these days and I think I will try to contribute directly with pull requests if it's not a hassle :-)

@ursenzler
Copy link
Member

partially fixed in #24 which is on master and available as a pre-release package.
IState instead of TState not done yet.

@djack42
Copy link
Author

djack42 commented Jan 2, 2018

thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants