search

oliverdunk / JukeboxAPI

Java API for Jukebox, my web based music player.
MIT License
19 stars 16 forks source link

Maven cleanup, use WorldGuardWrapper, fix region checks #49

Closed sgdc3 closed 4 years ago

sgdc3 commented 4 years ago

here we go! :)

Moutard3 commented 4 years ago

Great! Now you need to wait feedback from @oliverdunk ;)

oliverdunk commented 4 years ago

Great! Now you need to wait feedback from @oliverdunk ;)

Thanks for taking a look at this @Moutard3.

@sgdc3, thanks for your contribution! The fixes and refactoring here are great. I'm curious if there's a particular WorldGuard version you're introducing WorldGuardWrapper to support, or if you just thought it was a nice fit (it looks like you work on it)? I'm not against it, but our current solution is working well, and would allow us to support other plugins in the future.

Feel free to reply here. Splitting your MR up might be nice, so we can merge the other fixes sooner. Not essential though.

sgdc3 commented 4 years ago

@oliverdunk WGW won't stop you from adding hooks to other protection plugins, it just allows you to delegate the cross version support no-sense to a more mature cross-project library. 😃

oliverdunk commented 4 years ago

@sgdc3 I understand this, but our current approach is working well. While your project may be slightly older, our implementation has been around for a while too.

At the moment, I don't plan on merging this. I'd be happy to accept a PR with the other fixes, but I don't think this is the right time to adopt that library.

Going forward, please don't hesitate to reach out on Discord, or open an issue, before making a change. I appreciate the time you spent here, and would be happy to chat in the future so you don't work on something that doesn't end up getting accepted.

sgdc3 commented 4 years ago

I have no time to rework this PR atm sorry, i'll continue to use my fork as the current official release still has huge performance issues due to that PlayerMoveEvent listener logic error.

oliverdunk commented 4 years ago

I have no time to rework this PR atm sorry, i'll continue to use my fork as the current official release still has huge performance issues due to that PlayerMoveEvent listener logic error.

No worries, that's totally fine. It is a bad bug so I hope to release a fix with it soon.