[RN][iOS] Default to speaker while in a conference #1254
Conversation
| @property (nonatomic, readonly) AVAudioSession *session; | ||
| @property (nonatomic, readonly) NSString *category; | ||
| @property (nonatomic, readonly) NSString *mode; | ||
| @property (nonatomic, readonly) BOOL initialized; |
There was a problem hiding this comment.
These properties are not really supposed to be public, right? As far as I understand their use in the respective .m file, they are private.
Further thinking about the .h file brought me to the realization that for the react-native modules that we have in ios/app - AudioMode and POSIX - are not really something that we plan to publicly expose through Objective C. They do have public APIs but these are in JavaScript. So the AudioMode.h and POSIX.h stopped making sense to me.
| if (!success || error) { | ||
| RCTLogInfo(@"Error overriding the desired session mode"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm concerned about routeChange: and 'setMode:resolve:reject:' having very similar source code that's core to this feature: setCategory:error: and setMode:error:. I think a common method invoked by the two makes me feel more comfortable.
| }; | ||
| } | ||
|
|
||
| export default AudioMode; |
There was a problem hiding this comment.
AudioMode is an abstraction (to me) that I don't see being used outside its feature. Unlike POSIX which started with the idea to be a collection of POSIX functions and, thus, likely belongs to features/base.
| return Promise.resolve(null); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
-
I think we can do away with their
Platform.OScheck here. EitherNativeModules.AudioModeis defined or not. Practically, it's not going to be defined on Android at this time. So with thePlatform.OScheck we're sure to have future work: once we implement it on Android, we'll have to modify this file again. -
Why put a stub
AudioMode? I don't think it's better to tell consumers that it's supported given that it's really not.
| .catch(err => { | ||
| console.warn(`Error setting audio mode: ${err}`); | ||
| }); | ||
| break; |
There was a problem hiding this comment.
-
The branch CONFERENCE_FAILED/LEFT is the same as APP_WILL_MOUNT. For simplicity's sake, I prefer them expressed as a single branch. Otherwise, I see they are different branches and I start desperately looking for a difference. At the end the difference is just the
{}around the ALL_WILL_MOUNT branch. -
All branches are basically, choose an AudioMode constant, invoke AudioMode.setMode with the chosen constant, catch an error, print an error. I prefer that logic to be expresses with as less branching as possible and as much common logic implemented in a common place as possible. So the branching is really just in "choose an AudioMode constant" and the remaining logic is the same.
| * lib-jitsi-meet and not the application. | ||
| * | ||
| * @param {string} room - The JitsiConference instance which will | ||
| * be left by the local participant. |
There was a problem hiding this comment.
The above is a copy of the documentation on conferenceWillLeave.
lyubomir
force-pushed
the
audio-mode
branch
2 times, most recently
from
f48a610 to
ec35e64
Compare
It's fired before creating the conference with lib-jitsi-meet.
The implementation is slightly more complex than it should because libwebrtc modifies the AVAudiosession category and mode internally, so we setup a route change handler and when it does we change it back to what we need.