-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Adding Placeholder API Compability internally [WIP] #1944
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
Conversation
|
Yep it fucked up |
|
As for registering our own placeholder, https://github.com/PlaceholderAPI/Essentials-Expansion is already here, I think it is better to just continue to contribute there rather than putting them internally onto Essentials |
Not sure if this is necessary - thoughts?
This will need to be manually changed everywhere `tl` is used where placeholders may be expected.
|
Nice one. |
Banbeucmas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin.yml might need some work
| website: http://tiny.cc/EssentialsCommands | ||
| description: Provides an essential, core set of commands for Bukkit. | ||
| softdepend: [Vault] | ||
| softdepend: [Vault, PlaceholderAPI] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1945
You once said you don't want to add it as sofedepend.
Will we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, forgot about that. Still need to implement handling of PAPI loading after EssentialsX though before we do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave that part to you then :)
Banbeucmas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was aware of this tho
| return null; | ||
| } | ||
|
|
||
| input = input.replaceAll("\\{|\\}", "%"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be some problem that a single { standing alone will become % after that. I don't use regex for a long time. Should we do something about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/{([A-Za-z0-9_]+)}/g might work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@md678685 I love IntelliJ

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the replacement with that regex would have been "%$1%", since the ()s in the regex capture the placeholder name (bobsplugin_hunger) and the $1 puts it back in the string once replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how the problem still excist tho. Idk why but seem like Java Regex behaves different than JS
|
|
||
| input = input.replaceAll("\\{|\\}", "%"); | ||
|
|
||
| if (papiEnabled == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be here, in case PAPI loads before EssentialsX - in that instance, Essentials won't receive PAPI's PluginEnableEvent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is also the reason for the null - at the point that the class is loaded, we won't know whether PAPI is enabled, and it also might not be a safe time to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that make sense. Let me change that
| //The warp function is a wrapper used to teleportPlayer a player to a /warp | ||
| @Override | ||
| public void warp(IUser teleportee, String warp, Trade chargeFor, TeleportCause cause) throws Exception { | ||
| UserWarpEvent event = new UserWarpEvent(teleportee, warp, chargeFor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be in its own PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this change is in this PR but the actual PR is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I accidentally create the Papi branch out of the original branch of my fork and PR before even realize it.
We will just solve both PR at the same time or if anyone have a better idea.
|
Should we close this in favor of #2041? |
|
@Banbeucmas Yes, closing this PR now. |

Pretty sure I want this to be an enhancement in 2.16.0
Still WIP tho.
Just that the placeholder seem to work flawlessly with PlaceholderAPI 2.8.5 tested
I hope my pr didn't fucked up lol. Haven't try to merge specific branch yet