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

feat: Implement setBalance feature for android #1413

Closed

Conversation

ritheshSalyan
Copy link

Description

Added the setBalance feature for android.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
  • I have updated/added relevant examples in example.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

Copy link
Collaborator

@Gustl22 Gustl22 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
We should move the individual implementations to WrappedPlayer to save much code. No need to add an extra map method, which makes this a bit more complicated than it is :)
Also remove this line, to enable Android tests for Balance.
Also you may can change parity table for balance to support Android as well :)

response.notImplemented()
val balance = call.argument<Double>("balance") ?: error("Balance is required")
player.balance = balance.toFloat()

return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Plz remove return statement, so that the call returns 1 in case it succeeds.
Also remove unnecessary empty line :)

@@ -48,6 +48,15 @@ class WrappedPlayer internal constructor(
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

add extra empty line, to better separate setters

@@ -16,6 +16,7 @@ interface Player {
fun release()

fun setVolume(volume: Float)
fun setBalance(balance: Float)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather change the interface to:

fun setVolume(leftVolume: Float, rightVolume: Float)

and remove the setBalance method.
That way we can remove the redundant code in SoundPoolPlayer and MediaPlayerPlayer and move the calculation to WrappedPlayer. Then also there's no need to keep track of volume and balance in their implementation.

//
private var volume = 1.0f;
private var balance = 0.0f;

Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, if moved to WrappedPlayer

return (x - inMin) * (outMax - outMin) / (inMax - inMin) + outMin
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, if moved to WrappedPlayer

val rightVolume = map(balance,-1.0f,1.0f,0.0f,1.0f) * volume;

mediaPlayer.setVolume(leftVolume, rightVolume)

Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, if moved to WrappedPlayer

/* Keep track of latest volume and balance */
private var volume = 1.0f;
private var balance = 0.0f;

Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, if moved to WrappedPlayer

}

private fun map(x: Float, inMin: Float, inMax: Float, outMin: Float, outMax: Float): Float {
return (x - inMin) * (outMax - outMin) / (inMax - inMin) + outMin
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, if moved to WrappedPlayer

if (field != value) {
field = value
if (!released) {
player?.setBalance(value)
Copy link
Collaborator

@Gustl22 Gustl22 Feb 7, 2023

Choose a reason for hiding this comment

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

Here call, or at least that's my interpretation of the balance feature:

val leftVolume = min(1, 1 - value)
val rightVolume = min(1, 1 + value)
player?.setVolume(leftVolume, rightVolume)

@Gustl22
Copy link
Collaborator

Gustl22 commented Mar 3, 2023

@ritheshSalyan do you plan on continue working on this? Thanks for your support :D

@Gustl22 Gustl22 mentioned this pull request Mar 19, 2023
7 tasks
@Gustl22
Copy link
Collaborator

Gustl22 commented Mar 19, 2023

Closing in favor of #1444

@Gustl22 Gustl22 closed this Mar 19, 2023
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