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

Discussion: registration of events listeners and the plugin instance required for that #45

Closed
Trigary opened this issue Feb 22, 2020 · 2 comments
Labels
discussion Indicates that the issue is for discussing the described change

Comments

@Trigary
Copy link
Contributor

Trigary commented Feb 22, 2020

An instance of Plugin is required to register an event listener. At the moment only one listener exists in IF: GuiListener, so I will exclusively use that in my examples. The Gui class has a static hasRegisteredListeners boolean variable that is used to register an event listener if one hasn't been registered yet. The plugin instance is the one passed into the Gui constructor. What happens if that plugin gets disabled? Even if we ignore the broken reload command and the even better PlugMan plugin, plugins are still disabled if they throw an exception in their onEnable(). If the plugin associated with IF's event listener is disabled, IF's event listener gets unregistered without IF knowing about it. I see three options:

  • don't rely on a real plugin, try to "mock" one for Bukkit (it's a hackish solution, but luckily only the server itself can have access to this "virtual" plugin instance)
  • listen to PluginDisableEvent, migrate to another plugin and register a new listener when it's called (this solution seems a bit complicated, could go wrong a thousand ways)
  • don't worry about this, since if a plugin gets disabled, something's wrong and IF can't make itself that much fool- and bugproof

I would be more than happy to implement and PR whatever we decide on (if it's not the "don't worry about this" option, of course). This is a place for arguments and counter-arguments.

@stefvanschie stefvanschie added the discussion Indicates that the issue is for discussing the described change label Feb 23, 2020
@stefvanschie
Copy link
Owner

I'm not certain if this is really a problem. IF is meant to be shaded in the plugin and relocated. If multiple plugins use IF that exist on the same server, each of those would then therefore have their own instance of IF; as far as I'm aware, the data between these different shaded IFs is not shared. Each plugin that makes use of IF should therefore get their own listener, so the plugin that uses IF registers the event listener exclusively for their own guis. Of course this plugin could get disabled, but then only that plugin's even listener would get unregistered; any other plugins on the same server that make use of IF should still be fine, since those should still have their own registered listener for their guis. And if the plugin gets disabled, the plugin won't be running anymore and therefore shouldn't be able to make use of any guis.

Of course it is technically possible to register the first gui via a different plugin than your own by grabbing a different plugin instance (which is a very questionable choice on the developer's part in my opinion), so that may be something we could prevent from happening (although in my opinion not really needed).

@Trigary
Copy link
Contributor Author

Trigary commented Mar 13, 2020

I totally forgot about the shading, relocation when writing this. Makes no sense now. Closing this issue.

@Trigary Trigary closed this as completed Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Indicates that the issue is for discussing the described change
Projects
None yet
Development

No branches or pull requests

2 participants