Carpet

Carpet

2M Downloads

[Internal] DetachServer isn't called for extensions. Is it really needed?

altrisi opened this issue ยท 4 comments

commented

SettingsManagers have a detachServer method that resets their rules to default (with a null source) and sets their server to null. That method is only called for Carpet's SettingsManager in CarpetServer (at onServerClosed). That seems that it may be bad, because that may mean that rules won't be reset when changing a world.

However, the first thing (after setting the server) that the manager does when attachServer is called (this one is correctly called for extensions ) is to reset the rules to default with an actual ServerCommandSource. That means that, no matter if server is detached or not, rules will be reset the same way, and probably the most effective method is the one in attachServer, since rules that may require a source to change (e.g. one of skyblock extension's rules) will be called correctly.

Therefore: Is detachServer really needed? If so, should it be called for extensions too? Should it keep the reset functionality?

The only check to see if the server is null (condition that only happens when server is detached or before attaching) is in notifyPlayersCommandsChanged.

May it be to clear pointers to the MinecraftServer? (not sure if that would be useful for anything or how that works, just thought about gc)

commented

loosing ties with the server is most appropriate way to do, other than just veering off static classes and replace them with instances - might be less convenient to use though. I would honestly call that on the extensions in case they have things to clean as well. Find it most appopriate myself.

commented

I didn't fully understand that last comment (can't see relations with instances). Basically the thing is that detachServer is only called for Carpet's manager, but not for extensions. However that doesn't really affect extensions since the server will be exchanged anyway in attachServer, and rules will be reset (which happens twice for Carpet's since it resets when detaching without a source and when attaching with a source).

And the question was about whether to keep it as is, keeping it and calling it in extensions too, keep it without resetting rules on it (aka only setting the server to null in detachServer), or just don't keep it.

commented

Or did you mean making server static among all SettingsManager instances?

commented

That's also an option. I think proper detaching from a server that's shut down is the best thing to do and how its implemented for extensions - its... just an iplementation detail.