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

Allow split state definitions #29

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SpaceBison
Copy link

Consider the following state machine:

StateMachine.create<String, Int, Nothing> {
	initialState(STATE_A)

	state(STATE_A) {
		on(EVENT_1) {
			transitionTo(STATE_B)
		}

		onExit(firstDefinitionOnExitListener)
	}

	state(STATE_A) {
		on(EVENT_2) {
			transitionTo(STATE_B)
		}

		onExit(secondDefinitionOnExitListener)
	}

	state(STATE_B) {
		onEnter(firstDefinitionOnEnterListener)
	}

	state(STATE_B) {
		onEnter(secondDefinitionOnEnterListener)
	}
}

Currently, trying to transition from STATE_A on EVENT_1 will result in a valid transition, but attempting a transition on EVENT_2 will result in an invalid transition. There's no warning or error given neither for the second STATE_A nor the second STATE_B definitions. A similar behavior can be observed for on exit and on enter listeners - only firstDefinitionOnExitListener and firstDefinitionOnEnterListener will be called when transitioning from STATE_A to STATE_B.

This PR aims to make state machines take into account all the state definitions passed to the graph builder. This is achieved by merging all applicable state definitions when resolving a transition for a given state and event instead of just taking the first one that matches.

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2020

CLA assistant check
All committers have signed the CLA.

@timzaak
Copy link

timzaak commented Jun 30, 2020

This would be great.

// I can handle an event in one place  for more than one state.
  state(StateMachine.Matcher.any<State, State>().where {
            !(this is State.MachineError)
        })

Copy link

@JaviRpo JaviRpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is perfec to me.

@@ -51,7 +51,14 @@ class StateMachine<STATE : Any, EVENT : Any, SIDE_EFFECT : Any> private construc
private fun STATE.getDefinition() = graph.stateDefinitions
.filter { it.key.matches(this) }
.map { it.value }
.firstOrNull() ?: error("Missing definition for state ${this.javaClass.simpleName}!")
.also { if (it.isEmpty()) error("Missing definition for state ${this.javaClass.simpleName}!") }
.fold(Graph.State<STATE, EVENT, SIDE_EFFECT>()) { acc, state ->
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, this also works when we want to have a generic transition

private val stateMachine = StateMachine.create<String, Int, Nothing> {
                initialState(STATE_A)
                state(STATE_A) {
                    on(EVENT_1) {
                        transitionTo(STATE_B)
                    }
                }
                state(STATE_B) {
                    on(EVENT_2) {
                        transitionTo(STATE_A)
                    }
                }
                state<String> {
                    on(EVENT_3) {
                        transitionTo(STATE_C)
                    }
                }
            }

for example, this one, having state<String> or state(any()) we can move from any state to another state with the same event.

@JaviRpo
Copy link

JaviRpo commented Jul 1, 2021

What do we need for this PR to be merged and have a version with this change?

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

Successfully merging this pull request may close these issues.

4 participants