votemanager: added additional resource settings for ban votes #192
Conversation
…e is provided
Saving nil in a table seems to have odd side effects when further adding key values to it.
local tab = {true,false,false,nil,5} tab.cheese = "Yes" local a,b,c,d,e = unpack (tab) outputChatBox(tostring(e))
Should output 5 but instead outputs nil. If I remove the tab.cheese = "Yes"
local tab = {true,false,false,nil,5} local a,b,c,d,e = unpack (tab) outputChatBox(tostring(e))
It will correctly output 5 again. However I don't see any easy and clean way to get around it, as votemanager depends on some further modification of the tables and unpacking them later to work properly. Turning the vote reason into an empty string "" sounds like a rather small sacrifice to ensure proper functionality.
banPlayer provides a lot of banning options, none of which votemanager currently supports changing. This commit adds settings to the meta.xml for the following settings: voteban_banip true/false (default: true); voteban_banusername true/false (default: false); voteban_banserial true/false (default: false); voteban_duration number in seconds (default: 3600)
addendum to previous commit banPlayer provides a lot of banning options, none of which votemanager currently supports changing. This commit adds settings to the meta.xml for the following settings: voteban_banip true/false (default: true); voteban_banusername true/false (default: false); voteban_banserial true/false (default: false); voteban_duration number in seconds (default: 3600)
…e is provided addendum Removed unnecessary reason == "" from kill votes as it is never used after the initial use.
[managers]/votemanager/meta.xml
Outdated
| <setting name="*voteban_banip" value="[true]"/> | ||
| <setting name="*voteban_banusername" value="[false]"/> | ||
| <setting name="*voteban_banserial" value="[false]"/> | ||
| <setting name="*voteban_duration" value="[3600]"/> |
There was a problem hiding this comment.
adding the 4 settings, the chosen setting names should be understandable intuitively
| @@ -58,6 +58,12 @@ addEventHandler("onResourceStart", thisResourceRoot, | |||
| info.percentage = get(settingsGroup.."_percentage") or nil | |||
| info.timeout = get(settingsGroup.."_timeout") or nil | |||
| info.allowchange = get(settingsGroup.."_allowchange") | |||
| if name == "ban" then | |||
There was a problem hiding this comment.
not entirely sure whether this or if settingsGroup == "voteban" then would be better. I think either is fine.
| @@ -426,6 +432,8 @@ function voteKick(player, reason) | |||
| local title = "Kick "..getPlayerName(player).."?" | |||
| if reason then | |||
| title = title.." ("..reason..")" | |||
| else | |||
| reason = "" | |||
There was a problem hiding this comment.
making reason an empty string if none is supplied for reasons explained in the pull request
| @@ -446,14 +454,16 @@ function voteBan(player, reason) | |||
| local title = "Ban "..getPlayerName(player).."?" | |||
| if reason then | |||
| title = title.." ("..reason..")" | |||
| else | |||
| reason = "" | |||
There was a problem hiding this comment.
making reason an empty string if none is supplied for reasons explained in the pull request
| end | ||
| return startPoll{ | ||
| title=title, | ||
| percentage = vote.ban.percentage, | ||
| visibleTo = rootElement, | ||
| timeout = vote.ban.timeout, | ||
| allowchange = vote.ban.allowchange; | ||
| [1]={"Yes",banPlayer,player,serverConsole,reason}, | ||
| [1]={"Yes",banPlayer,player,vote.ban.banip,vote.ban.banusername,vote.ban.banserial,serverConsole,reason,vote.ban.duration}, |
There was a problem hiding this comment.
if reason was nil here it would result in the ban duration being ignored, because after unpacking the table in
it would get lost
|
This PR appears to work as intended, but I think it would be better for players to be banned by serial by default. Thoughts? |
|
and I think IP ban should be disabled by default? I know some people play with each other and have the same IP |
|
I chose the default to be consistent with the default of banPlayer as well as how voteban previously banned players, but if everyone agrees that serial is the way to go I see no problem with changing it to reflect that. |
banPlayer provides a lot of banning options, none of which votemanager currently supports changing for its ban votes. This commit adds the following settings to the resource:
Default values are the same as the default within banPlayer, except for duration, which is 1 hour instead of infinite. Note that voteban_enabled default remains false just as before.
The addition of the empty reason string when no reason is supplied via command is a workaround to avoid issues with lua's unpack() when unpacking tables containing nil entries. Here's an example:
local tab = {true,false,false,nil,5} tab.cheese = "Yes" local a,b,c,d,e = unpack (tab) outputChatBox(tostring(e))should output 5 but instead outputs nil. Without the modificationtab.cheese = "Yes"it will correctly output 5. Meanwhile directly referencing it without unpack always seems to be working as intendedlocal tab = {true,false,false,nil,5} tab.cheese = "Yes" local a,b,c,d,e = unpack (tab) outputChatBox(tostring(tab[5]))outputting 5 correctly. Unless banPlayer internally handles an empty string very differently to nil I don't see any major drawbacks with this. I can't think of any better workaround, as votemanager relies on further modifying the tables and unpack() to read from them. But maybe someone else has a good idea.Lastly note that votekicks weren't functional before and will not be before multitheftauto/mtasa-blue#1301 gets resolved.