Skip to content
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

added setMixerName #65

Merged
merged 4 commits into from
Jul 9, 2020
Merged

added setMixerName #65

merged 4 commits into from
Jul 9, 2020

Conversation

JellevanAbbema
Copy link
Contributor

@JellevanAbbema JellevanAbbema commented Jul 7, 2020

Description

Alright this is my first pull request so I hope I'm doing everything right. (Any feedback is welcome!)
I came across issue #64 which is something I fixed a while back for a program I'm writing (note: I'm not a software engineer and just doing this as a hobby) at that time I didn't know GitHub that well and had no idea how to share that code the right way, but thought it would be useful to share now for @RinAndShizuka

The only changes I made was add a function to set the mixerName and remove 1 line of code that overwrites the SourceDataLine (which I believe was useless anyway). Overall quite simple since almost all the necessary code is already there.
The setMixerName function should be called BEFORE creating a line and thus before opening a file.

Fixes #64

What kind of change does this PR introduce? (check at least one)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code style update
  • Refactor
  • Build-related changes
  • This change requires a documentation update
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Has This Been Tested?

  • Yes
  • No

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Contributor

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great first PR :) 🎉🎉

I suggest also adding a test for this new feature. This will ensure the feature works correctly & won’t break with future contributions. If you need help with this just let me know :)

@@ -529,7 +529,8 @@ private void createLine() throws LineUnavailableException, StreamPlayerException
outlet.setSourceDataLine((SourceDataLine) mixer.getLine(lineInfo));
}

outlet.setSourceDataLine((SourceDataLine) AudioSystem.getLine(lineInfo));
//Had to comment this because it overwrites the data line we just set, thus making code above useless (this line is already run when there is no mixer name set)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove the line instead of deleting it. Maybe mention this in the commit message so that this note is tracked (instead of having the message in the code)

@@ -529,7 +529,8 @@ private void createLine() throws LineUnavailableException, StreamPlayerException
outlet.setSourceDataLine((SourceDataLine) mixer.getLine(lineInfo));
}

outlet.setSourceDataLine((SourceDataLine) AudioSystem.getLine(lineInfo));
//Had to comment this because it overwrites the data line we just set, thus making code above useless (this line is already run when there is no mixer name set)
//outlet.setSourceDataLine((SourceDataLine) AudioSystem.getLine(lineInfo));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d Remove this line

@@ -257,6 +257,13 @@
*/
void setLineBufferSize(int size);

/**
* Set the name of the mixer. This should be called before opening a Line.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add this note to the java doc for the implementation function in Stream Player, as most clients calling the function will be calling it from StreamPlayer

@goxr3plus
Copy link
Owner

Great job guys @AObuchow when you feel this will be ready you can merge it :). I am thinking of remaking the repository code more simple as it was before... There are too many classes now and no documentation.

removed line 532 because it overwrites the SourceDataLine
@JellevanAbbema
Copy link
Contributor Author

Alright so I changed the things that @AObuchow mentioned, I'm trying to write a test (I just recently started to use tests myself so just still figuring it out). Right now (I think) there is no possibility to check the mixer that is currently being used, so also no way to check if it correspond to the set mixer.
Should I add a 'getCurrentMixer' function? (or something like that?)

@goxr3plus
Copy link
Owner

@JellevanAbbema Yes add that function it will be useful.

@AObuchow
Copy link
Contributor

AObuchow commented Jul 8, 2020

This looks good to me but @HelgeStenstrom might want to have a look.

One thing I noticed is there's no actual dedicated test for get/set mixer? It seems the implementation of the StreamPlayerTest was just modified. It might be nicer to setup a dedicated JUnit test for get/set mixer, so that if it fails we know what "feature" is broken.

@goxr3plus
Copy link
Owner

Looks good to me also. @JellevanAbbema have you tested that it works correctly, you runned some files and all good? If yes I merge.

@JellevanAbbema
Copy link
Contributor Author

Yes I did do a couple of tests, played it on the speakers of my second monitor (which is not the default audio device) without a problem.

@goxr3plus
Copy link
Owner

You the best :)

@goxr3plus
Copy link
Owner

I think many improvements can be done on this library.

@goxr3plus goxr3plus merged commit ecdabb4 into goxr3plus:master Jul 9, 2020
@goxr3plus
Copy link
Owner

I just made a new release feel free to check and use :)

https://github.com/goxr3plus/java-stream-player/releases

@goxr3plus
Copy link
Owner

10.0.2

@AObuchow
Copy link
Contributor

Thanks @goxr3plus ! :)

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.

Ability to change which mixer it plays to?
3 participants