EngineHub
Completely fixed trigger()
Did anyone ever use trigger?
Following PR fixes almost everything not event-specific in the whole trigger system. It allows the usage of trigger again.
Fixes done: Fixing 4 inconsistent events (one causing a NPE). Fixing EventBuilder which was also incompatible with all events except one. -> Preventing not useful spamy developer debug on proper usage. Rewriting ManualTrigger to support sending serverwide events without binding them first.
This PR has been created in collaboration with @Pieter12345 Sadly we seem to have slightly different opinions on how to solve the EventBuilder issue (and on PR's). Ofc, I prefer my solution, since it does not require checking extends but instead fixes the 3 broken events - as long as everybody continues to create events properly it will work in future.
Additional things which will break if not fixed: There are some events expecting to support trigger() but actually not implement the _instantiate method.
player_move player_teleport player_portal_travel vehicle_move
These would trigger the, in this PR prevented debug message, warning that no _instantiate method exists.
Has issue with one wrong import, issue resolved, lets ask travis for more issues.
Given that the code works, I like the following changes:
- Added some "extends MCPlayerEvent" to interfaces where applicable.
- Removed static warmup() call, so methods are only cached when they are needed.
- Changed an error message, which is good, but could be improved by adding a test which ensures that every event implementation has an _instantiate method.
- Changed ManualTrigger(), which if it works as expected should fix the having-a-bind-requirement of trigger().
- Updated trigger() documentation to match the new ManualTrigger() implementation.
The one thing I do not like is:
- Changed the _instantiate code to return a BukkitMCEvent (extends BindableEvent) instead of a Bukkit Event implementation (extends nothing from CH).
Having the _instantiate method as it is ensures that some Bukkit Event object was instantiated and passed to the corresponding BukkitMCEvent class (and therefore indirectly ensures that some event can be broadcasted server-wide, as it has to have a Bukkit event object). It also disallows some _instantiate method to instantiate events from a different type, even though that would be very poor programming from the developer that writes that (or a copy-paste error perhaps). An argument against, on the other hand, is that making the _instantiate method return a BukkitMCEvent does not require reflection to pass the Bukkit Event to the BukkitMCEvent constructor, which is more efficient.
@LadyCailin and @PseudoKnight , this _instantiate design choice is something I'd like to hear your opinions about. If we can all agree on something, @Ecconia and I can figure out how to make our PR's compatible for merging.
- I agree on adding a compile test, instead of adding a runtime warning for missing _instantiate methods.
- Else I would be fine with your choice regards the return type of _instantiate. All, except one Event using _instantiate, currently return a BukkitMC*Event. Returning a Bukkit event just means to undo the code removal from me and adjusting all _instantiate methods. This moves the wrapping of BukkitMC*Events from _instantiate methods to the EventBuilder.