Skip to content
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

Add option to hide the room list for all users. #1052

Closed
wants to merge 2 commits into from

Conversation

fancycode
Copy link
Member

This makes sense for cases where the rooms are managed by a different app and the users should not see all available rooms.

lib/Manager.php Outdated Show resolved Hide resolved
@fancycode
Copy link
Member Author

Btw, I have similar changes upcoming to remove adding participants to a room from the dropdown above the participant list and to remove the share room links. Do you want to have these in a separate PR or included in this one?

@nickvergessen
Copy link
Member

Shouldn't it be enough for those if the user is not a moderator/owner of the all?
That should already hide the possibility to access both.

@fancycode
Copy link
Member Author

For regular participants that is enough, but the registered users are owners of their events/rooms and can invite additional moderator users.

This makes sense for cases where the rooms are managed by a different
app and the users should not see all available rooms.

Signed-off-by: Joachim Bauch <[email protected]>
@fancycode
Copy link
Member Author

Changed to get setting from config.php and also pushed the changed UI code I missed in my first push (sorry for that).

* @return bool
*/
public function hideRoomList() {
return $this->config->getSystemValue('spreed.hide_roomlist', false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about naming this talk-webinary-only-mode and doing all with one config?
cc @Ivansss

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would rather keep it separate with different options for room list, participant list, etc.

We could however support an additional property that disables multiple things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, it makes no sense for normal Talk installations.
And I dont want us to have to test 4 different config combinations on each release, that's why I would prefer a "full" or "restricted" thing, not multiple different levels.


this._sidebarView.addTab('participants', { label: t('spreed', 'Participants'), icon: 'icon-contacts-dark' }, this._participantsView);
},
/**
* @param {string} token
*/
_setRoomActive: function(token) {
if (OC.getCurrentUser().uid) {
if (this._rooms) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm this makes the code execute for guests?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of: _rooms is only created for logged in users at

this._rooms = new OCA.SpreedMe.Models.RoomCollection();
so the behaviour is the same as before:

@nickvergessen
Copy link
Member

For now this is not the way we want to go with the UI, so I will close this and it can be looked into again after the Vue rewrite #1347

@nickvergessen nickvergessen deleted the hide-roomlist branch July 12, 2019 09:11
@Antreesy Antreesy removed this from the 💔 Backlog milestone Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants