Slimefun4 icon indicating copy to clipboard operation
Slimefun4 copied to clipboard

SlimefunItemStack reimplementation

Open Intybyte opened this issue 1 year ago โ€ข 1 comments

Description

We can't extend itemstack anymore we need to fix EVERYTHING

Proposed changes

Use a delegate instead of extending itemstack

Related Issues (if applicable)

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.

Intybyte avatar Oct 22 '24 20:10 Intybyte

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! ๐Ÿ‘€

github-actions[bot] avatar Oct 22 '24 20:10 github-actions[bot]

Really not a fan of getDelegate tbh, not sure a great other solution tho

JustAHuman-xD avatar Oct 25 '24 08:10 JustAHuman-xD

Once this works i will try replacing it with google's ForwardingObject rather than adding a new dependency, similar to how many paper classes are implemented so to not get surprises later on

(Edit) this breaks a lot of compatibility compared to using lombok as you need to first get the delegate and then run the methods on the delegate, instead lombok generates code to access said delegate methods directly

Intybyte avatar Oct 25 '24 11:10 Intybyte

Slimefun preview build

A Slimefun preview build is available for testing! Commit: 3adafcdc

https://preview-builds.walshy.dev/download/Slimefun/4251/3adafcdc

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!

github-actions[bot] avatar Nov 09 '24 19:11 github-actions[bot]

No idea why it still says Merge Conflicts when i already solved those, it looks like there are some problems, rewriting them here for consistency

  • ~~SoulBound tests don't pass yet~~
  • ~~Locked items needs a new implementation, their tests are temporarily disabled~~ Now creating defensive copies
  • ~~Run delombok to remove the dependency that I put on SlimefunItemStack~~

Intybyte avatar Nov 12 '24 10:11 Intybyte

tested this and everything seems to be working fine. no errors in console and all items working as intended.

Boomer-1 avatar Nov 18 '24 17:11 Boomer-1

WrongItemStackException isn't used anymore, should it be removed altogether? also SlimefunItem#isItemStackImmutable is now useless as I am returning defensive copies, should I remove that one too?

Intybyte avatar Nov 21 '24 21:11 Intybyte

WrongItemStackException isn't used anymore, should it be removed altogether? also SlimefunItem#isItemStackImmutable is now useless as I am returning defensive copies, should I remove that one too?

Search for the exception and the function, and the result is only Slimefun core and its fork. No other plugins use them, so I say just remove them.

ybw0014 avatar Nov 22 '24 02:11 ybw0014

Pull request made: https://github.com/Intybyte/Slimefun4-Fixes/pull/3 or you want to use patch: https://discord.com/channels/565557184348422174/565570276038017044/1309652382279401605

ybw0014 avatar Nov 23 '24 07:11 ybw0014

retested 11-24-24 all seems to be working fine on preview build 6d38511a

Boomer-1 avatar Nov 25 '24 03:11 Boomer-1

No need to retest from 0 the last commit as it just removes unused code

Intybyte avatar Nov 25 '24 13:11 Intybyte

We've done a ton of testing, had no issues, everything is working perfectly

orcachillin avatar Dec 02 '24 22:12 orcachillin

What is preventing this from being merged?

JustinDevB avatar Dec 11 '24 15:12 JustinDevB

Starting to look like someone is too lazy to press merge

JustinDevB avatar Jan 04 '25 18:01 JustinDevB

Moving bundle features to another PR as they are out of scope here #4278

Intybyte avatar Jan 09 '25 13:01 Intybyte