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

Add TalismanActivateEvent (Updated version of #3920) #4045

Merged
merged 25 commits into from
Dec 12, 2023

Conversation

JustAHuman-xD
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD commented Dec 7, 2023

Description

I've made this pull request to update and apply some suggestions from #3920
I also have a couple of ideas of things to add to the PR but I'm going to ask for opinions on those beforehand

Proposed changes

  • In `Talisman#trigger(Event, SlimefunItem, boolean) I changed the logic behind using ender talismans
    • Instead of passing the normal talisman to the Talisman#activateTalisman method it now passes the ender talisman's talisman (mouthful I know)
    • The reason behind this change is to ensure that TalismanActivateEvent#getTalisman will properly reflect wether an ender or normal talisman was used
  • Added the TalismanActivateEvent
    • Extends PlayerEvent and implements Cancellable
    • Called when any talisman is activated by the Talisman#activateTalisman event
    • Contains The Player, Talisman, and ItemStack used when activating a talisman
  • In Talisman#activateTalisman call the new event
    • if the event is not cancelled continue with normal operations
    • only consume the talisman if the event preventsConsumption is false (which is the default value)
  • Added TestActivateTalismanEvent (Blame sfiguz for any problems with it (half joking))
    • Tests that the event properly fires for normal and ender talismans
    • Tests that the event has the expected fields with normal and ender talismans
    • Tests that the event can be cancelled
    • Tests that the event can prevent consumption

Related Issues (if applicable)

N/A

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 and others added 14 commits July 25, 2023 10:16
Activates when a Talisman is activated.
…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]>


Conflicts:
	src/main/java/io/github/thebusybiscuit/slimefun4/api/events/TalismanActivatedEvent.java
…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]>
…n/items/magical/talismans/Talisman.java


didnt commit the first time I guess

Co-authored-by: J3fftw <[email protected]>
@JustAHuman-xD JustAHuman-xD requested a review from a team as a code owner December 7, 2023 20:27
@github-actions github-actions bot added the 🔧 API This Pull Request introduces API changes. label Dec 7, 2023
Copy link
Contributor

github-actions bot commented Dec 7, 2023

Your Pull Request was automatically labelled as: "🔧 API"
Thank you for contributing to this project! ❤️

@JustAHuman-xD JustAHuman-xD added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Dec 7, 2023
@JustAHuman-xD
Copy link
Contributor Author

The couple of ideas I had that I wanted opinions on:

  • Adding a field to the event that controls whether or not the talisman should be consumed
  • (I had another one but have currently forgotten it)

Copy link
Contributor

github-actions bot commented Dec 7, 2023

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: c43dad64

https://preview-builds.walshy.dev/download/Slimefun/4045/c43dad64

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!

Co-authored-by: Daniel Walsh <[email protected]>
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment

Choose a reason for hiding this comment

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

Requesting change as I don't see a reason not to add the optional preventConsumption flag. Adding a test request while I am at it

EDIT: Justin added the needs testing already

@Sfiguz7 Sfiguz7 self-assigned this Dec 8, 2023
Sfiguz7
Sfiguz7 previously approved these changes Dec 8, 2023
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment

Choose a reason for hiding this comment

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

Spent some time in VC and all looks good, especially the tests <3

@J3fftw1
Copy link
Contributor

J3fftw1 commented Dec 9, 2023

can we also have tests that the endertalisman doesnt work in a normal inventory and for the normal talisman not to work in an enderchest

Sfiguz7
Sfiguz7 previously approved these changes Dec 12, 2023
@J3fftw1 J3fftw1 closed this Dec 12, 2023
@J3fftw1 J3fftw1 reopened this Dec 12, 2023
@J3fftw1 J3fftw1 closed this Dec 12, 2023
@J3fftw1 J3fftw1 reopened this Dec 12, 2023
@Sfiguz7 Sfiguz7 closed this Dec 12, 2023
@Sfiguz7 Sfiguz7 reopened this Dec 12, 2023
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment

Choose a reason for hiding this comment

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

Don't worry about the list of commits above, for some reason I was unable to push to Justin's fork and I am an impatient blueberry so I copied over my changes a piece at a time. Also didn't know about batch committing suggested changes so there you go

@Sfiguz7 Sfiguz7 added Build tested Used to indicate the PR preview build has been tested by one of the team and removed 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Dec 12, 2023
Copy link
Contributor

@J3fftw1 J3fftw1 left a comment

Choose a reason for hiding this comment

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

Looks good to alessio

@Sfiguz7 Sfiguz7 merged commit 88ac05f into Slimefun:master Dec 12, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 API This Pull Request introduces API changes. Build tested Used to indicate the PR preview build has been tested by one of the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants