Team Chest Mutation #90

DirkyJerky · 2018-06-09T07:23:26Z

No description provided.

PotatoStealer · 2018-06-09T08:39:49Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+import tc.oc.pgm.mutation.types.KitMutation;
+import tc.oc.pgm.wool.WoolMatchModule;
+
+import java.util.*;


No wildcard imports please

PotatoStealer · 2018-06-09T08:40:29Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+public class TeamChestMutation extends KitMutation {
+    final static int SLOT_ID = 17; // Top right
+    final static Material TOOL_TYPE = Material.ENDER_CHEST;


Do we need this extra field?

Because it is used six times, I believe so. So in the future it may be easy to change it if needed.

PotatoStealer · 2018-06-09T08:45:12Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+    public TeamChestMutation(Match match) {
+        super(match, false);
+        oWmm = Optional.ofNullable(match().getMatchModule(WoolMatchModule.class));


Match::getMatchModule is an @nullable. Is there a need for an Optional?

If you want the optional use match().module(...)

PotatoStealer · 2018-06-09T08:46:24Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        oWmm = Optional.ofNullable(match().getMatchModule(WoolMatchModule.class));
+        for (Party party : match().getParties()) {
+            if (party.isParticipatingType()) {
+                // Could the chest title be localized properly?


Yes it can, but you'll have to create an inventory view each time a player wishes to view it. See NavigatorInterface.

PotatoStealer · 2018-06-09T08:49:01Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        if (event.getItem().getType() != TOOL_TYPE) return;
+
+        Player bukkitPlayer = event.getPlayer();
+        if (bukkitPlayer == null) return;


Will this actually happen?

I don't know. I've seen some weird things with Bukkit events returning null when they shouldn't be, better safe than sorry.

PotatoStealer · 2018-06-09T08:49:53Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+            // If the item is in the off-hand slot, it wont get put back visually for the player without this.
+            if(event.getHand() == EquipmentSlot.OFF_HAND) event.getActor().updateInventory();
+            bukkitPlayer.openInventory(teamInventory);
+


Unnecessary newline

PotatoStealer · 2018-06-09T08:50:40Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        return false;
+    }
+
+    private Optional<Inventory> getTeamsInventory(Player bukkitPlayer) {


For our needs, would an @nullable possibly be better?

Can we take in a MatchPlayer instead of a bukkit Player object?

Optional's are preferred over null values

Use Optionals if it makes for cleaner code, if it makes things messier then you don't need to.

PotatoStealer · 2018-06-09T08:52:14Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        }
+
+        // If normal right click, in their inventory, on the chest, then open shared inventory.
+        getTeamsInventory(event.getActor()).ifPresent(teamInventory -> {


DRY, can we set a variable for event.getActor()?

PotatoStealer · 2018-06-09T08:53:52Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        return Optional.of(teamChests.get(team));
+    }
+
+    private Kit getKitForPlayer(MatchPlayer player) {


PGMTranslations is legacy. See RenderedItemBuilder.

Electroid

Great work, Dirky!

There are a couple of odds and ends to fix, but overall you're doing a great job. In general, simplicity in code is a must, so as you learn the codebase you'll find we have lots of utility methods to make your life as a programmer easier.

Let me know if you have any questions.

Electroid · 2018-06-09T15:50:19Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+    public TeamChestMutation(Match match) {
+        super(match, false);
+        oWmm = Optional.ofNullable(match().getMatchModule(WoolMatchModule.class));


If you want the optional use match().module(...)

Electroid · 2018-06-09T15:50:40Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+public class TeamChestMutation extends KitMutation {
+    final static int SLOT_ID = 17; // Top right
+    final static Material TOOL_TYPE = Material.ENDER_CHEST;
+    final static String ITEM_NAME_KEY = "mutation.type.teamchest.item_name";


The translation strings don't need to be statically defined

Electroid · 2018-06-09T15:51:32Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+    }
+
+    // Open shared inventory instead of placing the chest
+    @EventHandler(priority = EventPriority.HIGHEST)


Add to the annotation ignoreCancelled = true

Electroid · 2018-06-09T15:53:04Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        });
+    }
+
+    private boolean isItemEvil(ItemStack item) {


isBlacklistedItem

Electroid · 2018-06-09T15:57:20Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        // No putting evil items (ender chest, possibly wool) into the chest
+        Optional<Inventory> teamChest = getTeamsInventory(event.getActor());
+        if (teamChest.filter(teamInventory -> {
+            return teamInventory.equals(event.getView().getTopInventory());


Single line lambdas don't need the {} this whole block can be simplified:

if(getTeamsInventory(event.getActor()).filter(i -> i.equals(event.getView().getTopInventory())).isPresent() && isBlacklistedItem(event.getCurrentItem()) { event.setCancelled(true); return; }

Electroid · 2018-06-09T16:12:57Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+    final static String ITEM_LORE_KEY = "mutation.type.teamchest.item_lore";
+    final static int CHEST_SIZE = 27;
+
+    final Map<Party, Inventory> teamChests = new WeakHashMap<>();


Even though this is weak, you want to clear this disable()

Electroid · 2018-06-09T16:13:27Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+    final Map<Party, Inventory> teamChests = new WeakHashMap<>();
+
+    final Optional<WoolMatchModule> oWmm;


Just call it wools or optWools, oWmm is ambiguous.

Electroid · 2018-06-09T16:16:51Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+    public TeamChestMutation(Match match) {
+        super(match, false);
+        oWmm = Optional.ofNullable(match().getMatchModule(WoolMatchModule.class));
+        for (Party party : match().getParties()) {


Mutation modules are special, because they can be enabled or disabled on the fly. So this logic should go into

@Override public boolean enable() { super.enable(); for (Party party : match().getParties()) { // Rest of implementation } }

You will also have to make sure you implement disable() too.

Electroid · 2018-06-09T16:18:19Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        MatchPlayer player = match().getPlayer(bukkitPlayer);
+        Party team = player.getParty();
+
+        if (!team.isParticipating()) return Optional.empty();


Instead of a team check, you can check the player.
MatchPlayer player = match().participant(bukkitPlayer)

If they aren't participating, it will be optional absent.

Electroid · 2018-06-09T16:18:59Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+        if (!team.isParticipating()) return Optional.empty();
+
+        return Optional.of(teamChests.get(team));


Can teamChests.get(team) be null? If so you want Optional.ofNullable or else a runtime error will be thrown.

"""
I don't think you need to throw an exception if a chest isn't found, can't think of any cases where that should happen.
"""
--- Shiny

DirkyJerky · 2018-06-09T18:51:30Z

I updated things as per review.

I suggest using github's "Squash as you merge" feature here :P.

The ignoreCancelled = true is preventing the event handler from receiving any RIGHT_CLICK_AIR events, so should I remove the ignoreCancelled or just forgo right clicking air opening the chest?

PotatoStealer · 2018-06-10T02:28:52Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+    private Kit getKitForPlayer(MatchPlayer player) {
+        ItemStack stack = new ItemBuilder(item(TOOL_TYPE))
+                .name(ChatColor.DARK_PURPLE + PGMTranslations.t("mutation.type.teamchest.item_name", player))


Are we able to not use PGMTranslations?

I couldn't seem to get it to work yet,
new Component(new TranslatableComponent("mutation.type.teamchest.item_name"), ChatColor.DARK_PURPLE).getText()
doesn't work.

Neither did new Component(ChatColor.DARK_PURPLE).translate("mutation.type.teamchest.item_name").getText()

toLegacyText()

How do I get a ComponentRenderContext? That is what I assume I need (Readme says I need it), because nothing is translating it is just showing the translation key.

I can't inject it as I assume that is how it is normally recieved.

Mutations don't support injection right now, so using PGMTranslations should be fine.

Electroid · 2018-06-11T14:25:13Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+    @Override
+    public void disable() {
+        teamChests.clear();
+    }


Remember super.disable()

It's best to disable the current class, then the super class.

Electroid · 2018-06-11T14:25:30Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+        super.enable();
+        for (Party party : match().getParties()) {
+            if (party.isParticipatingType()) {
+                // Could the chest title be localized properly?


Make this a TODO

Electroid · 2018-06-11T14:27:24Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+
+    private Kit getKitForPlayer(MatchPlayer player) {
+        ItemStack stack = new ItemBuilder(item(TOOL_TYPE))
+                .name(ChatColor.DARK_PURPLE + PGMTranslations.t("mutation.type.teamchest.item_name", player))


Mutations don't support injection right now, so using PGMTranslations should be fine.

Electroid · 2018-06-11T14:31:15Z

You can forgo the ignoreCancelled and this is ready to ship after some minor fixes.

Electroid · 2018-06-21T20:09:00Z

PGM/src/main/java/tc/oc/pgm/mutation/types/kit/TeamChestMutation.java

+    @Override
+    public void disable() {
+        teamChests.clear();
+    }


It's best to disable the current class, then the super class.

AustinLMayes · 2018-11-21T00:42:01Z

@DirkyJerky any news?

DirkyJerky added 2 commits June 9, 2018 02:21

Team Chest mutation

9c1d871

Only objective wools blocked from chest

3c40914

PotatoStealer suggested changes Jun 9, 2018

View reviewed changes

Electroid suggested changes Jun 9, 2018

View reviewed changes

Electroid added the review label Jun 9, 2018

DirkyJerky added 15 commits June 9, 2018 12:48

No wildcard imports

bf6bd71

Use convenience method module

faad1cd

Tidy

8b038f6

Inline translation strings

d2dac49

ignoreCancelled = true

0edadff

isBlacklistedItem

839b4dc

Single-line lambda

f0e720b

Organize onChestUse and refactor onInventoryClick

261a032

Inline SLOT_ID

8748f45

Cleanup isBlacklistedItem

935b209

oBlah -> optBlah

952e20b

Better boolean logic with Optionals

da42262

enable() + disable()

a10f8e4

streamline getTeamsInventory

33863c3

Blep

2bf6472

PotatoStealer reviewed Jun 10, 2018

View reviewed changes

PotatoStealer approved these changes Jun 10, 2018

View reviewed changes

Electroid suggested changes Jun 11, 2018

View reviewed changes

DirkyJerky force-pushed the rebased branch from d110067 to 71ccd8a Compare June 11, 2018 16:29

Last round of fixes

9b27ca8

DirkyJerky force-pushed the rebased branch from 71ccd8a to 9b27ca8 Compare June 11, 2018 16:31

Electroid suggested changes Jun 21, 2018

View reviewed changes

Last fix

dcd7c1c


		final Map<Party, Inventory> teamChests = new WeakHashMap<>();

		final Optional<WoolMatchModule> oWmm;


		if (!team.isParticipating()) return Optional.empty();

		return Optional.of(teamChests.get(team));

Team Chest Mutation #90

Are you sure you want to change the base?

Team Chest Mutation #90

Conversation

DirkyJerky commented Jun 9, 2018

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

DirkyJerky Jun 9, 2018 • edited Loading

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

Electroid 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

Choose a reason for hiding this comment

DirkyJerky Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

DirkyJerky commented Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

DirkyJerky Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirkyJerky Jun 10, 2018 • edited Loading

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

Electroid commented Jun 11, 2018

Choose a reason for hiding this comment

AustinLMayes commented Nov 21, 2018

DirkyJerky Jun 9, 2018 •

edited

Loading

DirkyJerky Jun 9, 2018 •

edited

Loading

DirkyJerky commented Jun 9, 2018 •

edited

Loading

DirkyJerky Jun 10, 2018 •

edited

Loading

DirkyJerky Jun 10, 2018 •

edited

Loading