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

Rework statemachine #139

Merged
merged 48 commits into from
Oct 15, 2021
Merged

Rework statemachine #139

merged 48 commits into from
Oct 15, 2021

Conversation

Grazfather
Copy link
Collaborator

@Grazfather Grazfather commented Sep 25, 2021

This reworks the statemachine as imagined by @eccentric-j.

It keeps the state machine static, only uses 2 atoms: 1 for current state and one for context.

Each state defines a transition function for each action it supports. These functions return a new state, a new context, and an effect. Effects are sent to all subscribers, along with the new state and new context. They cannot modify the state or context, but can instead use it. The intent here is to allow these 'effect handlers' to display modals, set up hot keys, etc.

Additionally, we add a helper effect-handler, which is a higher order function that takes a map of effect->handler, and closes over an atom. The handlers provided in this way should return their own cleanup function, which is stored in the atom. This allows the returned function to be registered as an effect handler and to have the returned cleanup function automatically called on the subsequent event.

  • Add tests
  • Port modal over
  • [ ] Add modal tests
  • [ ] (Maybe?) Integrate new subscriber/stream
  • Dog food (a week+?)
  • Port apps over
  • Port vim over
  • Remove debug prints
  • Replace statemachine with new statemachine, modal with new-modal, apps with new-apps

Copy link
Collaborator

@jaidetree jaidetree left a comment

Choose a reason for hiding this comment

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

It's off to a very strong start! Feels like 80% there. These could also be used to power the frp stream library I'm slowly working on so I'm looking forward to having this merged.

What are your feelings towards renaming it from new-statemachine to statemachine2.fnl and merging it when the API + tests are done, then replacing the old library with it in a separate PR?

lib/new-statemachine.fnl Outdated Show resolved Hide resolved
lib/new-statemachine.fnl Outdated Show resolved Hide resolved
lib/new-statemachine.fnl Outdated Show resolved Hide resolved
lib/new-statemachine.fnl Outdated Show resolved Hide resolved
Copy link
Collaborator

@jaidetree jaidetree left a comment

Choose a reason for hiding this comment

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

Tests look good, only minor copy changes

test/new-statemachine-test.fnl Outdated Show resolved Hide resolved
test/new-statemachine-test.fnl Outdated Show resolved Hide resolved
lib/new-modal.fnl Outdated Show resolved Hide resolved
@Grazfather Grazfather marked this pull request as ready for review September 26, 2021 17:04
There is no timeout state so the name makes no sense.
jaidetree
jaidetree previously approved these changes Sep 26, 2021
Copy link
Collaborator

@jaidetree jaidetree left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice job! Will let it sit to give Ag time to take a look before merging.

We want to do this so that once we leave an app the state of the machine
has :app set to nil, otherwise the modal menu will continue to display
items from the previous app.

Because we are still firing an effect, we need to guard around the
effect handlers, doing nothing when we have an enter or launch effect
but no app.
@jaidetree
Copy link
Collaborator

Thoughts on renaming signal to send?

@agzam
Copy link
Owner

agzam commented Oct 10, 2021

Thoughts on renaming signal to send?

Yeah, I do too, feel that "signal" is a bit ambiguous - "is it a noun; a verb?". "Send" doesn't seem to be a great fit here either. I think what we're looking for is the equivalent for "emit", and "emit" and "broadcast" feel a bit too pretentious to me. We're not dealing with a distributed database of some sort, right?

I started looking for synonyms, and all that I could come up with, are a bunch of lousier variants. Things like: communicate, convey, deliver, emanate, enunciate, percolate, radiate, promote, propagate

Maybe, keeping up with the tradition of Lisp, we should use something bare elemental? Something like "shout" or even "yell"? 😆

They say "naming is the hardest thing in software". Naming itself may not so much, but explaining to other people why a particular name was chosen - that's tough.

@Grazfather
Copy link
Collaborator Author

transmit?

I agree signal can come off as a noun. I will just say that I don't really have a horse in this race, I'll let you guys decide what you want and I can make the change.

@jaidetree
Copy link
Collaborator

They say "naming is the hardest thing in software". Naming itself may not so much, but explaining to other people why a particular name was chosen - that's tough.

Agreed. Don't want to bikeshed too much on it, but names have their importance too. The reason I brought this up was because upon reviewing the code again I didn't recognize it.

I proposed send because it's used in a popular JS state machine library that inspired this design. https://xstate.js.org/docs/guides/start.html.

It seems to me send encompasses the intention: send an event (or action) to the state machine which may or may not cause a transition. We can guarantee it will be sent, like the way we can send an email, but we cannot guarantee the statemachine will respond to it, depending on its current state, similar to how the recipient of said email may not respond or act upon it. Ideally I'd like to land somewhere between something intuitive and jargony and send feels like a reasonable choice given it communicates that a known event occurs and the fsm is responsible for handling it from that point forward.

@agzam
Copy link
Owner

agzam commented Oct 12, 2021

It seems to me send encompasses the intention: send an event (or action) to the state machine which may or may not cause a transition.

I was thinking of what would be the term used in finite automata theory, and I couldn't find any. There are "states", states can "accept" or "reject". There are "transitions" and "actions", but there seems to be no commonly agreed term for a general action of transitioning.

The descriptive name perhaps would be trigger-transition but that's too verbose, and we're not writing Java.
In the real-life machine, that thing would be called maybe "shift", but that looks maybe even more confusing than "signal".

I guess send does fit in here. And in the case of someone questioning the name, we'd just say "git blame" it and hope they'd find this exchange. Or better yet, let's make it a docsting: "Sends an event (or action) to the state machine which may or may not cause a transition."

@Grazfather
Copy link
Collaborator Author

Done

jaidetree
jaidetree previously approved these changes Oct 15, 2021
Copy link
Collaborator

@jaidetree jaidetree left a comment

Choose a reason for hiding this comment

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

Caught some minor naming nitpicks, other than that the implementation looks really solid. Nice work!

lib/apps.fnl Outdated
(fn []
(unbind-keys)))))

(fn my-effect-handler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor change but could use a better name. Some options that come to mind:

  • app-effect-handler
  • map-effect-handler
  • named-effect-handler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! I forgot about this one

lib/apps.fnl Outdated

(fn proxy-actions
[fsm]
(fn action-watcher
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we discussed naming functions after verbs. Though this is pretty concise now maybe it would benefit from being an inline function?

lib/modal.fnl Outdated
@@ -54,7 +54,7 @@ switching menus in one place which is then powered by config.fnl.
nil))))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Event Dispatchers
;; Action senders
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps there's a better phrase that suggests "Here's some public API functions you can call whenever"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't export these but I can name this heading better.

_action (atom.new nil)
_effect (atom.new nil)
_extra (atom.new nil)]
(fsm.subscribe (fn [{: prev-state : next-state : action : effect : extra}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminds me to make an it-async that takes a callback function to call after the tests are performed. Then instead of all these atoms we can perform the tests in the subscribe callback itself.

We could try performing those tests in the callback, it might work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does!

Copy link
Collaborator

@jaidetree jaidetree left a comment

Choose a reason for hiding this comment

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

Looks good!

@Grazfather Grazfather merged commit 85d6f26 into master Oct 15, 2021
@Grazfather Grazfather deleted the new-fsm branch October 15, 2021 16:14
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.

3 participants