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

suggestion on transition and condition, which should include Event #34

Closed
frogcjn opened this issue Nov 1, 2015 · 10 comments
Closed

suggestion on transition and condition, which should include Event #34

frogcjn opened this issue Nov 1, 2015 · 10 comments
Milestone

Comments

@frogcjn
Copy link

frogcjn commented Nov 1, 2015

There is a case that the event has associate values which cannot be enumerated all the possible values, so that it is not possible to add all Event Route into State Machine.

In this case, the only way to do it is to use condition.

But unfortunately, condition only includes transition which has no information about the event.

So I suggest to add Event information into transition.

@frogcjn
Copy link
Author

frogcjn commented Nov 1, 2015

For example:
.LoadAction(Int) Event
.CancelAction Event

.Pending State
.Loading(Int) State

.LoadAction event has an associate integer to suggest which task it wants to trigger.
.Loading state has an associate integer to suggest which task it is loading.

when the .LoadAction triggered, and the machine on .Loading state with the same associate number, then the Event should be ignored.

@frogcjn frogcjn changed the title suggestion on transition and condition, which should includes Event suggestion on transition and condition, which should include Event Nov 1, 2015
@inamiy
Copy link
Member

inamiy commented Nov 1, 2015

Hi, I added your above example as a XCTestCase in cc6bc8f.
Is this what you are looking for?

@frogcjn
Copy link
Author

frogcjn commented Nov 1, 2015

Yes.

Because transition has no information on event, we should add all possible events to the machine.

        for actionId in 1...100 {
            machine.addRouteEvent(.LoadAction(actionId), transitions: [ .AnyState => .Loading(actionId) ], condition: { $0.fromState != .Loading(actionId) })
        }

if
(1) I want the Int as greater as I want, or
(2) the associate value is a generic type from subclass's enum, or
(3) the associate value is a data

then the StateMachine cannot work on this kind of situations.
These situations have the same character: event's associate value cannot be fully enumerated by current class.

@inamiy
Copy link
Member

inamiy commented Nov 1, 2015

Oh, OK I got it!
You are right, and current typealias Condition = (transition: Transition) -> Bool has too few parameters to distinguish Event and other stuffs e.g. userInfo.

I think it should be something like:

typealias ConditionContext = (event: Event, transition: Transition, userInfo: Any?)
typealias Condition = ConditionContext -> Bool

And for this enhancement, we will need a major version bump.
Let me take some time to work on this 😉

@frogcjn
Copy link
Author

frogcjn commented Nov 1, 2015

Yes! Thanks!

@frogcjn
Copy link
Author

frogcjn commented Nov 1, 2015

And another suggestion is, can you use Optional.None to represent the meaning of AnyState or AnyEvent?
Because it needs code to add an nil init for an event, and it is ugly to add one more State though the fact the machine hasn't that State.

If there is a place needs nil to represent for the meaning of "AnyState" or "AnyEvent", just put "Event?" or "State?" as the parameter's type.

@inamiy
Copy link
Member

inamiy commented Nov 3, 2015

Thanks for above feedback as well!
This will also be another big breaking change, so I will refactor existing codes for both ideas :)

@inamiy inamiy added this to the 4.0.0 milestone Nov 3, 2015
@frogcjn
Copy link
Author

frogcjn commented Nov 3, 2015

Thanks:)

@inamiy
Copy link
Member

inamiy commented Nov 14, 2015

Sorry for late reply.
I just finished writing a rough draft for solving your above issues in #36.

Please check and tell me your thoughts 😉

@inamiy
Copy link
Member

inamiy commented Dec 10, 2015

This issue has been completed in #36 and ver 4.0.0.
Please see Ver 4.0.0 Release Notes for more detail.

@frogcjn Thanks again for your contribution! 🎉

@inamiy inamiy closed this as completed Dec 10, 2015
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