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 a TalismanActivatedEvent which fires when a Player uses a Talisman. #3920

Closed
wants to merge 12 commits into from

Conversation

Sniperkaos
Copy link
Contributor

@Sniperkaos Sniperkaos commented Jul 25, 2023

Description

I found it odd that there wasn't an event to listen for when Talismans are activated, and thought it would be
helpful for one to be added.

Proposed changes

Added TalismanActivationEvent, which extends PlayerEvent.
It is fired when the static function activateTalisman is called.

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@Sniperkaos Sniperkaos requested a review from a team as a code owner July 25, 2023 22:07
@github-actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@TheBusyBiscuit TheBusyBiscuit added the 🔧 API This Pull Request introduces API changes. label Jul 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 68fa4e85

https://preview-builds.walshy.dev/download/Slimefun/3920/68fa4e85

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

Copy link
Member

@TheBusyBiscuit TheBusyBiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea but some changes need to be made :)

@TheBusyBiscuit TheBusyBiscuit self-assigned this Jul 25, 2023
cworldstar and others added 4 commits July 26, 2023 08:22
…lismanActivatedEvent.java


changes variable names to camel case

Co-authored-by: TheBusyBiscuit <[email protected]>
…lismanActivatedEvent.java


changes the who variable to a more clear player variable

Co-authored-by: Sefiraat <[email protected]>
@Sniperkaos Sniperkaos marked this pull request as draft July 26, 2023 12:33


Conflicts:
	src/main/java/io/github/thebusybiscuit/slimefun4/api/events/TalismanActivatedEvent.java
talisman.sendMessage(p);
TalismanActivatedEvent TalismanEvent = new TalismanActivatedEvent(p, talisman, talismanItem);
Bukkit.getPluginManager().callEvent(TalismanEvent);
if(!TalismanEvent.isCancelled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(!TalismanEvent.isCancelled()) {
if (!TalismanEvent.isCancelled()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure I committed this but apparently the change hasn't gone through, I don't really know why

Sniperkaos and others added 2 commits July 26, 2023 14:12
…n/items/magical/talismans/Talisman.java


adds a space for readability

Co-authored-by: J3fftw <[email protected]>
…n/items/magical/talismans/Talisman.java


more spaces for readability

Co-authored-by: J3fftw <[email protected]>
@Sniperkaos Sniperkaos marked this pull request as ready for review July 26, 2023 18:13
…n/items/magical/talismans/Talisman.java


didnt commit the first time I guess

Co-authored-by: J3fftw <[email protected]>
*
*/

public class TalismanActivatedEvent extends PlayerEvent implements Cancellable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might wanna rename this.
when it’s cancelled it’s not activated.
So maybe TalismanActivatEvent Or something else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. I didn't even think of that.

@Phoenix-Starlight
Copy link
Member

Phoenix-Starlight commented Jul 27, 2023 via email

* @return The {@link Talisman} used.
*
*/
@Nonnull
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the style preferences, annotations that target the return type should be inline. Same for line 60.

Suggested change
@Nonnull
public @Nonnull Talisman getTalisman() {

@Sefiraat
Copy link
Member

Sefiraat commented Sep 3, 2023

@Sniperkaos Still working on the suggestions above or happy for it to be closed?

Sfiguz7 added a commit that referenced this pull request Dec 12, 2023
Co-authored-by: TheBusyBiscuit <[email protected]>
Co-authored-by: Sefiraat <[email protected]>
Co-authored-by: J3fftw <[email protected]>
Co-authored-by: Daniel Walsh <[email protected]>
Co-authored-by: China Worldstar <[email protected]>
Co-authored-by: cworldstar <[email protected]>
Co-authored-by: Alessio Colombo <[email protected]>
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Dec 12, 2023
@Sfiguz7 Sfiguz7 closed this Dec 12, 2023
@Sfiguz7 Sfiguz7 added Stale Marks a PR as stale. and removed ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! 🔧 API This Pull Request introduces API changes. labels Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Marks a PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants