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

API Suggestion: Add TownPreDeleteCmdEvent and NationPreDeleteCmdEvent #7410

Closed
galacticwarrior9 opened this issue May 7, 2024 · 5 comments
Closed
Assignees
Milestone

Comments

@galacticwarrior9
Copy link
Contributor

Please explain your feature request to the best of your abilities:

Follows the same principle as the existing TownPreUnclaimEvent. The new events would be called in /t delete and /n delete respectively.

PreDeleteTownEvent and PreNationDeleteEvent are called in the database handler, which means players are not sent the cancellation message. This can leave players confused (see #6178 for a similar problem). Some may also only want to prevent players from deleting via command, without interfering with deletions triggered elsewhere in the plugin.

@Warriorrrr
Copy link
Member

Instead of a new event, why not just add the deleter to it so that the message can be sent?

@galacticwarrior9
Copy link
Contributor Author

Sure, that would help. However, there is still the problem of the existing events being called by Towny when it deletes a town/nation internally. I would prefer not to interfere with these deletions as it isn't necessary for my use-case and can lead to odd issues.

@Warriorrrr
Copy link
Member

Having a deletion cause exposed would help with that

@galacticwarrior9
Copy link
Contributor Author

I agree, that would be great

@LlmDl
Copy link
Member

LlmDl commented May 8, 2024

I'm kinda thinking this one is going to have a fairly big refactor behind it. The issue I ran into before was that the removeTown and removeNation methods don't return anything for the other methods to work off of.

We could change those (and other things) over to booleans so some of the methods could deal with failures.

The other thing that's a bit of an issue is that towny can send a town/nation to be deleted for very good reasons (like having no residents or no towns,) and another plugin can currently stop that object being deleted.

We could also create removeTown(Town, new DeleteCause(Player, DeleteReason.TOWN_DELETE_COMMAND) and simply pass in a new DeleteCause object. DeleteCause would need to support a Player being nullable, and probably have a enum of actual reasons backing it. Then removeTown(Town, Boolean, DeleteCause) could send back an error message when the event is cancelled when DeleteCause.getPlayer() isn't null. It could also skip throwing the event entirely when the DeleteCause.getDeleteReason() is something critical where the TownyObject really shouldn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants