-
Notifications
You must be signed in to change notification settings - Fork 448
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
⏳ Lobby #1926
⏳ Lobby #1926
Conversation
Working on mockups for both the toggle as well as the waiting screen before I leave for vacation, will post them in the next few hours. :) |
Here’s the mockups: Toggle in Talk sidebar2 versions. One just inserts more stuff in the sidebar, the other also modifies the way we show the share link. Menu
Direct toggle Lobby / waiting screen
What do you think @nickvergessen @karlitschek? cc @nextcloud/designers for review :) |
looks good. there should also be a way to set a start time when the call automatically starts |
Mutliple issues with the mock up for the lobby
But other then that, I like how it looks. |
Yes, it's not a good idea to duplicate your work :) |
@nickvergessen good points. Easy fix for all: |
044322e
to
b2b76c0
Compare
We decided to go for the dropdown, because I also implemented a datetime field as "end time" for the lobby now, after quick discussions with frank |
You mean the menu on the sidebar? |
#1926 (comment) |
👍 |
b2b76c0
to
5289f1b
Compare
I think the backend is mostly done. For the Web UI the following changes are needed: Enabling/disabling the lobby
Waiting in the lobby
|
Tried my best with the setting UI and at least there are buttons now to check the behaviour, |
36d11a7
to
ede1750
Compare
af0751f
to
f8f6dd9
Compare
83f8033
to
a22974d
Compare
I have removed all the previous UI related code (I have not added the new UI yet, as it depends on other pull request which are not merged yet), bumped the version to trigger the migration, added a notification to the external signaling server when the lobby state changes, added integration tests and cleaned a bit the commit history. @nickvergessen The old "Actual lobby page" (3f54354) became "Get password from session if not given when joining room" after the clean up (the fix for a previous commit was merged with it, the dummy pages were removed and the signaling was made available when in the lobby for the initial version). Why is that change needed? Is it needed for the lobby or is it a general fix that can be extracted to its own pull request? Or was it just needed for the dummy lobby page and no longer needed? Can you provide a test case that shows the issue that it solves? The commit itself introduced a regression when joining a password protected conversation as a logged in user (I have added an acceptance test that shows it); I fixed it for logged in users following the change you made for guests... but please check it :-) Besides that, as the signaling is now accessible while in the lobby, is pinging when getting a single room still needed? Finally, it would be nice to have some integration tests when setting the start time of the lobby... but I do not know how to simulate specific times; an option would be to test with times N seconds in the future and add a sleep in the tests... but of course I would prefer something cleaner ;-) |
It is needed because in the meantime the page reloaded the room every x seconds. But on the way it lost the password and also previously we removed the password everytime you loaded the room. Now we don't do that anymore. But also it's been more than 2 months since I wrote the code, so I don't recall all details anymore. |
Ah, now I see, thanks.
Yes, that change is not needed for the current lobby UI, so I have extracted it to #2121
This pull request will have to be rebased anyway, so I will drop the commits related to the password and the ping when doing it. No need to wait for the other one to be merged nor anything, though.
Detailed commit messages help with that ;-) |
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…r was reached Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The lobby constants were named from the point of view of the webinary (open to all participants, open to moderators only), but from the point of view of the lobby it is the opposite (no lobby, lobby for non moderators). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The lobby state can be set to no lobby or to lobby for non moderators from the room management menu; in this initial version no date can be set yet. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Unfortunately the date picker component from Nextcloud only allows picking a date, but not a time; there is a date time picker for Vue, but integrating only that right now would require much effort. Therefore, for the time being, the start time needs to be introduced manually. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
5d5d18e
to
22be1a9
Compare
Yeah, if I had time before that would have been great. Do you think that I have enjoyed overtiming for two months and also then crunching a few nights? Hint: no.
I did not want to keep rebasing several drafts until the required previous work settled. But OK, I will try to do it next time.
I do. But yes, whatever. Anyway, regarding the lobby UI the external signaling server does not send update messages to guests when a room changes. Due to this, when the lobby state is changed the UI for guests is not immediately updated, although it will be eventually updated due to the periodic forced synchronization (which was added to work around an issue in the UI of users, but ironically helps here too ;-) ). This issue is unrelated to the lobby itself, and a proper fix requires changes in both Talk and the external signaling server, so it is something for a different pull request (and the future). Besides that, the UI when joining as a guest a room with the lobby enabled is not good; the problem is that the guest first receives the room data when she has not joined the room, but that is enough to set up the UI. Then, when the actual room data is received it is found that the user is in the lobby, so the UI changes to the lobby view. It is basically the same problem as in guest mentions with the external signaling server, and actually unrelated to the lobby; it will be (eventually) fixed in a different pull request. Finally, note that if the lobby is enabled while a call is ongoing the regular users will not be expelled from the call. That is something that should be handled by the signaling more than the UI, though, so also something for later. |
Ah, so that happens because signaling continuous to work now. Yeah let's discuss a plan for that in a follow up issue |
Weird; it slipped my tests because it only happens with newer Firefox and Chromium versions if the internal signaling server is used; in older Firefox versions or when the external signaling server it does not happen :-O In any case, I do not think that this is related to the lobby, as I have seen that before in the past, but I never found a consistent way to reproduce it. Now with the lobby I have, so I will see what is going on ;-) |
It was indeed unrelated and needs to be backported, so here is the fix: #2124 (there is no need to rebase the lobby pull request once that one is merged). |
So, all checkboxes ticked, ready to go? Or still something missing before this? |
Quicker kill when turn on lobby again=> Don't retry signaling when the room was deleted or restricted #1928Fix #382