Clean up data-gathering code

- Refer to this as "data" rather than "metadata". I'm still not sure
   where the meta came from - blame OpenPeripheral I guess.
 - Likewise, use getItemDetail within inventory methods, rather than
   getItemMeta.
 - Refactor common data-getting code into one class. This means that
   turtle.getItemDetail, turtle.inspect and commands.getBlockInfo all
   use the same code.
 - turtle.getItemDetail now accepts a second "detailed" parameter which
   will include the full metadata (#471, #452).
 - Tags are now only included in the detailed list. This is a breaking
   change, however should only affect one version (1.89.x) and I'm not
   convinced that the previous behaviour was safe.

doc/stub/turtle.lua

@@ -206,16 +206,17 @@ function getItemSpace(slot) end
 --- Get detailed information about the items in the given slot.
 --
 -- @tparam[opt] number slot The slot to get information about. Defaults to the @{turtle.select|selected slot}.
+-- @tparam[opt] boolean detailed Whether to include "detailed" information. When @{true} the method will contain
+-- much more information about the item at the cost of taking longer to run.
 -- @treturn nil|table Information about the given slot, or @{nil} if it is empty.
 -- @usage Print the current slot, assuming it contains 13 dirt.
 --
 --     print(textutils.serialize(turtle.getItemDetail()))
 --     -- => {
 --     --    name = "minecraft:dirt",
---     --    damage = 0,
 --     --    count = 13,
 --     -- }
-function getItemDetail(slot) end
+function getItemDetail(slot, detailed) end
 
 function getFuelLevel() end
 

src/main/java/dan200/computercraft/api/turtle/event/TurtleInspectItemEvent.java

@@ -16,8 +16,8 @@
 /**
  * Fired when a turtle gathers data on an item in its inventory.
  *
- * You may prevent items being inspected, or add additional information to the result. Be aware that this is fired on
- * the computer thread, and so any operations on it must be thread safe.
+ * You may prevent items being inspected, or add additional information to the result. Be aware that this may be fired
+ * on the computer thread, and so any operations on it must be thread safe.
  *
  * @see TurtleAction#INSPECT_ITEM
  */
@@ -25,8 +25,15 @@ public class TurtleInspectItemEvent extends TurtleActionEvent
 {
     private final ItemStack stack;
     private final Map<String, Object> data;
+    private final boolean mainThread;
 
+    @Deprecated
     public TurtleInspectItemEvent( @Nonnull ITurtleAccess turtle, @Nonnull ItemStack stack, @Nonnull Map<String, Object> data )
+    {
+        this( turtle, stack, data, false );
+    }
+
+    public TurtleInspectItemEvent( @Nonnull ITurtleAccess turtle, @Nonnull ItemStack stack, @Nonnull Map<String, Object> data, boolean mainThread )
     {
         super( turtle, TurtleAction.INSPECT_ITEM );
 
@@ -34,6 +41,7 @@ public TurtleInspectItemEvent( @Nonnull ITurtleAccess turtle, @Nonnull ItemStack
         Objects.requireNonNull( data, "data cannot be null" );
         this.stack = stack;
         this.data = data;
+        this.mainThread = mainThread;
     }
 
     /**
@@ -58,6 +66,17 @@ public Map<String, Object> getData()
         return data;
     }
 
+    /**
+     * If this event is being fired on the server thread. When true, information which relies on server state may be
+     * exposed.
+     *
+     * @return If this is run on the main thread.
+     */
+    public boolean onMainThread()
+    {
+        return mainThread;
+    }
+
     /**
      * Add new information to the inspection result. Note this will override fields with the same name.
      *

src/main/java/dan200/computercraft/core/asm/TaskCallback.java

@@ -48,9 +48,15 @@ else if( response.length >= 4 && response[3] instanceof String )
         }
     }
 
-    public static Object[] checkUnwrap( MethodResult result )
+    static Object[] checkUnwrap( MethodResult result )
     {
-        if( result.getCallback() != null ) throw new IllegalStateException( "Cannot return MethodResult currently" );
+        if( result.getCallback() != null )
+        {
+            // Due to how tasks are implemented, we can't currently return a MethodResult. This is an
+            // entirely artificial limitation - we can remove it if it ever becomes an issue.
+            throw new IllegalStateException( "Cannot return MethodResult for mainThread task." );
+        }
+
         return result.getResult();
     }
 

src/main/java/dan200/computercraft/shared/computer/apis/CommandAPI.java

@@ -5,24 +5,21 @@
  */
 package dan200.computercraft.shared.computer.apis;
 
-import com.google.common.collect.ImmutableMap;
 import com.mojang.brigadier.tree.CommandNode;
 import com.mojang.brigadier.tree.LiteralCommandNode;
 import dan200.computercraft.ComputerCraft;
 import dan200.computercraft.api.lua.*;
 import dan200.computercraft.shared.computer.blocks.TileCommandComputer;
+import dan200.computercraft.shared.peripheral.generic.data.BlockData;
 import dan200.computercraft.shared.util.NBTUtil;
-import net.minecraft.block.Block;
 import net.minecraft.block.BlockState;
 import net.minecraft.command.CommandSource;
 import net.minecraft.command.Commands;
 import net.minecraft.nbt.CompoundNBT;
 import net.minecraft.server.MinecraftServer;
-import net.minecraft.state.IProperty;
 import net.minecraft.tileentity.TileEntity;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.World;
-import net.minecraftforge.registries.ForgeRegistries;
 
 import java.util.*;
 
@@ -73,18 +70,7 @@ private Object[] doCommand( String command )
     {
         // Get the details of the block
         BlockState state = world.getBlockState( pos );
-        Block block = state.getBlock();
-
-        Map<Object, Object> table = new HashMap<>();
-        table.put( "name", ForgeRegistries.BLOCKS.getKey( block ).toString() );
-
-        Map<Object, Object> stateTable = new HashMap<>();
-        for( ImmutableMap.Entry<IProperty<?>, Comparable<?>> entry : state.getValues().entrySet() )
-        {
-            IProperty<?> property = entry.getKey();
-            stateTable.put( property.getName(), getPropertyValue( property, entry.getValue() ) );
-        }
-        table.put( "state", stateTable );
+        Map<String, Object> table = BlockData.fill( new HashMap<>(), state );
 
         TileEntity tile = world.getTileEntity( pos );
         if( tile != null ) table.put( "nbt", NBTUtil.toLua( tile.write( new CompoundNBT() ) ) );
@@ -92,13 +78,6 @@ private Object[] doCommand( String command )
         return table;
     }
 
-    @SuppressWarnings( { "unchecked", "rawtypes" } )
-    private static Object getPropertyValue( IProperty property, Comparable value )
-    {
-        if( value instanceof String || value instanceof Number || value instanceof Boolean ) return value;
-        return property.getName( value );
-    }
-
     @LuaFunction( mainThread = true )
     public final Object[] exec( String command )
     {

src/main/java/dan200/computercraft/shared/peripheral/generic/data/BlockData.java

@@ -0,0 +1,43 @@
+/*
+ * This file is part of ComputerCraft - http://www.computercraft.info
+ * Copyright Daniel Ratcliffe, 2011-2020. Do not distribute without permission.
+ * Send enquiries to dratcliffe@gmail.com
+ */
+
+package dan200.computercraft.shared.peripheral.generic.data;
+
+import com.google.common.collect.ImmutableMap;
+import net.minecraft.block.BlockState;
+import net.minecraft.state.IProperty;
+
+import javax.annotation.Nonnull;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class BlockData
+{
+    @Nonnull
+    public static <T extends Map<? super String, Object>> T fill( @Nonnull T data, @Nonnull BlockState state )
+    {
+        data.put( "name", Objects.toString( state.getBlock().getRegistryName() ) );
+
+        Map<Object, Object> stateTable = new HashMap<>();
+        for( ImmutableMap.Entry<IProperty<?>, ? extends Comparable<?>> entry : state.getValues().entrySet() )
+        {
+            IProperty<?> property = entry.getKey();
+            stateTable.put( property.getName(), getPropertyValue( property, entry.getValue() ) );
+        }
+        data.put( "state", stateTable );
+        data.put( "tags", DataHelpers.getTags( state.getBlock().getTags() ) );
+
+        return data;
+    }
+
+    @SuppressWarnings( { "unchecked", "rawtypes" } )
+    private static Object getPropertyValue( IProperty property, Comparable value )
+    {
+        if( value instanceof String || value instanceof Number || value instanceof Boolean ) return value;
+        return property.getName( value );
+    }
+}

src/main/java/dan200/computercraft/shared/peripheral/generic/data/DataHelpers.java

@@ -0,0 +1,23 @@
+/*
+ * This file is part of ComputerCraft - http://www.computercraft.info
+ * Copyright Daniel Ratcliffe, 2011-2020. Do not distribute without permission.
+ * Send enquiries to dratcliffe@gmail.com
+ */
+
+package dan200.computercraft.shared.peripheral.generic.data;
+
+import net.minecraft.util.ResourceLocation;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+public class DataHelpers
+{
+    public static Map<String, Boolean> getTags( Collection<ResourceLocation> tags )
+    {
+        Map<String, Boolean> result = new HashMap<>( tags.size() );
+        for( ResourceLocation location : tags ) result.put( location.toString(), true );
+        return result;
+    }
+}

src/main/java/dan200/computercraft/shared/peripheral/generic/data/FluidData.java

@@ -4,7 +4,7 @@
  * Send enquiries to dratcliffe@gmail.com
  */
 
-package dan200.computercraft.shared.peripheral.generic.meta;
+package dan200.computercraft.shared.peripheral.generic.data;
 
 import net.minecraftforge.fluids.FluidStack;
 
@@ -12,13 +12,21 @@
 import java.util.Map;
 import java.util.Objects;
 
-public class FluidMeta
+public class FluidData
 {
     @Nonnull
-    public static <T extends Map<? super String, Object>> T fillBasicMeta( @Nonnull T data, @Nonnull FluidStack stack )
+    public static <T extends Map<? super String, Object>> T fillBasic( @Nonnull T data, @Nonnull FluidStack stack )
     {
         data.put( "name", Objects.toString( stack.getFluid().getRegistryName() ) );
         data.put( "amount", stack.getAmount() );
         return data;
     }
+
+    @Nonnull
+    public static <T extends Map<? super String, Object>> T fill( @Nonnull T data, @Nonnull FluidStack stack )
+    {
+        fillBasic( data, stack );
+        data.put( "tags", DataHelpers.getTags( stack.getFluid().getTags() ) );
+        return data;
+    }
 }

src/main/java/dan200/computercraft/shared/peripheral/generic/data/ItemData.java

@@ -4,7 +4,7 @@
  * Send enquiries to dratcliffe@gmail.com
  */
 
-package dan200.computercraft.shared.peripheral.generic.meta;
+package dan200.computercraft.shared.peripheral.generic.data;
 
 import com.google.gson.JsonParseException;
 import net.minecraft.item.ItemStack;
@@ -20,10 +20,10 @@
 import java.util.Objects;
 import java.util.stream.Collectors;
 
-public class ItemMeta
+public class ItemData
 {
     @Nonnull
-    public static <T extends Map<? super String, Object>> T fillBasicMeta( @Nonnull T data, @Nonnull ItemStack stack )
+    public static <T extends Map<? super String, Object>> T fillBasic( @Nonnull T data, @Nonnull ItemStack stack )
     {
         data.put( "name", Objects.toString( stack.getItem().getRegistryName() ) );
         data.put( "count", stack.getCount() );
@@ -31,14 +31,13 @@ public static <T extends Map<? super String, Object>> T fillBasicMeta( @Nonnull
     }
 
     @Nonnull
-    public static <T extends Map<? super String, Object>> T fillMeta( @Nonnull T data, @Nonnull ItemStack stack )
+    public static <T extends Map<? super String, Object>> T fill( @Nonnull T data, @Nonnull ItemStack stack )
     {
         if( stack.isEmpty() ) return data;
 
-        fillBasicMeta( data, stack );
+        fillBasic( data, stack );
 
         data.put( "displayName", stack.getDisplayName().getString() );
-        data.put( "rawName", stack.getTranslationKey() );
         data.put( "maxCount", stack.getMaxStackSize() );
 
         if( stack.isDamageable() )
@@ -52,6 +51,8 @@ public static <T extends Map<? super String, Object>> T fillMeta( @Nonnull T dat
             data.put( "durability", stack.getItem().getDurabilityForDisplay( stack ) );
         }
 
+        data.put( "tags", DataHelpers.getTags( stack.getItem().getTags() ) );
+
         CompoundNBT tag = stack.getTag();
         if( tag != null && tag.contains( "display", Constants.NBT.TAG_COMPOUND ) )
         {
@@ -60,7 +61,7 @@ public static <T extends Map<? super String, Object>> T fillMeta( @Nonnull T dat
             {
                 ListNBT loreTag = displayTag.getList( "Lore", Constants.NBT.TAG_STRING );
                 data.put( "lore", loreTag.stream()
-                    .map( ItemMeta::parseTextComponent )
+                    .map( ItemData::parseTextComponent )
                     .filter( Objects::nonNull )
                     .map( ITextComponent::getString )
                     .collect( Collectors.toList() ) );

src/main/java/dan200/computercraft/shared/peripheral/generic/methods/FluidMethods.java

@@ -12,7 +12,7 @@
 import dan200.computercraft.api.peripheral.IComputerAccess;
 import dan200.computercraft.api.peripheral.IPeripheral;
 import dan200.computercraft.core.asm.GenericSource;
-import dan200.computercraft.shared.peripheral.generic.meta.FluidMeta;
+import dan200.computercraft.shared.peripheral.generic.data.FluidData;
 import net.minecraft.fluid.Fluid;
 import net.minecraft.util.ResourceLocation;
 import net.minecraftforge.common.capabilities.ICapabilityProvider;
@@ -49,7 +49,7 @@ public ResourceLocation id()
         for( int i = 0; i < size; i++ )
         {
             FluidStack stack = fluids.getFluidInTank( i );
-            if( !stack.isEmpty() ) result.put( i + 1, FluidMeta.fillBasicMeta( new HashMap<>( 4 ), stack ) );
+            if( !stack.isEmpty() ) result.put( i + 1, FluidData.fillBasic( new HashMap<>( 4 ), stack ) );
         }
 
         return result;

src/main/java/dan200/computercraft/shared/peripheral/generic/methods/InventoryMethods.java

@@ -12,7 +12,7 @@
 import dan200.computercraft.api.peripheral.IComputerAccess;
 import dan200.computercraft.api.peripheral.IPeripheral;
 import dan200.computercraft.core.asm.GenericSource;
-import dan200.computercraft.shared.peripheral.generic.meta.ItemMeta;
+import dan200.computercraft.shared.peripheral.generic.data.ItemData;
 import net.minecraft.inventory.IInventory;
 import net.minecraft.item.ItemStack;
 import net.minecraft.util.ResourceLocation;
@@ -56,19 +56,19 @@ public static int size( IItemHandler inventory )
         for( int i = 0; i < size; i++ )
         {
             ItemStack stack = inventory.getStackInSlot( i );
-            if( !stack.isEmpty() ) result.put( i + 1, ItemMeta.fillBasicMeta( new HashMap<>( 4 ), stack ) );
+            if( !stack.isEmpty() ) result.put( i + 1, ItemData.fillBasic( new HashMap<>( 4 ), stack ) );
         }
 
         return result;
     }
 
     @LuaFunction( mainThread = true )
-    public static Map<String, ?> getItemMeta( IItemHandler inventory, int slot ) throws LuaException
+    public static Map<String, ?> getItemDetail( IItemHandler inventory, int slot ) throws LuaException
     {
         assertBetween( slot, 1, inventory.getSlots(), "Slot out of range (%s)" );
 
         ItemStack stack = inventory.getStackInSlot( slot - 1 );
-        return stack.isEmpty() ? null : ItemMeta.fillMeta( new HashMap<>(), stack );
+        return stack.isEmpty() ? null : ItemData.fill( new HashMap<>(), stack );
     }
 
     @LuaFunction( mainThread = true )

src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java

@@ -13,13 +13,12 @@
 import dan200.computercraft.api.turtle.event.TurtleActionEvent;
 import dan200.computercraft.api.turtle.event.TurtleInspectItemEvent;
 import dan200.computercraft.core.apis.IAPIEnvironment;
+import dan200.computercraft.core.asm.TaskCallback;
 import dan200.computercraft.core.tracking.TrackingField;
+import dan200.computercraft.shared.peripheral.generic.data.ItemData;
 import dan200.computercraft.shared.turtle.core.*;
-import net.minecraft.item.Item;
 import net.minecraft.item.ItemStack;
-import net.minecraft.util.ResourceLocation;
 import net.minecraftforge.common.MinecraftForge;
-import net.minecraftforge.registries.ForgeRegistries;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -309,28 +308,26 @@ public final MethodResult inspectDown()
     }
 
     @LuaFunction
-    public final Object[] getItemDetail( Optional<Integer> slotArg ) throws LuaException
+    public final MethodResult getItemDetail( ILuaContext context, Optional<Integer> slotArg, Optional<Boolean> detailedArg ) throws LuaException
     {
-        // FIXME: There's a race condition here if the stack is being modified (mutating NBT, etc...)
-        //  on another thread. The obvious solution is to move this into a command, but some programs rely
-        //  on this having a 0-tick delay.
         int slot = checkSlot( slotArg ).orElse( turtle.getSelectedSlot() );
+        boolean detailed = detailedArg.orElse( false );
+
+        return detailed
+            ? TaskCallback.make( context, () -> getItemDetail( slot, true ) )
+            : MethodResult.of( getItemDetail( slot, false ) );
+    }
+
+    private Object[] getItemDetail( int slot, boolean detailed )
+    {
         ItemStack stack = turtle.getInventory().getStackInSlot( slot );
         if( stack.isEmpty() ) return new Object[] { null };
 
-        Item item = stack.getItem();
-        String name = ForgeRegistries.ITEMS.getKey( item ).toString();
-        int count = stack.getCount();
+        Map<String, Object> table = detailed
+            ? ItemData.fill( new HashMap<>(), stack )
+            : ItemData.fillBasic( new HashMap<>(), stack );
 
-        Map<String, Object> table = new HashMap<>();
-        table.put( "name", name );
-        table.put( "count", count );
-
-        Map<String, Boolean> tags = new HashMap<>();
-        for( ResourceLocation location : item.getTags() ) tags.put( location.toString(), true );
-        table.put( "tags", tags );
-
-        TurtleActionEvent event = new TurtleInspectItemEvent( turtle, stack, table );
+        TurtleActionEvent event = new TurtleInspectItemEvent( turtle, stack, table, detailed );
         if( MinecraftForge.EVENT_BUS.post( event ) ) return new Object[] { false, event.getFailureMessage() };
 
         return new Object[] { table };

src/main/java/dan200/computercraft/shared/turtle/core/TurtleInspectCommand.java

@@ -5,20 +5,16 @@
  */
 package dan200.computercraft.shared.turtle.core;
 
-import com.google.common.collect.ImmutableMap;
 import dan200.computercraft.api.turtle.ITurtleAccess;
 import dan200.computercraft.api.turtle.ITurtleCommand;
 import dan200.computercraft.api.turtle.TurtleCommandResult;
 import dan200.computercraft.api.turtle.event.TurtleBlockEvent;
-import net.minecraft.block.Block;
+import dan200.computercraft.shared.peripheral.generic.data.BlockData;
 import net.minecraft.block.BlockState;
-import net.minecraft.state.IProperty;
 import net.minecraft.util.Direction;
-import net.minecraft.util.ResourceLocation;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.world.World;
 import net.minecraftforge.common.MinecraftForge;
-import net.minecraftforge.registries.ForgeRegistries;
 
 import javax.annotation.Nonnull;
 import java.util.HashMap;
@@ -51,23 +47,7 @@ public TurtleCommandResult execute( @Nonnull ITurtleAccess turtle )
             return TurtleCommandResult.failure( "No block to inspect" );
         }
 
-        Block block = state.getBlock();
-        String name = ForgeRegistries.BLOCKS.getKey( block ).toString();
-
-        Map<String, Object> table = new HashMap<>();
-        table.put( "name", name );
-
-        Map<Object, Object> stateTable = new HashMap<>();
-        for( ImmutableMap.Entry<IProperty<?>, ? extends Comparable<?>> entry : state.getValues().entrySet() )
-        {
-            IProperty<?> property = entry.getKey();
-            stateTable.put( property.getName(), getPropertyValue( property, entry.getValue() ) );
-        }
-        table.put( "state", stateTable );
-
-        Map<String, Boolean> tags = new HashMap<>();
-        for( ResourceLocation location : block.getTags() ) tags.put( location.toString(), true );
-        table.put( "tags", tags );
+        Map<String, Object> table = BlockData.fill( new HashMap<>(), state );
 
         // Fire the event, exiting if it is cancelled
         TurtlePlayer turtlePlayer = TurtlePlaceCommand.createPlayer( turtle, oldPosition, direction );
@@ -77,11 +57,4 @@ public TurtleCommandResult execute( @Nonnull ITurtleAccess turtle )
         return TurtleCommandResult.success( new Object[] { table } );
 
     }
-
-    @SuppressWarnings( { "unchecked", "rawtypes" } )
-    private static Object getPropertyValue( IProperty property, Comparable value )
-    {
-        if( value instanceof String || value instanceof Number || value instanceof Boolean ) return value;
-        return property.getName( value );
-    }
 }