Add a more strict form of IPocketAccess.getEntity

Before IPocketAccess.getEntity would return the entity which last held
fthis computer, even if not holding it any more. As
ba823bae13 describes, this caused
pocket.equip/pocket.unequip to dupe items.

We move the validation from the PocketAPI into the main IPocketAccess
implementation, to ensure this issue does not occur elsewhere. Note, we
require a separate method, as this is no longer thread-safe.

We also now return ok, err instead of throwing an exception, in order to
be consistent with the turtle functions. See dan200/ComputerCraft#328.

src/main/java/dan200/computercraft/api/pocket/IPocketAccess.java

@@ -24,10 +24,22 @@ public interface IPocketAccess
      * Gets the entity holding this item.
      *
      * @return The holding entity. This may be {@code null}.
+     * @deprecated Use {@link #getValidEntity()} where possible.
      */
     @Nullable
+    @Deprecated
     Entity getEntity();
 
+    /**
+     * Gets the entity holding this item with additional safety checks.
+     *
+     * This must be called on the server thread.
+     *
+     * @return The holding entity, or {@code null} if none exists.
+     */
+    @Nullable
+    Entity getValidEntity();
+
     /**
      * Get the colour of this pocket computer as a RGB number.
      *

src/main/java/dan200/computercraft/shared/pocket/apis/PocketAPI.java

@@ -12,9 +12,9 @@ import dan200.computercraft.api.lua.LuaException;
 import dan200.computercraft.api.pocket.IPocketUpgrade;
 import dan200.computercraft.shared.PocketUpgrades;
 import dan200.computercraft.shared.pocket.core.PocketServerComputer;
-import dan200.computercraft.shared.pocket.items.ItemPocketComputer;
 import dan200.computercraft.shared.util.InventoryUtil;
 import dan200.computercraft.shared.util.WorldUtil;
+import net.minecraft.entity.Entity;
 import net.minecraft.entity.player.EntityPlayer;
 import net.minecraft.entity.player.InventoryPlayer;
 import net.minecraft.item.ItemStack;
@@ -59,20 +59,10 @@ public class PocketAPI implements ILuaAPI
                 // equipBack
                 return context.executeMainThreadTask( () ->
                 {
-                    if( !(m_computer.getEntity() instanceof EntityPlayer) )
+                    Entity entity = m_computer.getValidEntity();
-                    {
+                    if( !(entity instanceof EntityPlayer) ) return new Object[] { false, "Cannot find player" };
-                        throw new LuaException( "Cannot find player" );
+                    EntityPlayer player = (EntityPlayer) entity;
-                    }
-
-                    EntityPlayer player = (EntityPlayer) m_computer.getEntity();
                     InventoryPlayer inventory = player.inventory;
-
-                    int computerID = m_computer.getID();
-                    if ( !pocketComputerExists( inventory.mainInventory, computerID ) && !pocketComputerExists( inventory.offHandInventory, computerID ) )
-                    {
-                        throw new LuaException( "Cannot find pocket computer" );
-                    }
-
                     IPocketUpgrade previousUpgrade = m_computer.getUpgrade();
 
                     // Attempt to find the upgrade, starting in the main segment, and then looking in the opposite
@@ -82,7 +72,7 @@ public class PocketAPI implements ILuaAPI
                     {
                         newUpgrade = findUpgrade( inventory.offHandInventory, 0, previousUpgrade );
                     }
-                    if( newUpgrade == null ) throw new LuaException( "Cannot find a valid upgrade" );
+                    if( newUpgrade == null ) return new Object[] { false, "Cannot find a valid upgrade" };
 
                     // Remove the current upgrade
                     if( previousUpgrade != null )
@@ -101,30 +91,20 @@ public class PocketAPI implements ILuaAPI
                     // Set the new upgrade
                     m_computer.setUpgrade( newUpgrade );
 
-                    return null;
+                    return new Object[] { true };
                 } );
 
             case 1:
                 // unequipBack
                 return context.executeMainThreadTask( () ->
                 {
-                    if( !(m_computer.getEntity() instanceof EntityPlayer) )
+                    Entity entity = m_computer.getValidEntity();
-                    {
+                    if( !(entity instanceof EntityPlayer) ) return new Object[] { false, "Cannot find player" };
-                        throw new LuaException( "Cannot find player" );
+                    EntityPlayer player = (EntityPlayer) entity;
-                    }
-
-                    EntityPlayer player = (EntityPlayer) m_computer.getEntity();
                     InventoryPlayer inventory = player.inventory;
-
-                    int computerID = m_computer.getID();
-                    if ( !pocketComputerExists( inventory.mainInventory, computerID ) && !pocketComputerExists( inventory.offHandInventory, computerID ) )
-                    {
-                        throw new LuaException( "Cannot find pocket computer" );
-                    }
-
                     IPocketUpgrade previousUpgrade = m_computer.getUpgrade();
 
-                    if( previousUpgrade == null ) throw new LuaException( "Nothing to unequip" );
+                    if( previousUpgrade == null ) return new Object[] { false, "Nothing to unequip" };
 
                     m_computer.setUpgrade( null );
 
@@ -138,7 +118,7 @@ public class PocketAPI implements ILuaAPI
                         }
                     }
 
-                    return null;
+                    return new Object[] { true };
                 } );
             default:
                 return null;
@@ -168,20 +148,4 @@ public class PocketAPI implements ILuaAPI
 
         return null;
     }
-
-    private static boolean pocketComputerExists( NonNullList<ItemStack> inv, int computerID )
-    {
-        for( ItemStack invStack : inv )
-        {
-            if( !invStack.isEmpty() && invStack.getItem() instanceof ItemPocketComputer )
-            {
-                if( ComputerCraft.Items.pocketComputer.getComputerID( invStack ) == computerID )
-                {
-                    return true;
-                }
-            }
-        }
-
-        return false;
-    }
 }

src/main/java/dan200/computercraft/shared/pocket/core/PocketServerComputer.java

@@ -14,6 +14,7 @@ import dan200.computercraft.shared.computer.core.ComputerFamily;
 import dan200.computercraft.shared.computer.core.ServerComputer;
 import dan200.computercraft.shared.network.NetworkHandler;
 import net.minecraft.entity.Entity;
+import net.minecraft.entity.EntityLivingBase;
 import net.minecraft.entity.player.EntityPlayer;
 import net.minecraft.entity.player.EntityPlayerMP;
 import net.minecraft.entity.player.InventoryPlayer;
@@ -46,6 +47,29 @@ public class PocketServerComputer extends ServerComputer implements IPocketAcces
         return m_entity;
     }
 
+    @Nullable
+    @Override
+    public Entity getValidEntity()
+    {
+        Entity entity = m_entity;
+        if( entity == null || m_stack == null || entity.isDead ) return null;
+
+        if( m_entity instanceof EntityPlayer )
+        {
+            InventoryPlayer inventory = ((EntityPlayer) m_entity).inventory;
+            return inventory.mainInventory.contains( m_stack ) || inventory.offHandInventory.contains( m_stack ) ? entity : null;
+        }
+        else if( m_entity instanceof EntityLivingBase )
+        {
+            EntityLivingBase living = (EntityLivingBase) m_entity;
+            return living.getHeldItemMainhand() == m_stack || living.getHeldItemOffhand() == m_stack ? entity : null;
+        }
+        else
+        {
+            return null;
+        }
+    }
+
     @Override
     public int getColour()
     {

src/main/java/dan200/computercraft/shared/pocket/peripherals/PocketModem.java

@@ -47,7 +47,7 @@ public class PocketModem extends AbstractPocketUpgrade
     {
         if( !(peripheral instanceof PocketModemPeripheral) ) return;
 
-        Entity entity = access.getEntity();
+        Entity entity = access.getValidEntity();
 
         PocketModemPeripheral modem = (PocketModemPeripheral) peripheral;
 

src/main/java/dan200/computercraft/shared/pocket/peripherals/PocketSpeaker.java

@@ -41,7 +41,7 @@ public class PocketSpeaker extends AbstractPocketUpgrade
 
         PocketSpeakerPeripheral speaker = (PocketSpeakerPeripheral) peripheral;
 
-        Entity entity = access.getEntity();
+        Entity entity = access.getValidEntity();
         if( entity instanceof EntityLivingBase )
         {
             EntityLivingBase player = (EntityLivingBase) entity;

src/main/resources/assets/computercraft/lua/rom/programs/pocket/equip.lua

@@ -1,6 +1,6 @@
-local ok, err = pcall( pocket.equipBack )
+local ok, err = pocket.equipBack()
 if not ok then
-    printError( "Nothing to equip" )
+    printError( err )
 else
     print( "Item equipped" )
 end

src/main/resources/assets/computercraft/lua/rom/programs/pocket/unequip.lua

@@ -1,6 +1,6 @@
-local ok, err = pcall( pocket.unequipBack )
+local ok, err = pocket.unequipBack()
 if not ok then
-    printError( "Nothing to unequip" )
+    printError( err )
 else
     print( "Item unequipped" )
 end