-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
fix(prosody-auth): Don't loose initial tracks. #14358
base: master
Are you sure you want to change the base?
Conversation
07e6513
to
27ab4e5
Compare
f9a9ee6
to
ec46340
Compare
|
||
return APP.store.dispatch(initPrejoin(localTracks, errors)); | ||
// Note: Not sure if initPrejoin needs to be async. But let's wait for it just to be sure the |
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.
It doesn't need to be. It doesn't even need access to the store so it doesn't even need to be async.
export function initPrejoin(tracks: Object[], errors: Object) {
return async function(dispatch: IStore['dispatch']) {
dispatch(setPrejoinDeviceErrors(errors));
dispatch(prejoinInitialized());
tracks.forEach(track => dispatch(trackAdded(track)));
};
}
You could rewrite this to take the store as a parameter and just call it, since it's not really an action. Then in there you can batch-dispatch the actual actions.
// dispatch(connect(jid, password)); | ||
// FIXME: Workaround for the web version. To be removed once we get rid of conference.js | ||
dispatch(joinConference(undefined, false, jid, password)); | ||
dispatch(connect(jid, password)); |
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.
Has this been tested with XMPP auth? I recall we've broken it a number of times in the past.
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.
Yes.
@@ -153,6 +158,11 @@ MiddlewareRegistry.register(store => next => action => { | |||
error.recoverable = true; | |||
|
|||
_handleLogin(store); | |||
} else { | |||
batch(() => { |
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.
This feeels like the wrong place to be handling this. Authentication should not be handling track stuff.
You can handle this in the media middleware, since the recoverable flag will already be set once you calll next
when handling this action.
@@ -264,6 +274,11 @@ function _handleLogin({ dispatch, getState }: IStore) { | |||
const videoMuted = isLocalTrackMuted(state['features/base/tracks'], MEDIA_TYPE.VIDEO); | |||
|
|||
if (!room) { | |||
batch(() => { |
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.
Ditto.
APP.conference.startConference(jitsiTracks).catch(logger.error); | ||
} | ||
}); | ||
.then(() => dispatch(disconnect())); |
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.
When will the connection begin again?
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 see some of the logic got moved to base/conference, but only to the web middleware. When does mobile start connecting again?
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.
we decided with @damencho to remove the reconnect in order not to flood the infrastructure with reconnects. IIRC the UX now will be that the user will see an error and can reload or press the Join button again.
* @param {string} action.type - Type of action. | ||
* @returns {ICommonState} | ||
*/ | ||
function _common(state: ICommonState = _COMMON_INITIAL_STATE, action: AnyAction) { |
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.
Generally speaking, base/media has always represented user intent, that is, a muted a flag in here does not necesssarily mean there is a track.
The initial gUM promise is concerned with actual tracks, which fits better in base/tracks.
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'd like to see some more details on the commit description too, since I suspect the problem of remaining stuck in a broken state wasn't reproducing all the time, right?
66ae26f
to
7f5da25
Compare
During the prosody login cycle the initial GUM tracks were lost causing the user to start the call without local media and audio/video mute buttons staying forever in pending state.
7f5da25
to
727c442
Compare
yes. it happens when the prejoin is not enabled. |
727c442
to
a13c332
Compare
During the prosody login cycle the initial GUM tracks were lost causing the user to start the call without local media and audio/video mute buttons staying forever in pending state.