Fixes 2 command issues - Githubissues

xtomyserrax commented 4 years ago

Fixes a small issue in ally command (a return was missing)
Fixes an issue with the ban command when you tried to ban a player that couldnt be found (This happens with non existing players: https://pastebin.com/41jPD9Z6 ; and this happens when you try to ban a player you have recently unbanned, doesnt really matter if its online or offline: https://pastebin.com/ePYFeqwP).

And this is a question: could we check in AsyncPlayerChatEvent for clan chat if the event is cancelled? For example it happens in my server that if Im in staffchat and simple clans chat at the same time then I send messages to both chats. Of course Im dumb for not remembering that Im on both of them, but there are some plugins which after the command requires some chat messages and they would also be sent to the clan chat. Dont know what you think about it. (Path: src/main/java/net/sacredlabyrinth/phaed/simpleclans/listeners/SCPlayerListener.java)

I finally learnt how to change two files and make one single pull request haha. Im really noob with github world.

Thanks!

xtomyserrax commented 4 years ago

Well I have changed what I did before. Now it should work fine for Online and Offline players. Also added method ban(ClanPlayer cp) and now the ban(uuid) and ban(playername) methods will call the ban(cp) one, so there is actually code on just one of them and if you ever need to fix or change something to the ban method you do it once

xtomyserrax commented 4 years ago

The cp == null message should be different like: "Player cant be banned from a clan as he doesnt belong to any" or whatever. Didnt want to touch that

RoinujNosde commented 4 years ago

And this is a question: could we check in AsyncPlayerChatEvent for clan chat if the event is cancelled? For example it happens in my server that if Im in staffchat and simple clans chat at the same time then I send messages to both chats.

Open an issue for this, please.

xtomyserrax commented 4 years ago

Maybe add a "This command is disabled" message so you get feedback of why a command you are trying to use does nothing? This can be used for example with /clan fee (check and set)

The fee command actually returns if it is disabled and doesnt send message, it would be cool to have some feedback (because if you have perm, the /clan command stills shows you in the list the fee command, it can be hidden from that list but wouldnt be the best option)

https://github.com/marcelo-mason/SimpleClans/blob/master/src/main/java/net/sacredlabyrinth/phaed/simpleclans/commands/FeeCommand.java

Thanks, let me know what you think

RoinujNosde commented 4 years ago

I think a "disabled" message is necessary. Also hiding from the list if not enabled.

xtomyserrax commented 4 years ago

Cool, if you want add the message/s and Ill help you adding them to the commands

Edit: the fee command now wont be shown in the list if not enabled

xtomyserrax commented 4 years ago

I guess I have to make a new pull request because of the new update which changed many things? Let me know and Ill do it! Ty

RoinujNosde commented 4 years ago

It's alright, I'm reviewing it right now.

marcelo-mason / SimpleClans

Fixes 2 command issues #562