Cleanup of enchantments #1593

parlough · 2017-11-12T10:43:57Z

SpongeAPI | SpongeCommon | SpongeVanilla | SpongeForge

Read API PR for more information.

liach · 2017-11-12T17:25:31Z

src/main/java/org/spongepowered/common/mixin/core/item/MixinEnchantment.java

    public String enchantment$getName() {
-        return shadow$getName();
+        return I18n.translateToLocal(shadow$getName());


Is this change intended?

Yeah as the getName method previously returned the translation key of "enchantment." + name. For example "enchantment.waterWalker" or similar.

liach · 2017-11-12T17:26:29Z

src/main/java/org/spongepowered/common/registry/type/data/KeyRegistryModule.java

@@ -436,7 +436,7 @@ public void registerDefaults() {

        this.fieldMap.put("last_date_played", makeSingleKey(TypeTokens.INSTANT_TOKEN, TypeTokens.INSTANT_VALUE_TOKEN, of("LastTimePlayed"), "sponge:last_time_played", "Last Time Played"));

-        this.fieldMap.put("hide_enchantments", makeSingleKey(TypeTokens.BOOLEAN_TOKEN, TypeTokens.BOOLEAN_VALUE_TOKEN, of("HideEnchantments"), "sponge:hide_enchantments", "Hide Enchantments"));
+        this.fieldMap.put("hide_enchantments", makeSingleKey(TypeTokens.BOOLEAN_TOKEN, TypeTokens.BOOLEAN_VALUE_TOKEN, of("HideEnchantments"), "sponge:hide_enchantments", "Hide EnchantmentTypes"));


This one is not that much...

liach · 2017-11-12T17:27:01Z

src/main/java/org/spongepowered/common/registry/type/data/KeyRegistryModule.java

@@ -412,7 +412,7 @@ public void registerDefaults() {

        this.fieldMap.put("persists", makeSingleKey(TypeTokens.BOOLEAN_TOKEN, TypeTokens.BOOLEAN_VALUE_TOKEN, of("Persists"), "sponge:persists", "Persists"));

-        this.fieldMap.put("stored_enchantments", makeListKey(TypeTokens.LIST_ITEM_ENCHANTMENT_TOKEN, TypeTokens.LIST_ITEM_ENCHANTMENT_VALUE_TOKEN, of("StoredEnchantments"), "sponge:stored_enchantments", "Stored Enchantments"));
+        this.fieldMap.put("stored_enchantments", makeListKey(TypeTokens.LIST_ITEM_ENCHANTMENT_TOKEN, TypeTokens.LIST_ITEM_ENCHANTMENT_VALUE_TOKEN, of("StoredEnchantments"), "sponge:stored_enchantments", "Stored EnchantmentTypes"));


I think you should change these names.

liach · 2017-11-12T17:27:19Z

...main/java/org/spongepowered/common/registry/type/event/DamageModifierTypeRegistryModule.java

        this.modifierTypeMap.put("offensive_potion_effect", new SpongeDamageModifierType("Offensive PotionEffect", "offensive_potion_effect"));
        this.modifierTypeMap.put("defensive_potion_effect", new SpongeDamageModifierType("Defensive PotionEffect", "defensive_potion_effect"));
        this.modifierTypeMap.put("negative_potion_effect", new SpongeDamageModifierType("Negative PotionEffect", "negative_potion_effect"));
        this.modifierTypeMap.put("hard_hat", new SpongeDamageModifierType("Hard Hat", "hard_hat"));
        this.modifierTypeMap.put("shield", new SpongeDamageModifierType("Shield", "shield"));
        this.modifierTypeMap.put("blocking", this.modifierTypeMap.get("shield"));
        this.modifierTypeMap.put("armor", new SpongeDamageModifierType("Armor", "armor"));
-        this.modifierTypeMap.put("armor_enchantment", new SpongeDamageModifierType("Armor Enchantment", "armor_enchantment"));
+        this.modifierTypeMap.put("armor_enchantment", new SpongeDamageModifierType("Armor EnchantmentType", "armor_enchantment"));


Not these two

parlough · 2017-11-12T17:44:21Z

testplugins/src/main/java/org/spongepowered/test/CustomInventoryTest.java

@@ -67,6 +69,20 @@
 public class CustomInventoryTest {

    @Listener
+    public void onBlockInteract(InteractBlockEvent event, @Root Player player) {


This was for some testing, will remove this later.

parlough · 2017-11-22T05:07:21Z

One second, will fix the issue...

gabizou

Small ages.

gabizou · 2017-11-22T05:44:08Z

src/main/java/org/spongepowered/common/data/util/DataQueries.java

@@ -64,8 +64,8 @@
    public static final DataQuery VELOCITY_Y = of("Y");
    public static final DataQuery VELOCITY_Z = of("Z");

-    // Enchantments
-    public static final DataQuery ENCHANTMENT_ID = of("Enchantment");


This may need to be deprecated legacy versioned

gabizou · 2017-11-22T05:47:12Z

src/main/java/org/spongepowered/common/item/enchantment/SpongeEnchantmentBuilder.java

+
+    @Override
+    public Enchantment.Builder from(final Enchantment value) {
+        this.enchantmentType = value.getType();


Needs a null check.

gabizou · 2017-11-22T05:47:57Z

src/main/java/org/spongepowered/common/item/enchantment/SpongeEnchantmentBuilder.java

+    @Override
+    protected Optional<Enchantment> buildContent(DataView container) throws InvalidDataException {
+        checkNotNull(container, "The data view cannot be null!");
+        if (container.contains(Queries.ENCHANTMENT_ID, Queries.LEVEL)) {


Invert the if block, style change, I know, just preference. Fail fast.

gabizou · 2017-11-22T05:48:13Z

src/main/java/org/spongepowered/common/item/enchantment/SpongeEnchantmentBuilder.java

+        if (container.contains(Queries.ENCHANTMENT_ID, Queries.LEVEL)) {
+            final String id = DataUtil.getData(container, Queries.ENCHANTMENT_ID, String.class);
+            final int level = DataUtil.getData(container, Queries.LEVEL, Integer.class);
+            final Optional<EnchantmentType> enchantmentType = SpongeImpl.getRegistry().getType(EnchantmentType.class, id);


SpongeImpl vs Sponge?

gabizou · 2017-11-22T05:49:26Z

src/main/java/org/spongepowered/common/mixin/core/item/MixinEnchantment.java

-@Implements(@Interface(iface = Enchantment.class, prefix = "enchantment$"))
-public abstract class MixinEnchantment implements Enchantment, IMixinEnchantment {
+@Implements(@Interface(iface = EnchantmentType.class, prefix = "enchantment$"))
+public abstract class MixinEnchantment implements IMixinEnchantment {


Why remove it? It'll still perfectly implement just fine normally.

IMixinEnchantment extends Enchantment so I didn't think it was needed.

Thinking about it further, the IMixin interfaces shouldn't extend the API interfaces for the basic understanding that they are only from implementation, The only reason why there's IMixinEntity and a few others doing this is because of Java compiler requirements with potentially ambiguous methods.

gabizou · 2017-11-22T05:49:59Z

src/main/resources/META-INF/common_at.cfg

@@ -20,7 +20,7 @@ public net.minecraft.command.EntitySelector func_82381_h(Ljava/lang/String;)Ljav
 public net.minecraft.crash.CrashReport field_71511_b # cause
 public net.minecraft.crash.CrashReportCategory field_85078_a # crashReport

-public net.minecraft.enchantment.Enchantment field_180311_a # enchantmentsList
+public net.minecraft.enchantmentType.Enchantment field_180311_a # enchantmentsList


gabizou · 2017-11-23T16:17:05Z

src/main/java/org/spongepowered/common/mixin/core/item/MixinEnchantment.java

-@Implements(@Interface(iface = Enchantment.class, prefix = "enchantment$"))
-public abstract class MixinEnchantment implements Enchantment, IMixinEnchantment {
+@Implements(@Interface(iface = EnchantmentType.class, prefix = "enchantment$"))
+public abstract class MixinEnchantment implements IMixinEnchantment {


Thinking about it further, the IMixin interfaces shouldn't extend the API interfaces for the basic understanding that they are only from implementation, The only reason why there's IMixinEntity and a few others doing this is because of Java compiler requirements with potentially ambiguous methods.

parlough requested a review from gabizou as a code owner November 12, 2017 10:43

This was referenced Nov 12, 2017

Cleanup of enchantments SpongePowered/SpongeVanilla#361

Closed

Cleanup of enchantments SpongePowered/SpongeForge#1847

Closed

Cleanup of enchantments SpongePowered/SpongeAPI#1687

Merged

liach reviewed Nov 12, 2017

View reviewed changes

parlough commented Nov 12, 2017

View reviewed changes

parlough changed the title ~~[WIP] Cleanup of enchantments~~ Cleanup of enchantments Nov 16, 2017

parlough force-pushed the feature/enchantment-clean branch from ba802cc to 49b67af Compare November 22, 2017 05:03

parlough force-pushed the feature/enchantment-clean branch from 172ac49 to ba802cc Compare November 22, 2017 05:09

gabizou requested changes Nov 22, 2017

View reviewed changes

gabizou requested changes Nov 23, 2017

View reviewed changes

ryantheleach assigned gabizou and parlough Nov 27, 2017

ryantheleach added system: data type: enhancement An improvement version: 1.12 (u) API: 7 (unsupported since May 21st 2021) labels Nov 27, 2017

ryantheleach added this to the API 7.0 milestone Nov 27, 2017

parlough force-pushed the feature/enchantment-clean branch from 9e05be5 to 7aa274b Compare December 1, 2017 06:16

Implement enchantment cleanup

9e9e94f

parlough force-pushed the feature/enchantment-clean branch from 7aa274b to 9e9e94f Compare December 1, 2017 06:37

parlough merged commit f085786 into SpongePowered:bleeding Dec 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of enchantments #1593

Cleanup of enchantments #1593

parlough commented Nov 12, 2017 •

edited

liach Nov 12, 2017

parlough Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

liach Nov 12, 2017

parlough Nov 12, 2017

parlough commented Nov 22, 2017

gabizou left a comment

gabizou Nov 22, 2017

gabizou Nov 22, 2017

gabizou Nov 22, 2017

gabizou Nov 22, 2017

gabizou Nov 22, 2017

parlough Nov 22, 2017

gabizou Nov 23, 2017

gabizou Nov 22, 2017

gabizou Nov 23, 2017

Cleanup of enchantments #1593

Cleanup of enchantments #1593

Conversation

parlough commented Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough commented Nov 22, 2017

gabizou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough commented Nov 12, 2017 •

edited