Move clientserver package into a new subproject #4751
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwvanderlinde
added
the
code-maintenance
kwvanderlinde
changed the title
kwvanderlinde
force-pushed
the
refactor/3777-separate-client-server-project
branch
from
20c9ce1
to
50f09bc
Compare
Since the clientserver code does not understand what a `Player` is, we can't have `Handshake.getPlayer()`. Since this method is only used called on `ServerHandshake`, the method has been moved there and removed from `Handshake` and `ClientHandshake`. Additionally, the `Handshake` interface does not need to inherit from `MessageHandler`. Implementations can be `MessageHandler` if they need to be (and they current do), but nothing about the handshake process inherently requires this to be true.
Both `WebRTCConnection` and `WebRTCServer` have had the following changes made: 1. Instead of accepting a `ServerConfig`, they both accept a `String` server name. This was the only part of the config used in the WebRTC code. 2. Instead of calling `MapTool.stopServer()` and `MapTool.showError()`, they now both have `Listener` interfaces that communicate the events that needed these actions take. The `ConnectionFactory` is responsible for hooking these up to the `MapTool` methods. I'm not a big fan of this approach, but I wanted to avoid complete reworks of server initialization and connection management, so this is what I went with. Also a bunch of `if` statements were gifted braces.
This avoids putting `PeerConnectionObserver` and `RTCDataChannelObserver` in the API of `WebRTCConnection`. Not doing this will require subtle dependency requirements once in a separate project.
Left behind was `ConnectionFactory` as it is really a MapTool-specific tool for building connections and servers rather than a part of the general client-server logic itself. Also pulled in was `Handshake` and `HandshakeObserver` since they are used in the client-server and are an important part of the process (the implementations were left behind though). Common build configurations wered moved into a `shared.gradle` file applied to the root project and subprojects. Dependencies specific to the clientserver subproject were moved out of the root project to keep things clear.
kwvanderlinde
force-pushed
the
refactor/3777-separate-client-server-project
branch
from
50f09bc
to
dca7107
Compare
cwisniew
approved these changes
Apr 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
First step on #3777
Description of the Change
This moves almost the entirety of the
clientserverpackage into a newclientserversubproject. A bit of work was need to avoid certain references to outside ofclientserver, but it's mostly mechanical transformation.These seem are the main things to call out:
HandshakeandHandshakeObserverare now part ofclientserveras they were part of its interface already, just located elsewhere.WebRTCConnectionno longer directly implements thePeerConnectionObserverandRTCDataChannelObserverinterfaces, instead providing nested classes for that. Without this change, theclientserverproject would have to leak its dependency on thewebrtclibrary.WebRTCConnectionandWebRTCServer, rather than require a MapToolServerPolicywe now require just aStringserver name, since that was all that was needed.WebRTCConnectionandWebRTCServer, there were direct references to theMapToolclass in a couple of error handling cases. To avoid this dependency, I addedListenerinterfaces to both so the main project can instead provide these calls. I don't especially like this, but I don't think it's worth trying to find a better solution in this PR. I have some more general ideas for reworking client & server code that would allow better handling of these cases, but that will be its own thing.The only thing left behind in the main project was
ConnectionFactorysince that is really MapTool-specific logic rather than a part of the genericclientservercode.Possible Drawbacks
Should be none, functionality is identical.
Documentation Notes
N/A
Release Notes
clientserverlibrary into a subproject.This change is