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

[Api] Give & Pick Slimefun Item api #4252

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JustAHuman-xD
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD commented Oct 24, 2024

Description

Addons with complex SlimefunItem's that can have data different for the same slimefun item often have problems with the middle click & give item command, as they may lack some data that would be there when attained in survival, this PR adds 2 methods to help solve that problem.

Proposed changes

  • Added SlimefunItem#getGiveItemResult(Player)
    • Defaults to getItem and is called in GiveCommand in place of getItem
  • Added SlimefunItem#getPickBlockResult(Player, Block)
    • Also defaults to getItem and is called in MiddleClickListener in place of getItem
      • Also adds an instanceof to MiddleClickListener as the event returns HumanEntity not Player and I didn't want it to be inconsistent, testing is needed to make sure it still works

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.

- SlimefunItem#getGiveItemResult(Player)
- SlimefunItem#getPickBlockResult(HumanEntity, Block)
@JustAHuman-xD JustAHuman-xD requested a review from a team as a code owner October 24, 2024 14:04
@github-actions github-actions bot added the 🔧 API This Pull Request introduces API changes. label Oct 24, 2024
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 8f66bf3

https://preview-builds.walshy.dev/download/Slimefun/4252/8f66bf32

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!

WalshyDev
WalshyDev previously approved these changes Oct 24, 2024
Copy link
Member

@WalshyDev WalshyDev left a comment

Choose a reason for hiding this comment

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

I really like this, great job!

I'm not the biggest fan of giveItem since /sf give is a command not just a special mc thingy but, I think it's fine. Generally, I try to make APIs that work first-party the same as they do third-party. So, any addon could do something similar rather than it being a special Slimefun hardcoded thing

For this situation, I think it's fine. I haven't seen others asking for a similar functionality and if someone wants to make one, we can easily move over.

TheBusyBiscuit
TheBusyBiscuit previously approved these changes Oct 24, 2024
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.

Also a fan of this, simple but a very useful addition to the API 👍🏻
Thanks!

…n/listeners/MiddleClickListener.java

Co-authored-by: Daniel Walsh <walshydev@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants