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

Adds support for Shopkeeper plugin #1200

Closed
wants to merge 1 commit into from

Conversation

Aquaxoc
Copy link

@Aquaxoc Aquaxoc commented Jan 12, 2021

Adds support for Shopkeeper plugin (#145)

Checks if the entity is a shopkeeper with metadata and added an entry in the config.

@RoboMWM
Copy link

RoboMWM commented Jan 13, 2021

Like I do with PrettySimpleShop, the shop plugin should probably not ignore the event if it's canceled. This will also automatically make it compatible with other region plugins as well. There are no use cases that I know of where you'd want a plugin to blanket-deny access to shops.

Though - if you're trying to avoid the error message, then it should listen at a lower priority and cancel.

@RoboMWM RoboMWM closed this Jan 13, 2021
@ElementW
Copy link

ElementW commented Jan 14, 2021

@RoboMWM

the shop plugin should probably not ignore the event if it's canceled

This incites plugins to disregard the incremental checks & cancellation the API provides to have plugins like GP work in the first place. It is not the responsibility of higher priority plugins to accommodate, as part of their normal flow of operation, quirks of lower priority protection plugins because of their lack of ability to create check exclusion rules. GP has neither declarative (YAML) or API ways to request a specific type of entity and/or metadata to bypass checks, and that's a shame.

If this PR was made generic and allows for configurable Entity.hasMetadata()-based check bypasses, would you accept it?

if you're trying to avoid the error message, then it should listen at a lower priority and cancel.

https://github.com/TechFortress/GriefPrevention/blob/5a70d694e7accde180c7d805ec5d83f4a08551fb/src/main/java/me/ryanhamshire/GriefPrevention/PlayerEventHandler.java#L1166-L1167

https://github.com/TechFortress/GriefPrevention/blob/5a70d694e7accde180c7d805ec5d83f4a08551fb/src/main/java/me/ryanhamshire/GriefPrevention/PlayerEventHandler.java#L1315

There is no lower level than LOWEST, and you should know that the Bukkit/Spigot API & plugin system make no guarantee of run order on the same priority level, making your suggestion wonky at best since results will be unpredictable, which is exactly what @Aquaxoc and I observered between multiple server configurations as Shopkeepers already listens on LOWEST on the particular entity interact event that's causing trouble (LivingEntityShopListener.java:75-76).

@RoboMWM
Copy link

RoboMWM commented Jan 14, 2021

event priorities will be changed in v19 (or whenever the event funnel is implemented). This will also include a way to make GP ignore based on whatever you want as well. See dev/v20 branch for an example of such an implementation.

I'll take a PR to move it to LOW though. (That being said, there is a guarantee - if your plugin loads/registers before another, then your event handler will fire before another on the same priority; you can also loadbefore too.)

GP has neither declarative (YAML) or API ways to request a specific type of entity and/or metadata to bypass checks, and that's a shame.

That's coming with v18.

@RoboMWM
Copy link

RoboMWM commented Jan 14, 2021

I could also accept a PR like this in the meantime, probably without a config option I guess.

@blablubbabc
Copy link

blablubbabc commented Jan 27, 2021

I happen to come across this PR (I have several plugin users that run into the issue this PR intends to resolve from time to time) and wanted to note that I might have come up with another (and more generally applicable) solution.
The Shopkeepers plugin already has a 'loadbefore' entry to load before the GriefPrevention plugin (which already is not an ideal solution, because it requires being aware of conflicting plugins), so that Shopkeepers' event handlers execute first, cancel the event, and can thereby cause GriefPrevention to ignore the event. However, this broke whenever the Shopkeepers plugin has been dynamically reloaded (which re-registered its event handlers and thereby moved them to the end of the already registered event handlers of other plugins, such as GriefPrevention).
The next version of Shopkeepers will now forcefully re-register all other plugins' event handlers whenever it is reloaded, so that they get moved to the back of the registered listeners as well (this should be transparent to those other plugins, and also preserve their order among each other). This should ensure that Shopkeepers' event handler(s) always execute first, even after being dynamically reloaded, and thereby resolve this issue. A quick test on my side has confirmed that this seems to work as intended.
(If there are plugins that actually want to execute before Shopkeepers' event handlers or want to cancel its action, there are other means available for that)

Edit: However, regardless of this particular issue potentially being resolved then, this shouldn't hinder GriefPrevention to reconsider its used event priorities. For instance, if I remember correctly, WorldGuard uses normal event priorities for its checks, thereby allowing other plugins to pick if they want to execute before that (and thereby providing behavior that is more specific than the quite general land protection behavior) or after that (to take into account if the event has already been cancelled by another plugin).

@RoboMWM
Copy link

RoboMWM commented Jan 27, 2021

Yea, very likely will be moving checks to LOW priority.

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

Successfully merging this pull request may close these issues.

None yet

4 participants