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.

Sign up for GitHub

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

Jump to bottom

Fix possible custom enchantment duplication #3864

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Fix possible custom enchantment duplication #3864

merged 6 commits into from
Jul 11, 2023

Conversation

justtos
Copy link
Contributor

@justtos justtos commented Jun 22, 2023 •
edited
Loading

Description

This PR fixes a possible duplication of custom enchantments due to inconsistencies in the removeEnchantment method of class ItemStack between Spigot and Paper due to Paper Server Patch 0069.

Has been tested on Spigot and Paper 1.19.2 only.

Proposed changes

Changed to method removeEnchant of class ItemMeta from the method removeEnchantment of class ItemStack, by also moving the item.setItemMeta(itemMeta) after the for loop.

Added a check and logger in hopes to prevent future enchantment duplication.

Related Issues (if applicable)

Resolves #3837

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.

@justtos justtos requested a review from a team as a code owner June 22, 2023 22:11
@JustAHuman-xD JustAHuman-xD added the ✨ Fix This Pull Request fixes an issue. label Jun 22, 2023
@justtos @Phoenix-Starlight
Fix comment spacing
b9f6a18 
Co-authored-by: Phoenix-Starlight <[email protected]>
J3fftw1
J3fftw1 suggested changes Jun 28, 2023
View reviewed changes
...usybiscuit/slimefun4/implementation/items/electric/machines/enchanting/AutoDisenchanter.java Outdated Show resolved Hide resolved
...usybiscuit/slimefun4/implementation/items/electric/machines/enchanting/AutoDisenchanter.java Outdated Show resolved Hide resolved
...usybiscuit/slimefun4/implementation/items/electric/machines/enchanting/AutoDisenchanter.java Outdated
meta.addStoredEnchant(entry.getKey(), entry.getValue(), true);
} else {
// Get Enchantment Name
Slimefun.logger().log(Level.SEVERE, "AutoDisenchanter has failed to remove enchantment \"{0}\"", entry.getKey().getKey().getKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

should we log the location as well for if its gonna spam this in console the admins can look into it?

Copy link
Contributor Author

@justtos justtos Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's needed as if it happens it's most likely going to happen for everyone.
Also not sure how I would get the location from inside the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think we should log the location.
Because something is likely to happen doesnt mean it will always happen.
not sure exactly how we can get the location tho.
so maybe scrap that idea or if others know please comment

Sfiguz7
Sfiguz7 reviewed Jun 28, 2023
View reviewed changes
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.

Agreed with Jeff's comments, other than that everything looks fine.
I am not marking this as approved yet as once you push your changes we will get a new temporary build automatically and can test in 1.20 too, since you mentioned you only tested in 1.19.2

@Sfiguz7 Sfiguz7 self-assigned this Jun 28, 2023
@Sefiraat Sefiraat self-assigned this Jun 28, 2023
@github-actions
Copy link
Contributor

Slimefun preview build

A Slimefun preview build is available for testing!

https://preview-builds.walshy.dev/download/Slimefun/3864/5401793866

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!

@justtos
Copy link
Contributor Author

justtos commented Jun 29, 2023

Using the preview build I have tested with both Spigot and Paper 1.20.1 and can confirm works normally.

@Sfiguz7 Sfiguz7 merged commit 2ef2e50 into Slimefun:master Jul 11, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sefiraat Sefiraat Sefiraat left review comments

@svr333 svr333 svr333 left review comments

@Phoenix-Starlight Phoenix-Starlight Phoenix-Starlight left review comments

@JustAHuman-xD JustAHuman-xD JustAHuman-xD left review comments

@Sfiguz7 Sfiguz7 Sfiguz7 approved these changes

@J3fftw1 J3fftw1 J3fftw1 approved these changes

Assignees

@Sefiraat Sefiraat

@Sfiguz7 Sfiguz7

@J3fftw1 J3fftw1

Labels
✨ Fix This Pull Request fixes an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Eco Enchants and Auto Disenchanter Enchantment Dupe
7 participants
@justtos @Sefiraat @svr333 @Sfiguz7 @J3fftw1 @Phoenix-Starlight @JustAHuman-xD