ctm / mb2-doc

Mb2, poker software
https://devctm.com
7 stars 2 forks source link

Split login page off Lobby #1376

Closed ctm closed 5 months ago

ctm commented 6 months ago

Refactor the login page into App so that neither nick nor ids are Options.

Make it so all the state associated with logging in is in App, and only state associated with being logged in is in Lobby. This means github_code is entirely removed from Lobby.

Doing this allows us to use String to store the nick-candidate in App and Nick (not Option<Nick>) to store the actual Nick in Lobby. This should allow us to get rid of a bunch of places where we have to deal with Options that logically have to be Some at various places in the Lobby. Doing this before finishing my branch that adds more restrictions to nicknames (#302) allows us to have restrictions that only apply to the entire Nick and not disallow non-qualifying prefixes.

I did a bunch of work before creating this check-list. In fact, I tried an overly aggressive refactorization that I got fed up with. So, I put into a separate branch, then tried a more stepwise approach and got much further. Since that was going well, I deleted the aggressive branch. As such, some of the entries in this check-list are from memory and were already done as part of my step-by-step refactorization.

ctm commented 6 months ago

Turns out, putting state in App probably isn't the way to go. There are limitations on the top-level component that might bite us. So I'm renaming Lobby to LoginOrLobby and then moving most of its guts to either a new Lobby or the new Login. So, LoginOrLobby will have very little state of its own.

Another—and perhaps better—way to do it would be to have separate routes for the lobby and login pages, but I'm accustomed to each of them being top-level. If I change my mind, it'll be trivial to switch to separate routes and very little of the work that I'm doing for LoginOrLobby will need to be thrown away.

ctm commented 6 months ago

I did a bunch of work on this yesterday. Initially, I thought I could just rely on Rust's "belligerent refactorization" and I went in without a plan. That did not go well, although I doggedly pursued it for longer than I should have. I've been running sleep deficits every day since Wednesday, and that only encouraged my stubbornness. Sunk cost fallacy, FTL. Luckily, I was at least awake enough to put my aggressive refactorization into its own branch and try a step-wise approach and that worked well.

By the time yesterday's evening tournament rolled around, I had two separate Login and Lobby components and the login process was switching between them, albeit with bugs. It was hard to go to bed knowing about the bugs, and it's hard not to just plow ahead and fixing stuff. In fact, I skipped my core exercise so I could work on this issue, but I chose to document the steps I worked on yesterday and lay out a series of steps for the future so that I could do things incrementally and have a sense of accomplishment as I checked off boxes, because today will be fragmented as well.

FWIW, Rust's belligerent refactoring really is a super-power, but not all refactorization attempts are equal.

ctm commented 6 months ago

Turns out, I didn't think about how our web socket is managed. With Lobby split off from Login, there's currently no way to transfer the web socket from Login to Lobby, which either means closing the Login web socket and opening a new one in the Lobby or having the web socket send messages to a common ancestor of both and then have those messages sent back down to the appropriate child.

I'll need to read some more Yew documentation and think about this a little bit. I doubt there's much overhead with passing it up and back down again, although that kind of rubs me the wrong way. Another possibility, I think would be to use some sort of trait object shim in ReconnectingSocket and simply swap that.

I think I'll do a trait object spike before I do anything else.

ctm commented 6 months ago

I did not need to use a trait object. When I initially wrote the code that eventually turned into ReconnectingWebSocket, I didn't fully grasp what a Callback was in Yew and I was essentially replicating that functionality in a way that forced ReconnectingWebSocket to be generic. By using Callback "properly", only the constructor for parent_sender needs to be generic, so, now that ReconnectingWebSocket is no longer generic, I can transfer it from login to lobby, as long as I change the parent. Duh.

Sleep is a good thing, as is revisiting code that was written in my Rust infancy days.

ctm commented 6 months ago

Ugh. I can not come up with a way to pass ReconnectingWebSocket from Login to Lobby without introducing unacceptable complexity.

The problem is that if we have separate Login and Lobby Components that don't exist at the same time, we have to pass the ReconnectingWebSocket up to a common ancestor. However, ReconnectingWebSocket currently assumes that it can talk to some Component to announce, for example, that the connection failed. So either the common ancestor also needs to be able to deal with a ReconnectingWebSocket message, either by presenting the same info to the user or by caching the messages to then forward to the Lobby. Furthermore, if we pass the ReconnectingWebSocket to the Lobby via a property, then we have to wrap it in a reference counter and some locking provider of interior mutability. Ick.

If we're willing to have Login and Lobby exist simultaneously, then we could, in theory, pass the ReconnectingWebSocket directly from Login to Lobby, but that would mean that the Lobby needs to start out with an Option<ReconnectingWebSocket> rather than a WebSocket. Ick.

So, I've renamed login-1376 to login-1376-login-or-lobby and am putting it aside indefinitely. Instead, I'll explore the possibility of introducing a LoginState enum that would have a LoggedOut variant with a nick_candidate String and a LoggedIn variant with a nick Nick. That should make the current Lobby code clearer and also allow me to finish the other Nick related issues (#1366 and #1367).

ctm commented 5 months ago

Turns out, rather than adding a LoginState, I updated ConnectionState to include what I had envisioned in my last comment. That code is working fine in login-1376,but I'm not going to deploy it until I've added more Nick validity checks, because it leaves the client more permissive than the server. So, I won't deploy until I've also added more restrictions to nicknames (#1367), which I'm working on today.

So now, the ConnectionState's LoggedOut variant carries a LoggingIn struct which has a nick_candidate field that is a NickPrefix and the NickPrefix is a newtype whose interior type is Either<Nick, String>, and that String is the only place a not-fully-formed Nick can exist and it doesn't even exist except when connection_state is LoggedOut.

I'm mildly disappointed that my refactor doesn't go further, because there's easily a lot more that can be done, but I've spent far too long on nickname related issues, mostly due to the fragmentation of my time associated with my mother's stroke, which was fifteen days ago.

ctm commented 5 months ago

Deploying shortly.