VideoSIPGW updates #2201
VideoSIPGW updates #2201
Conversation
damencho
force-pushed
the
sipgw-updates
branch
3 times, most recently
from
79d2ed1
to
15a534e
Compare
Also explains enabling the people search service and the request/response that are made around sipgw jibri service.
No invitation is sent when there is nobody to invite.
damencho
force-pushed
the
sipgw-updates
branch
from
15a534e
to
ec1d564
Compare
| this.props._conference, | ||
| this.state.inviteItems.filter( | ||
| i => i.type === 'videosipgw')); | ||
| && this.props.inviteRooms( |
There was a problem hiding this comment.
There is a type "room" and the function is called inviteRooms, but you filter for videosipgw. What is the difference between room and videosipgw?
There was a problem hiding this comment.
The invitation works with 3 types returned - user, room and videosipgw. User is a regular participant, room is like a chat room with multiple participants like in Stride. And also we videosipgw. So let's say we have chat room (type===room) and conference room (type=== videosipgw).
I agree its a little confusing and I also was wondering about that. If you have an idea how to improve it?
There was a problem hiding this comment.
Maybe we can imagine what it would be like with different names, like specifically calling them chatRoom and conferenceRoom/videoRoom/sipGW instead of using the general term room.
There was a problem hiding this comment.
Ok, I will address that.
| * the global sip gw availability in redux or show appropriate notification | ||
| * for sip gw sessions. | ||
| * Captures invitation actions that create sip gw sessions or display | ||
| * appropriate error/warning notifications. |
There was a problem hiding this comment.
Maybe the second paragraph here isn't needed.
There was a problem hiding this comment.
This describes the SIP_GW_INVITE_ROOMS case.
| if (toDispatch) { | ||
| dispatch(toDispatch); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Could you tell me how the timing of the join flow works? connection.initJitsiConference is called, which the browser takes over (async), but in the same stack as the call to connection.initJitsiConference the action CONFERENCE_WILL_JOIN is dispatched? I want to make sure it is guaranteed these conference listeners are set before the video sip gw events can be fired.
There was a problem hiding this comment.
So after we call initJitsiConference we fire the CONFERENCE_WILL_JOIN action, and after the call for initJitsiConference returns the conference object we call on it join.
Basically I was added the same that mobile is currently doing:
| const newSession = action.conference | ||
| .createVideoSIPGWSession(sipAddress, displayName); | ||
|
|
||
| if (newSession instanceof Error) { |
There was a problem hiding this comment.
Why did you choose returning an error over other possible implementations, such as returning a failed promise?
There was a problem hiding this comment.
Cause we are creating a session and returning it, there is no network operation involved and no need for any promise. This was the initial implementation.
But we can change the API of lib-jitsi-meet to be promised base, do you think its better that way?
There was a problem hiding this comment.
If it was the implementation that was already there then no I don't think you should change it right now so that I don't keep asking you to make additional changes. However, I do think it's better to be consistent with what returns because if a promise is always returned then errors can be handled in a catch.
| type: SIP_GW_AVAILABILITY_CHANGED, | ||
| status | ||
| }; | ||
| } |
There was a problem hiding this comment.
It's okay style-wise to have actions inside the middleware if they're not intended to be used publicly?
There was a problem hiding this comment.
The _availabilityChanged is private. And the intention is just to notify the reducer to store it in the state for future reference (showing notifications, handled in SIP_GW_INVITE_ROOMS case up in the code).
There was a problem hiding this comment.
Right, what I'm asking is in terms of style/organization, normally all actions go into an actions.js file but this is a special case because the action is meant to be private. So I want to ask specifically if you've given thought about if it's okay or not to have an action in a middleware file. If you've given it thought and it's fine then I'm okay with it (and someone else can just change it when they want).
|
How does it look now, any better? |
| this.props._conference, | ||
| this.state.inviteItems.filter( | ||
| i => i.type === 'videosipgw')); | ||
|
|
||
| invitePeople( | ||
| invitePeopleAndChatRooms( |
There was a problem hiding this comment.
I just noticed, the other actions are put onto this.props but invitePeopleAndChatRooms isn't. Do you care?
There was a problem hiding this comment.
My mistake here. Damyan showed this is a function and shouldn't be mapped to props.
* Adds initial documentation for sipgw jibri. Also explains enabling the people search service and the request/response that are made around sipgw jibri service. * Fixes add people dialog to invite users and rooms. No invitation is sent when there is nobody to invite. * Reuse some recording strings, by using arguments. * Make sure web also dispatches CONFERENCE_WILL_JOIN. * Introduces new feature videosipgw. * Fixes lint errors. * Renames methods to use people, chatRooms and videoRooms. * Updates to latest lib-jitsi-meet (dc3397b18b).
No description provided.