disable join if spec disabled #2605
Conversation
| @@ -23,6 +23,7 @@ private slots: | |||
| void actCreate(); | |||
| void actJoin(); | |||
| void checkResponse(const Response &response); | |||
| void updateButtonChoices(const QModelIndex &); | |||
There was a problem hiding this comment.
Why aren't you following the actXxxx naming conventions?
There was a problem hiding this comment.
Didn't realize there was a convention
There was a problem hiding this comment.
If you wanna make a Pr to fix all the style changes you outlined here, feel free to and I'll approve it
There was a problem hiding this comment.
I feel this was merged too soon. One of the reviewers @Daenyth hasnt replied yet.
There was a problem hiding this comment.
@ctrlaltca There is nothing in the contributing.md about peer review processes.
There was a problem hiding this comment.
Sorry, i don't get what you mean. I just said that right now there is no code convention defined about naming actions actXXX, but that as you suggested we should try to start using it and formalize it in the CONTRIBUTING.md file, that contains code best practices.
There was a problem hiding this comment.
Is there any documentation on peer review processes?
There was a problem hiding this comment.
Nope; afaik github actually has no way to force a review by external peers.
It typically boils down to the peer leaving a 👍 on the PR or commenting it.
| return; | ||
|
|
||
| const ServerInfo_Game &game = gameListModel->getGame(index.data(Qt::UserRole).toInt()); | ||
| bool overrideRestrictions = !tabSupervisor->getAdminLocked(); |
There was a problem hiding this comment.
Admins/Mods have full authority on a server
There was a problem hiding this comment.
If you want to check for permissions, isnt it better to do it through the ServerInfo_User: ServerInfo_User *getUserInfo() const { return userInfo; } from TabSupervisor, rather than checking if a tab is locked?
There was a problem hiding this comment.
The "supervisor" tab has a button that locks/unlocks admin superpowers.
The line is making the admin able to override the no-spectators-allowed constraint only when the superpowers are unlocked.
There was a problem hiding this comment.
Ok, so you need to click a button to enable a superuser permission.
This line also assigns an inverse result. It returns the getAdminLocked() and then ! it. This doesnt feel right. The ! should be placed in the check.
There was a problem hiding this comment.
Imho having a variable named overrideRestrictions helps understanding what is actually checked in the if(), while just putting !tabSupervisor->getAdminLocked() would make it more obscure, as you originally noticed by not getting why the check was performed.
There was a problem hiding this comment.
bool adminLocked = tabSupervisor->getAdminLocked()
Return if the admin is locked.
if (!adminLocked)
If admin is not locked, then proceed.
You dont need a custom overrideRestrictions name when the original variable says all it needs to.
There was a problem hiding this comment.
It was just imho, but if you know to have the truth, probably you are right.
| const ServerInfo_Game &game = gameListModel->getGame(index.data(Qt::UserRole).toInt()); | ||
| bool overrideRestrictions = !tabSupervisor->getAdminLocked(); | ||
|
|
||
| if (game.spectators_allowed() || overrideRestrictions) |
There was a problem hiding this comment.
Simplify to spectateButton->setEnabled(game.spectators_allowed() || overrideRestrictions)?
There was a problem hiding this comment.
True, might change it to this at some point in the future
|
Another thing... why don't we do the same with |
|
@tooomm Not a bad idea. can you open a ticket and i'll look into that later? |
|
@ZeldaZach are you able to make a new PR addressing the feedback in this PR? |
|
I can, yes, but it's not too high on my priority list right now as I have other tickets I'd rather address. If you'd like to make a PR I'd be more then glad to review it and merge it in |
ghost
mentioned this pull request
|
@ZeldaZach will do later, yes. Leaving home now. |
Related Ticket(s)
Short roundup of the initial problem
Join button should be disabled if the game doesn't allow spectators. This does so.
Screenshots