Add support for video calls when requesting the password for a share #1123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tested with Frank: works great 👍 |
lib/Room.php
Outdated
| @@ -550,7 +572,9 @@ public function removeParticipantBySession(Participant $participant) { | |||
| * @throws InvalidPasswordException | |||
| */ | |||
| public function joinRoom($userId, $password, $passedPasswordProtection = false) { | |||
| $this->dispatcher->dispatch(self::class . '::preJoinRoom', new GenericEvent($this)); | |||
| $this->dispatcher->dispatch(self::class . '::preJoinRoom', new GenericEvent($this, [ | |||
| 'userId' => $userId, | |||
There was a problem hiding this comment.
for consistancy add the password and the flag too and also add both parameters at the postJoinRoom event below
This reverts commit 112c73b.
Every time that a conversation to request a password was successfully created a new handler for the signaling event was registered. Thus, if a password was requested, the conversation ended, and the password was requested again from the same page then each event had two handlers. Until now, due to the behaviour of the handlers, even if incorrect it was not a problem. However, in preparation for changes to the "joinRoom" handler now a single handler is registered for each signaling event, no matter how many passwords were requested. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The implementation of "connection.joinRoom" calls "app.syncAndSetActiveRoom", which performs several actions needed when the full Talk UI is used, but unneeded when the embedded Talk sidebar is used. Moreover, as it is not possible to know when the rooms were synced, joining the call after joining the room could lead to UI issues due to the order in which elements were setup (for example, the empty content message "waiting for permissions" could be shown instead of "waiting for XXX to join the call"), so this is also a needed change to be able to add support for video calls. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This is needed to show notifications using the standard "OC.Notification.show" function, which is used by some Talk functions. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When a password is requested now the guest automatically joins the call, and once the sharer joins the call too a video call view appears in the Talk sidebar. Although it is not currently shown, the empty content message for guest users was set, as it is expected to be set by some event handlers. In a similar way the "#screens" element was also added, but there is no support yet for screensharing and thus the element is kept always hidden. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When a screen is shared it takes the upper area of the call container, and the videos are shown below it in a 200px high row, just like in the normal Talk UI. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The local video (or avatar) and the media controls to mute own audio, disable own video or share own screen are now included in the main view of the sidebar, just like in the normal Talk UI. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The empty content message informs the user when the app is waiting for the media permissions and then when it is waiting for the sharer to join the call. Thus it is now shown above the chat view, in the place that will be occupied by the call container once the call starts. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
In the public page for a conversation the token is always defined, so the signaling can be initialized as soon as the app starts. However, in the public share auth page the token is only available once the password is requested and the room created, so trying to initialize the signaling when the app starts ends in failed requests to the room endpoint. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
nickvergessen
force-pushed
the
add-support-for-video-calls-when-requesting-the-password-for-a-share
branch
from
adb16d5 to
4666973
Compare
|
Rebased and added the revert of the "Open the call in a new tab for now" |
Signed-off-by: Joas Schilling <coding@schilljs.com>
nickvergessen
approved these changes
Aug 8, 2018
nickvergessen
deleted the
add-support-for-video-calls-when-requesting-the-password-for-a-share
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up for #1049 (without its last commit).
This pull request starts a call as soon as the password is requested; the video and the shared screen are shown in the sidebar, above the chat view.
The video could be shown in a larger area between the password form and the chat view; however it was added above the chat view to keep the concept of a sidebar, and because in most cases the person that needs to clearly see the other participant is the sharer, and not the requester.
In any case there would be no problem in changing the layout to the one mentioned above, or adding a fullscreen button to the sidebar to show the video as large as possible in case the requester needs it.
Unfortunately there is an issue with the notifications; starting a call removes all the previous notifications for a room, so now the {email} requested the password to access a share notification is never shown to the sharer; instead, she receives A group call has started in {email} (as the name of the room is the e-mail of the sharee).
For
share:passwordrooms maybe it would make sense to not send the call notification and keep the password request notification instead?Another minor issue at the moment is that the local video is not shown until the sharer has joined the call, so the requester does not have time to check the video before the sharer joins. However, it would not be just a matter of showing the video while waiting for the other participant, as in that case the other participant could join all of a sudden when the requester was doing the checks; it would require a deeper UX change (also for the normal UI, it is a general issue, not exclusive of the sidebar).