hoehermann / purple-signald

Pidgin libpurple bridge to signald.
GNU General Public License v3.0
153 stars 19 forks source link

Transition message handling to a formal state machine #16

Closed fancypantalons closed 4 years ago

fancypantalons commented 4 years ago

So... this is... a pretty major further re-plumbing and I'd totally understand if you might not want to accept this. :)

The primary message loop is already pretty complicated. There's an implicit state machine that's built into it that's just sort've threaded through the code.

This is fine for now, but as I'm looking at the signald upgrade, I'm a bit concerned that it might need to get even more complex. It looks like there may be a need to call sync_groups and sync_contacts during initialization, and so we have even more steps.

So this refactoring formalizes the message handling into an actual state machine.

The state machine is constructed of nodes, each of which has a name and a hash of state transitions that can occur from that node.

The transition carries with it:

The callbacks are split to allow for reuse.

The main event loop is then facilitated with state transitions from running to running for each of the message types that are supported.

Anyway, let me know what you think. This is a significant change, but I'm thinking we might want the flexibility in the future, and I like how it "unwinds" the main loop and makes the state machine explicit.

As you can see, this basically guts most of signald_handle_input, so libsignald.c is even shorter now.

And as a bonus, the debug logging is a lot more useful since the transitions are explicit and logged.

I've tested linking, regular startup, and some basic messaging activities and it all seems to work nicely.

Feedback is welcome!

hoehermann commented 4 years ago

As you already suspect, my gut says, this is over-engineered and therefore I do not like it.

I try to be as close to whatever the API exposes and as far as I know, purple's existing "state machine" is strictly linear (with the states being "disconnected", "connecting" and "connected"). Other states such as "waiting for link to complete" do already exist, albeit implicitly (as in "current state is 'connecting', but waiting for the callback to happen").

In my eyes, managing all states explicitly imposes a management and maintenance overhead. For the debugging developer, it might be nice to have explicit states, but for the feature-adding developer, it might be annoying. I do not see any significant advantages for the end-user. I see you use the explicit state machine to query the contacts. Is this beneficial for the group chats in any way?

fancypantalons commented 4 years ago

As you already suspect, my gut says, this is over-engineered and therefore I do not like it.

I try to be as close to whatever the API exposes and as far as I know, purple's existing "state machine" is strictly linear (with the states being "disconnected", "connecting" and "connected"). Other states such as "waiting for link to complete" do already exist, albeit implicitly (as in "current state is 'connecting', but waiting for the callback to happen").

In my eyes, managing all states explicitly imposes a management and maintenance overhead. For the debugging developer, it might be nice to have explicit states, but for the feature-adding developer, it might be annoying. I do not see any significant advantages for the end-user. I see you use the explicit state machine to query the contacts. Is this beneficial for the group chats in any way?

Yup, certainly a tradeoff and a valid critique. It was a fun little experiment, but I'm fine setting the work aside. :)

As for moving the contact query into the state machine, it's not specifically in support of group chats.

Rather, that was a step toward adding calls to Signal _sync methods at startup, which the latest signald supports. I've noticed timeliness issues with group state not getting updated in signald, and explicitly syncing state at startup will help with that.

But there's obviously other ways to solve that problem!