Refactor transport-level code in preparation for TCP support #47
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
There is a bunch of transport-agnostic logic in osc.transport.udp that should be shared with the upcoming TCP implementation. The plan after this is to move UDP-specific things back down into osc.transport.udp. Then, implement tho corresponding TCP-specific bits in transport.tcp.
The data (a ByteBuffer) is an implementation detail of the transport. The higher level API is only concerned with sending and receiving OSCPackets, and doesn't need to worry about the underlying data unless something goes wrong.
I decided to remove some of the getter methods from OSCPort/OSCPortIn/OSCPortOut that concern themselves with transport-level implementation details. This resulted in compiler errors in the testPorts test case, as getRemoteAddress no longer exists. I thought about this, and I think that removing getRemoteAddress makes sense in the context of real world usage. I don't think it makes sense to keep it just to facilitate having these tests, which are only testing that the default values we specified are the values you get when you don't provide a value. I'm open to feedback on this - happy to add this back if needed!
Merged
|
Closing this in favor of #52, which includes these commits and many more! |
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.
Background
See my comment here: #12 (comment)
To summarize, the current implementation of JavaOSC is UDP only, and there is a lot of transport-level code (all UDP-specific) in OSCPortIn and OSCPortOut.
I plan to implement TCP support soon, and I want for users to be able to continue to use the same public API, with only the small additional step of setting the network protocol to TCP, something like this:
As a first step towards that, this PR refactors / reorganizes the parts of the codebase that have to do with transport. The next step will be to simply add a TCPTransport class and OSCPort{In,Out}Builder methods to allow the user to specify the network protocol.
Summary of changes
BREAKING Move higher-level
transport.udp.OSCPort*classes up a level totransport.OSCPort*.Added a Transport interface that captures the low level implementation details.
Moved the current UDP transport implementation into UDPTransport, which implements Transport.
Adjusted the
toStringimplementations of OSCPort et al to include information about the Transport.OSCParseExceptions now include the data we attempted to parse
Feedback welcome! 🙂