All NPCs should send scoreboard teams on player join

main/src/main/java/net/citizensnpcs/trait/ScoreboardTrait.java

@@ -19,13 +19,12 @@ import com.google.common.collect.HashMultimap;
 import com.google.common.collect.SetMultimap;
 
 import net.citizensnpcs.Settings.Setting;
+import net.citizensnpcs.api.CitizensAPI;
 import net.citizensnpcs.api.npc.NPC;
 import net.citizensnpcs.api.persistence.Persist;
 import net.citizensnpcs.api.trait.Trait;
 import net.citizensnpcs.api.trait.TraitName;
-import net.citizensnpcs.npc.ai.NPCHolder;
 import net.citizensnpcs.util.NMS;
-import net.citizensnpcs.util.PlayerUpdateTask;
 import net.citizensnpcs.util.Util;
 
 @TraitName("scoreboardtrait")
@@ -210,7 +209,7 @@ public class ScoreboardTrait extends Trait {
         }
 
         for (Player player : Bukkit.getOnlinePlayers()) {
-            if (player instanceof NPCHolder)
+            if (player.hasMetadata("NPC"))
                 continue;
             if (SENT_TEAMS.containsEntry(player.getUniqueId(), team.getName())) {
                 NMS.sendTeamPacket(player, team, 2);
@@ -222,14 +221,13 @@ public class ScoreboardTrait extends Trait {
     }
 
     public static void onPlayerJoin(PlayerJoinEvent event) {
-        for (Player npcPlayer : PlayerUpdateTask.getCurrentPlayerNPCs()) {
+        for (NPC npc : CitizensAPI.getNPCRegistry()) {
-            NPC npc = ((NPCHolder) npcPlayer).getNPC();
+            ScoreboardTrait trait = npc.getTraitNullable(ScoreboardTrait.class);
-
+            if (trait == null)
-            String teamName = npc.data().get(NPC.SCOREBOARD_FAKE_TEAM_NAME_METADATA, "");
+                continue;
-            Team team = null;
+            Team team = trait.getTeam();
-            if (teamName.length() == 0 || (team = Util.getDummyScoreboard().getTeam(teamName)) == null)
+            if (team == null || SENT_TEAMS.containsEntry(event.getPlayer().getUniqueId(), team.getName()))
                 continue;
-
             NMS.sendTeamPacket(event.getPlayer(), team, 0);
             SENT_TEAMS.put(event.getPlayer().getUniqueId(), team.getName());
         }

main/src/main/java/net/citizensnpcs/trait/ShopTrait.java

@@ -41,7 +41,7 @@ import net.citizensnpcs.api.util.Colorizer;
 import net.citizensnpcs.api.util.DataKey;
 import net.citizensnpcs.trait.shop.NPCShopAction;
 import net.citizensnpcs.trait.shop.NPCShopAction.GUI;
-import net.citizensnpcs.trait.shop.NPCShopAction.PendingAction;
+import net.citizensnpcs.trait.shop.NPCShopAction.Transaction;
 
 /**
  * Shop trait for NPC GUI shops.
@@ -247,29 +247,29 @@ public class ShopTrait extends Trait {
             }
         }
 
-        public boolean execute(List<NPCShopAction> actions, Function<NPCShopAction, PendingAction> func) {
+        public List<Transaction> execute(List<NPCShopAction> actions, Function<NPCShopAction, Transaction> func) {
-            List<PendingAction> pending = Lists.newArrayList();
+            List<Transaction> pending = Lists.newArrayList();
-            boolean success = true;
             for (NPCShopAction action : actions) {
-                PendingAction take = func.apply(action);
+                Transaction take = func.apply(action);
                 if (!take.isPossible()) {
                     pending.forEach(a -> a.rollback());
-                    success = false;
+                    return null;
-                    break;
                 } else {
                     take.run();
                     pending.add(take);
                 }
             }
-            return success;
+            return pending;
         }
 
         public void onClick(NPCShop shop, CitizensInventoryClickEvent event) {
             if (shop.type != ShopType.COMMAND) {
-                /* boolean success = execute(cost, action -> action.transfer(npc.getEntity(), event.getWhoClicked()));
+                List<Transaction> take = execute(cost, action -> action.take(event.getWhoClicked()));
-                if (success) {
+                if (take == null)
-                    execute(result, action -> action.transfer(event.getWhoClicked(), npc.getEntity()));
+                    return;
-                }*/
+                if (execute(result, action -> action.grant(event.getWhoClicked())) == null) {
+                    take.forEach(a -> a.rollback());
+                }
             }
         }
     }

main/src/main/java/net/citizensnpcs/trait/shop/NPCShopAction.java

@@ -12,15 +12,19 @@ import org.bukkit.Material;
 import org.bukkit.OfflinePlayer;
 import org.bukkit.entity.Entity;
 import org.bukkit.entity.Player;
+import org.bukkit.inventory.Inventory;
+import org.bukkit.inventory.InventoryHolder;
 import org.bukkit.inventory.ItemStack;
 
 import com.google.common.collect.Lists;
 
 import net.citizensnpcs.api.gui.InputMenus;
 import net.citizensnpcs.api.gui.InventoryMenuPage;
+import net.citizensnpcs.api.gui.MenuContext;
 import net.citizensnpcs.api.persistence.Persist;
 import net.citizensnpcs.api.persistence.PersistenceLoader;
 import net.citizensnpcs.api.persistence.PersisterRegistry;
+import net.citizensnpcs.api.util.Placeholders;
 import net.citizensnpcs.trait.shop.NPCShopAction.ItemAction.ItemActionGUI;
 import net.citizensnpcs.trait.shop.NPCShopAction.MoneyAction.MoneyActionGUI;
 import net.citizensnpcs.trait.shop.NPCShopAction.PermissionAction.PermissionActionGUI;
@@ -38,9 +42,9 @@ public abstract class NPCShopAction implements Cloneable {
         }
     }
 
-    public abstract PendingAction grant(Entity entity);
+    public abstract Transaction grant(Entity entity);
 
-    public abstract PendingAction take(Entity entity);
+    public abstract Transaction take(Entity entity);
 
     public static interface GUI {
         public InventoryMenuPage createEditor(NPCShopAction previous, Consumer<NPCShopAction> callback);
@@ -66,20 +70,30 @@ public abstract class NPCShopAction implements Cloneable {
         }
 
         @Override
-        public PendingAction grant(Entity entity) {
+        public Transaction grant(Entity entity) {
-            return PendingAction.create(() -> {
+            if (!(entity instanceof InventoryHolder))
-                return true;
+                return Transaction.fail();
+            Inventory source = ((InventoryHolder) entity).getInventory();
+            return Transaction.create(() -> {
+                return source.all(Material.AIR).size() > items.size();
             }, () -> {
+                source.addItem(items.toArray(new ItemStack[items.size()]));
             }, () -> {
+                source.removeItem(items.toArray(new ItemStack[items.size()]));
             });
         }
 
         @Override
-        public PendingAction take(Entity entity) {
+        public Transaction take(Entity entity) {
-            return PendingAction.create(() -> {
+            if (!(entity instanceof InventoryHolder))
-                return true;
+                return Transaction.fail();
+            Inventory source = ((InventoryHolder) entity).getInventory();
+            return Transaction.create(() -> {
+                return source.all(Material.AIR).size() > items.size();
             }, () -> {
+                source.removeItem(items.toArray(new ItemStack[items.size()]));
             }, () -> {
+                source.addItem(items.toArray(new ItemStack[items.size()]));
             });
         }
 
@@ -109,12 +123,12 @@ public abstract class NPCShopAction implements Cloneable {
         }
 
         @Override
-        public PendingAction grant(Entity entity) {
+        public Transaction grant(Entity entity) {
             if (!(entity instanceof OfflinePlayer))
-                return PendingAction.fail();
+                return Transaction.fail();
             Economy economy = Bukkit.getServicesManager().getRegistration(Economy.class).getProvider();
             OfflinePlayer player = (OfflinePlayer) entity;
-            return PendingAction.create(() -> {
+            return Transaction.create(() -> {
                 return true;
             }, () -> {
                 economy.depositPlayer(player, money);
@@ -124,12 +138,12 @@ public abstract class NPCShopAction implements Cloneable {
         }
 
         @Override
-        public PendingAction take(Entity entity) {
+        public Transaction take(Entity entity) {
             if (!(entity instanceof OfflinePlayer))
-                return PendingAction.fail();
+                return Transaction.fail();
             Economy economy = Bukkit.getServicesManager().getRegistration(Economy.class).getProvider();
             OfflinePlayer player = (OfflinePlayer) entity;
-            return PendingAction.create(() -> {
+            return Transaction.create(() -> {
                 return economy.has(player, money);
             }, () -> {
                 economy.withdrawPlayer(player, money);
@@ -180,40 +194,6 @@ public abstract class NPCShopAction implements Cloneable {
         }
     }
 
-    public static class PendingAction {
-        private final Runnable execute;
-        private final Supplier<Boolean> possible;
-        private final Runnable rollback;
-
-        public PendingAction(Supplier<Boolean> isPossible, Runnable execute, Runnable rollback) {
-            this.possible = isPossible;
-            this.execute = execute;
-            this.rollback = rollback;
-        }
-
-        public boolean isPossible() {
-            return possible.get();
-        }
-
-        public void rollback() {
-            rollback.run();
-        }
-
-        public void run() {
-            execute.run();
-        }
-
-        public static PendingAction create(Supplier<Boolean> isPossible, Runnable execute, Runnable rollback) {
-            return new PendingAction(isPossible, execute, rollback);
-        }
-
-        public static PendingAction fail() {
-            return new PendingAction(() -> false, () -> {
-            }, () -> {
-            });
-        }
-    }
-
     public static class PermissionAction extends NPCShopAction {
         @Persist
         public List<String> permissions = Lists.newArrayList();
@@ -226,54 +206,71 @@ public abstract class NPCShopAction implements Cloneable {
         }
 
         @Override
-        public PendingAction grant(Entity entity) {
+        public Transaction grant(Entity entity) {
             if (!(entity instanceof Player))
-                return PendingAction.fail();
+                return Transaction.fail();
             Player player = (Player) entity;
             Permission perm = Bukkit.getServicesManager().getRegistration(Permission.class).getProvider();
-            return PendingAction.create(() -> {
+            return Transaction.create(() -> {
                 return true;
             }, () -> {
                 for (String permission : permissions) {
-                    perm.playerAdd(player, permission);
+                    perm.playerAdd(player, Placeholders.replace(permission, player));
                 }
             }, () -> {
                 for (String permission : permissions) {
-                    perm.playerRemove(player, permission);
+                    perm.playerRemove(player, Placeholders.replace(permission, player));
                 }
             });
         }
 
         @Override
-        public PendingAction take(Entity entity) {
+        public Transaction take(Entity entity) {
             if (!(entity instanceof Player))
-                return PendingAction.fail();
+                return Transaction.fail();
             Player player = (Player) entity;
             Permission perm = Bukkit.getServicesManager().getRegistration(Permission.class).getProvider();
-            return PendingAction.create(() -> {
+            return Transaction.create(() -> {
                 for (String permission : permissions) {
-                    if (!perm.playerHas(player, permission)) {
+                    if (!perm.playerHas(player, Placeholders.replace(permission, player))) {
                         return false;
                     }
                 }
                 return true;
             }, () -> {
                 for (String permission : permissions) {
-                    perm.playerRemove(player, permission);
+                    perm.playerRemove(player, Placeholders.replace(permission, player));
                 }
             }, () -> {
                 for (String permission : permissions) {
-                    perm.playerAdd(player, permission);
+                    perm.playerAdd(player, Placeholders.replace(permission, player));
                 }
             });
         }
 
+        public static class PermissionActionEditor extends InventoryMenuPage {
+            private NPCShopAction base;
+            private Consumer<NPCShopAction> callback;
+
+            public PermissionActionEditor() {
+            }
+
+            public PermissionActionEditor(NPCShopAction base, Consumer<NPCShopAction> callback) {
+                this.base = base;
+                this.callback = callback;
+            }
+
+            @Override
+            public void initialise(MenuContext ctx) {
+            }
+        }
+
         public static class PermissionActionGUI implements GUI {
             private Boolean supported;
 
             @Override
             public InventoryMenuPage createEditor(NPCShopAction previous, Consumer<NPCShopAction> callback) {
-                return null;
+                return new PermissionActionEditor(previous, callback);
             }
 
             @Override
@@ -298,6 +295,40 @@ public abstract class NPCShopAction implements Cloneable {
         }
     }
 
+    public static class Transaction {
+        private final Runnable execute;
+        private final Supplier<Boolean> possible;
+        private final Runnable rollback;
+
+        public Transaction(Supplier<Boolean> isPossible, Runnable execute, Runnable rollback) {
+            this.possible = isPossible;
+            this.execute = execute;
+            this.rollback = rollback;
+        }
+
+        public boolean isPossible() {
+            return possible.get();
+        }
+
+        public void rollback() {
+            rollback.run();
+        }
+
+        public void run() {
+            execute.run();
+        }
+
+        public static Transaction create(Supplier<Boolean> isPossible, Runnable execute, Runnable rollback) {
+            return new Transaction(isPossible, execute, rollback);
+        }
+
+        public static Transaction fail() {
+            return new Transaction(() -> false, () -> {
+            }, () -> {
+            });
+        }
+    }
+
     public static Iterable<GUI> getGUIs() {
         return GUI.values();
     }

main/src/main/java/net/citizensnpcs/util/PlayerUpdateTask.java

@@ -75,10 +75,6 @@ public class PlayerUpdateTask extends BukkitRunnable {
         PLAYERS_PENDING_REMOVE.add(entity);
     }
 
-    public static Iterable<Player> getCurrentPlayerNPCs() {
-        return PLAYERS.values();
-    }
-
     public static void registerPlayer(org.bukkit.entity.Entity entity) {
         PLAYERS_PENDING_REMOVE.remove(entity);
         PLAYERS_PENDING_ADD.add(entity);