Skip to content

Extract class from stream player #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

HelgeStenstrom
Copy link
Collaborator

This is just a draft. It should not be merged.

I'm playing with the idea of splitting StreamPlayer into smaller classes. As a first try, I have created a new class Outlet (maybe a bad name, but it can be changed). The SourceDataStream and the associated controls (gain, pan etc) have been moved to the new class.
A few methods have been moved to the new class. More will follow. The methods that I'm moving are the ones that primarily work on the fields that have already been moved.

I have also made an interface with the public methods of StreamPlayer. The intention is to have a safety net, so that I don't change the de-facto interface (as seen by the application) by mistake.
But the separation can be better, if the interface can be changed, so that the application cannot call streamPlayer.getGain(), but has to call outlet.getGain() for the same purpose. If that is allowed, the wrapper methods in StreamPlayer that now calls methods in Outlet can be removed.

Again: this is experimental.
The application works as usual. Unit tests test were written before the refactoring, and work after it too.

See also: https://refactoring.com/catalog/extractClass.html

@goxr3plus
Copy link
Owner

I had exactly the same idea today in the morning, continue, i am watching the changes now. We want to maintain backward compatibility with the applications already using StreamPLayer if that is possible.

Also extracted flushAndStopOutlet, but it's not tested yet, so it will be moved when it's tested.
Test that paueses, to exercise flushAndStopOutlet()
@HelgeStenstrom
Copy link
Collaborator Author

I will not do intermediate pull request, but you can watch my branch.

@goxr3plus
Copy link
Owner

goxr3plus commented Sep 11, 2019

By the way passing XR3Player to Java 13 in September, and this project in Java 11, we can use multirelease so users can both use it ( Stream - Player) with Java 8 and Java11 and above, i haven't done it before but how hard it can be %)

@HelgeStenstrom
Copy link
Collaborator Author

I think this is ready to be merged now. There are a number of issues that I would like to deal with, but that is best done after this code is merged.

There are a lot of changes. I have done testing, and I haven't done any deliberate behavior changes, just refactorings that shouldn't change any behavior. But mistakes happen.
Tests are done mainly using the test class SourceDataLineTest.

The new class is called Outlet, and that is maybe not a good name. But it's easy to change later.

The StreamPlayer has the same de facto interface. I have also made an interface (in the Java sense), called StreamPlayerInterface, that StreamPlayer implements. The interface name is bad, I would rather call the interface SteamPlayer, but I didn't want to change the name of the implementation class at this time, as implementations would have to change how they create a new instance of the player.

@HelgeStenstrom HelgeStenstrom marked this pull request as ready for review September 11, 2019 20:04
@HelgeStenstrom
Copy link
Collaborator Author

There are a lot of commits. They should probably be squashed.

@goxr3plus
Copy link
Owner

We will change the names before the next release and show a tutorial on the read me for new users :)

@goxr3plus
Copy link
Owner

I read most of it, merging, we need to go through good testing.

@goxr3plus goxr3plus merged commit 634c991 into goxr3plus:master Sep 11, 2019
@HelgeStenstrom HelgeStenstrom deleted the extractClassFromStreamPlayer branch September 12, 2019 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants