-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat: new type proposition #495
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
base: feat/android-audio-focus-options
Are you sure you want to change the base?
feat: new type proposition #495
Conversation
export interface AudioOptions extends BaseAudioOptions { | ||
androidAcceptsDelayedFocusGain?: boolean; | ||
androidAudioAttributes?: AudioAttributeType; | ||
androidFocusGain?: focusGainType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't comment on the original type: we should have only one case-type when it comes to setting options, currently:
- ios settings are camel case
- android settings are snake case
- permission status (which is kind of related) is upper-case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also i'm wondering if we should include spatialization behaviour currently, it seems it is either auto
or off
. I think we could stick with whatever is default for now.
Especially some-day we will have our on spatialization implementation thus we would like to have this off by defULT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we talked a bit today about this: could you add short JSDoc-like descriptions to the values (aka what is their meaning). Could be a great start for documenting the api on code-level :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with case-type I wanted to stick with original system values, therefore it is different for ios and android, but it can be narrowed to one easily
* interruptions and emit events accordingly (true by default). | ||
* @param options - Additional audio options to configure the audio session. | ||
*/ | ||
setAudioOptions(observeAudioInterruption = true, options?: AudioOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to call observe interruption every play invocation on Android so merging AudioSession settings into one method its not the best option in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so am I correct, if the audio interruption event will be fired, it won't be invoked 2nd time and we have to re-observe it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
Closes #
Introduced changes
Checklist