Implement Websocket and Named Pipe connection and some other QoL#53
Implement Websocket and Named Pipe connection and some other QoL#53Oekn5w wants to merge 29 commits into
Conversation
|
Ok, I've only had a cursory look but the first thing that sticks out to me is that this is no longer installable on Linux at all due to the unconditional addition of I'd like to know if you used AI for any of this because there's some stuff that doesn't quite make sense. I'll give a quick set of review comments (because I can't even load this with the |
|
All hand-written in this here. Although I'm not very familiar with UV or python packaging in general (more doing stuff on the C/C++ side). Is there a way to define the pywin stuff as conditional? Or do we only need to to make the import and pywin stuff platform-guarded (along with the settings selection)? |
| """ | ||
| if not self.sock: | ||
| self.connect() | ||
| if not self.ensureConnected(): |
There was a problem hiding this comment.
camelCase is only for things that you have to use it for (in this case, calling things from PySide that use camel case). For attributes and methods we define, use snake_case.
There was a problem hiding this comment.
This may just seem like a python nitpick thing but in this project it helps me know when I'm calling something defined by PySide (that's probably pretty close to something in C++) or when I'm calling something internal to SplitGuides.
There was a problem hiding this comment.
I'll glance over this again, but I think I adhered to snake_case now
| If you are already using the Livesplit Server for something else, | ||
| this appliction shouldn't interfere with that, but only a TCP or a Websocket server can be running at a time. | ||
| You can change the connection to Livesplit that will be used in the settings. | ||
| There are 3 options: |
There was a problem hiding this comment.
I'd rather not give the user any options and instead just try/fail the different options internally.
There was a problem hiding this comment.
I've now added a connection logic cycling through the 2 (or 3 if applicable) methods, although I haven't updated the readme yet
|
Oh and if you're using a type checker and it gives errors, yeah the typing isn't always great in this project as it predates me consistently using a type checker. One day I'll go and fix things (but please don't do it in this PR). |
|
Ok, so I'll point out a number of small changes but there's also a larger structural change that will clean things up. WRT the tests being broken - to run the tests locally, you should just do |
| class ConnectionWS(ConnectionTypeBase, Prefab): | ||
| server: str = "localhost" | ||
| port: int = 16834 | ||
| timeout: int = 4 # 1 second not enough to establish a connection |
There was a problem hiding this comment.
Is one second seriously not long enough to form a websocket connection to localhost??
There was a problem hiding this comment.
yeah, 1 second didn't create a connection, at least with this python implementation. All in all, I'm not too happy about the failure behavior of Websocket here, the loops are taking ~16 s before the next reconnection attempt is made, I'll try to look into why that is the case even with the 4s timeout
There was a problem hiding this comment.
I know Python's usually seen as slow but it's not that slow for something like this. I'd almost rather not have websocket support if it's actually that slow.
| try: | ||
| self.ws.send(msg) # no CRLF on Websocket |
There was a problem hiding this comment.
In a number of these places we should check if ws is None before trying to access methods. Sure it'll raise an AttributeError but really the exception handling should be specific to the connection failing rather than it not existing in the first place.
| if sys.platform != "win32": | ||
| raise NotImplementedError('ConnectionPipe class only on Windows') |
There was a problem hiding this comment.
The trick here is that ConnectionPipe should only be defined on windows - and probably live in a separate file. This is part of the larger change.
With that done you no longer need to check the platform inside the class on every method.
| self.list_connection_types = [CONNECTIONTYPE_TCP, CONNECTIONTYPE_WS] | ||
|
|
||
| def is_connected(self) -> bool: | ||
| return (bool)(self.connection_obj) |
There was a problem hiding this comment.
This looks very C, the parentheses around bool are unnecessary in Python.
| stable_retry : int = attribute(default=0, init=False, repr=False) | ||
| next_attempt_idx : int = attribute(default=0, init=False, repr=False) | ||
| connection_obj : ConnectionTypeBase | None = attribute(default=None, init=False, repr=False) | ||
| list_connection_types : list = attribute(default=[], init=False, repr=False) |
There was a problem hiding this comment.
| list_connection_types : list = attribute(default=[], init=False, repr=False) | |
| list_connection_types: list = attribute(default_factory=list, init=False, repr=False) |
This is a python mutable default issue that dataclasses would have protected you from but I don't. This list would end up shared between every LivesplitConnection instance.
I don't think the errors that it causes come up in this case because you reassign the list instead of appending to it.
|
I'm going to make a branch with the larger structural change needed to complete this based on your branch. I'll try to make it clear what's going on in each commit. You can either then merge that in to continue the PR or I can try to finish it. |
This is to prepare to split off the windows-only elements into a separate file.
Check self.sock isn't None before trying to use it. (get rid of nasty red squiggles!) Move the `ping` logic into a separate static method. Close and set sock back to None if `ping` fails.
This will make it easy to force a specific connection for testing.
… connection hangs with the mocks in place.
All connection types now have hostname and port even if port isn't used for the pipe connection. Instead of trying a different connection type on each call to `.connect`, this tries them all in order and stops on the first successful one.
|
You can see my current fork here and see how it's restructured. https://github.com/DavidCEllis/SplitGuides/tree/servertypes-edit Tests are still failing due to the different connection types and the ping check. Hopefully it actually works - testing in a VM indicated that the named pipe connected at least! |
|
Thanks, yeah this makes a lot of sense to do, I'll continue on nailing down the expected Exceptions when the Livesplit becomes unavailable at various states. With these changes the times on the websocket connection were only 1-2 seconds above the timeout defined. |
|
I've updated the tests for the now expected outcomes, using your branch. Also I was testing the connections a bunch in different scenarios. Probably a few new test should be written still for the other connections |
2 participants
Resolves #30
This change initially started out just for implementing the Websocket support, but has grown a bit in scope. The individual commits should still be fairly complete for the individual features.
New features:
pywin32websocket-clientFixes:
QoL:
The Named Pipe should probably only be selectable and guarded by a platform check in code, but I don't have a Linux environment set up to see how that could work.
For now I've made the current TCP the default selection.