Align web socket commands with LiveView server #397
Conversation
src/ui/TimerView.tsx
Outdated
| case "togglepause": this.togglePauseOrStart(); break; | ||
| case "undo": this.undoSplit(); break; | ||
| case "skip": this.skipSplit(); break; | ||
| case "pause": this.togglePauseOrStart(); break; |
There was a problem hiding this comment.
I'd prefer this to either stay togglepause or for pause to be a (deprecated) alias.
There was a problem hiding this comment.
Yeah that's a good idea, I'll fix that up 👍
|
Considering there's all sorts of new functions dedicated entirely to the WebSocket communication, I'd probably also would want to see all of this logic moved out into a new source file. |
|
@CryZe I've re-worked this a bit as you suggested, hopefully this is looking a bit better now. Let me know if you'd like any other changes or would like to try things another way. Cheers 👍 |
src/ui/TimerView.tsx
Outdated
| @@ -353,4 +368,4 @@ export class TimerView extends React.Component<Props, State> { | |||
| private resumeGameTime() { | |||
| this.writeWith((t) => t.resumeGameTime()); | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Can we make sure that there are newlines at the end of files?
src/util/WebSocket.ts
Outdated
| } else { | ||
| return parts[0] + ":" + parts[1].slice(0, parts[1].length - 1); | ||
| } | ||
| } |
src/util/WebSocket.ts
Outdated
| } else { | ||
| return ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
Couldn't we just do:
return phases[phase] || "";
There was a problem hiding this comment.
Ideally we should use expect(phases[phase], "Unknown timer phase supplied") here or so, as the index out of bounds case should visibly explode and not implicitly do something weird.
src/util/WebSocket.ts
Outdated
| return "0" + parts[0] + ":" + parts[1].slice(0, parts[1].length - 1); | ||
| } else { | ||
| return parts[0] + ":" + parts[1].slice(0, parts[1].length - 1); | ||
| } |
There was a problem hiding this comment.
This method seems really hacky. Could we either modify formatLeaderboardTime to take additional arguments or just add an additional method to TimeUtil?
|
While you've moved some functionality over into WebSocket.ts, I'd actually like to see all the WebSocket logic be moved over. There should be close to nothing left in the TimerView. |
|
@CryZe Sorry this has taken so long, I've been a bit busy lately, but I've created a new WebSocket manager class which handles the web socket setup and communication. I think this is looking a bit cleaner now, but let me know if there are any further changes required. 👍 |
Currently the web socket commands are not named the same way as those used by the LiveSplit server component.
I have written a script to connect the Android Remote app to LiveSplitOne (here) but as some of the commands are different and/or not supplied it is tricky to get this working. In order for more tools to be able to interface better with LiveSplitOne I think it would be good to have standardized commands that are the same as the desktop app.
I am fairly new to this code base, so let me know if anything needs to change or could be done differently, I'm open to feedback and different ideas of how this should be done. This is currently more of a POC, but I'd be happy to help bring this to LiveSplitOne.