search

hoijui / JavaOSC

OSC content format/"protocol" library for JVM languages
http://www.illposed.com/software/javaosc.html
BSD 3-Clause "New" or "Revised" License
156 stars 43 forks source link

Provide a lower-level API for packet handling #38

Closed daveyarwood closed 5 years ago

daveyarwood commented 5 years ago

This solves the use case that I laid out in #36.

NB: This PR is built on top of #37 which includes the java8 branch, though I'm happy to rebase on top of develop and pull out the java8 part if desired.

Feedback welcome!

hoijui commented 5 years ago

... ouh and yeah, I think that in the end, until a release, java8 should not be in develop yet, but leave it there till we both are happy with this PR.

daveyarwood commented 5 years ago

Hey, thanks for the feedback! Just wanted to let you know that I've been super busy lately, but I will fix up this PR soon based on your review, whenever I get a bit of free time.

hoijui commented 5 years ago

ooook :-) btw, I quite convinced myself by now, that in order to supply the default packet listener, it would make most sense to have an OSCPortInFactory class, that creates and registers it, so we can leave any default stuff out of OSCPortIn, and thus leave it a cleaner class.

daveyarwood commented 5 years ago

Sounds reasonable to me! I'll take a stab at it.

On Thu, Jan 10, 2019, 6:08 AM hoijui <notifications@github.com wrote:

ooook :-) btw, I quite convinced myself by now, that in order to supply the default packet listener, it would make most sense to have an OSCPortInFactory class, that creaates and registers it, so we can leave any deafult stuff out of OSCPortIn, and thus leave it a cleaner class.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hoijui/JavaOSC/pull/38#issuecomment-453058346, or mute the thread https://github.com/notifications/unsubscribe-auth/AEroF-hD2ERJ5CO_GEvCQXxtoLZyxQs-ks5vBx8XgaJpZM4Zlu03 .

hoijui commented 5 years ago

cool, and .. thanks :-)

daveyarwood commented 5 years ago

@hoijui After playing around with this a little bit, I think the Builder pattern might be cleaner.

Consider the following pseudocode:

// The traditional way, using the default packet dispatcher and message
// listeners
OSCPortIn port = new OSCPortIn.Builder()
                              .setPort(12345)
                              .addMessageListener(messageListener1)
                              .addMessageListener(messageListener2)
                              .addMessageListener(messageListener3)
                              .build();

// The new "packet listener" way that I'm implementing
OSCPortIn port = new OSCPortIn.Builder()
                              .setPort(12345)
                              .setPacketListener(packetListener)
                              .build();

In build(), we could include logic about using the default packet dispatcher if the user did not set a custom listener via setPacketListener(...).

The Builder pattern would allow us to provide alternate ways of providing options without leading to a combinatorial explosion of constructors. I'm thinking in particular about how the local socket address can be provided as either a SocketAddress or an int, and also we allow the user to provide either 1 (same address for local & remote) or 2 SocketAddresses (local, remote). Here's how we could handle it with an OSCPortInBuilder:

// use the same SocketAddress for local and remote
new OSCPortIn.Builder().setPort(12345)...
// or, if the user wants to provide the address as a SocketAddress,
new OSCPortIn.Builder().setSocketAddress(address)...

// use different addresses for local vs. remote
new OSCPortIn.Builder().setLocalPort(12345).setRemotePort(67890)...
// or, if the user wants to provide SocketAddresses,
new OSCPortIn.Builder().setLocalSocketAddress(address1)
                       .setRemoteSocketAddress(address2)...

I'm happy to use the Factory pattern instead if that's your preference. Let me know your thoughts!

EDIT: This would also give us a clean way to handle providing one's own OSCParserFactory, vs. using the default one. The more I think about it, the harder I think it would be to use the Factory pattern for OSCPortIn for this reason.

daveyarwood commented 5 years ago

@hoijui I'm almost done fixing up the PR -- curious to hear your thoughts about OSCPortIn.Builder.

The one thing I haven't implemented just yet is the OSCPacketEvent class. That bit should be easy, I just have to run at the moment! :)

hoijui commented 5 years ago

I just saw the notifications about your messages now, and will not look at the code or anythign today (probably alos not tomorrow), but looks liek a very good idea! :-) thank you already!

daveyarwood commented 5 years ago

No rush! I think I've got this branch in a good place at this point. Whenever you have the time, let me know what you think.

daveyarwood commented 5 years ago

I think it makes sense to break out the builder into a separate class in a different file -- I'll make that change.

I don't think we really need to have an interface for OSCPortIn builders right now. I think it adds unnecessary complexity, vs. the simplicity of just having a single builder class. We could always add an interface in the future if we change our minds.

daveyarwood commented 5 years ago

@hoijui PR updated.

hoijui commented 5 years ago

awww.. looks good to me! :-) rebase on develop without java8 and we'll merge, I'd say! :-)

daveyarwood commented 5 years ago

@hoijui Cool. Done!

hoijui commented 5 years ago

.. am on it! I wanted to test compiling wiht Java 7 but.. I did not manaage to find a Java 7 JDK... so I figured it makes no sense to continue on this path, and I will switch to Java 8 right away. .. not forgottten. ;-)