plugins: Fix menu concurrency issues (#1444)

* Cleanup comparables a bit

* Make shift walker add entries on client tick

* Make climb up + down add entries on clienttick

* remove try catch

runelite-api/build.gradle

@@ -7,6 +7,7 @@ dependencies {
 
     implementation group: 'com.google.code.findbugs', name: 'jsr305', version: findbugs
     implementation group: 'org.slf4j', name: 'slf4j-api', version: slf4j
+    implementation group: 'org.apache.commons', name: 'commons-text', version: apacheCommonsText
 
     testImplementation group: 'junit', name: 'junit', version: junit
 }

runelite-api/src/main/java/net/runelite/api/MenuEntry.java

@@ -24,19 +24,18 @@
  */
 package net.runelite.api;
 
-import java.util.regex.Matcher;
+import lombok.AccessLevel;
-import java.util.regex.Pattern;
-import lombok.AllArgsConstructor;
 import lombok.Data;
 import lombok.Getter;
-import lombok.RequiredArgsConstructor;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import net.runelite.api.util.Text;
 
 /**
  * A menu entry in a right-click menu.
  */
 @Data
-@AllArgsConstructor
+@NoArgsConstructor
-@RequiredArgsConstructor
 public class MenuEntry
 {
 	/**
@@ -49,6 +48,7 @@ public class MenuEntry
 	 * If the option does not apply to any target, this field
 	 * will be set to empty string.
 	 */
+	@Setter(AccessLevel.NONE)
 	private String target;
 	/**
 	 * An identifier value for the target of the action.
@@ -56,6 +56,7 @@ public class MenuEntry
 	private int identifier;
 	/**
 	 * The action the entry will trigger.
+	 * {@link net.runelite.api.MenuOpcode}
 	 */
 	private int opcode;
 	/**
@@ -74,6 +75,17 @@ public class MenuEntry
 	 */
 	private boolean forceLeftClick;
 
+	public MenuEntry(String option, String target, int identifier, int opcode, int param0, int param1, boolean forceLeftClick)
+	{
+		this.option = option;
+		this.target = target;
+		this.identifier = identifier;
+		this.opcode = opcode;
+		this.param0 = param0;
+		this.param1 = param1;
+		this.forceLeftClick = forceLeftClick;
+	}
+
 	public static MenuEntry copy(MenuEntry src)
 	{
 		return new MenuEntry(
@@ -87,17 +99,22 @@ public class MenuEntry
 		);
 	}
 
-	private static final Matcher TAG_REGEXP = Pattern.compile("<[^>]*>").matcher("");
+	public void setTarget(String target)
-
-	@Getter(lazy = true)
-	private final String standardizedOption = standardize(option);
-	@Getter(lazy = true)
-	private final String standardizedTarget = standardize(LEVEL_MATCHER.reset(target).replaceAll(""));
-
-	public String standardize(String string)
 	{
-		return TAG_REGEXP.reset(string).replaceAll("").replace('\u00A0', ' ').trim().toLowerCase();
+		this.target = target;
+		this.standardizedTarget = null;
 	}
 
-	private static final Matcher LEVEL_MATCHER = Pattern.compile("\\(level-[0-9]*\\)").matcher("");
+	@Getter(AccessLevel.NONE)
+	private String standardizedTarget;
+
+	public String getStandardizedTarget()
+	{
+		if (standardizedTarget == null)
+		{
+			standardizedTarget = Text.standardize(target, true);
+		}
+
+		return standardizedTarget;
+	}
 }

runelite-api/src/main/java/net/runelite/api/util/Text.java

@@ -0,0 +1,108 @@
+package net.runelite.api.util;
+
+import org.apache.commons.lang3.StringUtils;
+
+public class Text
+{
+	private static final StringBuilder SB = new StringBuilder(32);
+	/**
+	 * Removes all tags from the given string.
+	 *
+	 * @param str The string to remove tags from.
+	 * @return The given string with all tags removed from it.
+	 *
+	 * I know this is a monstrosity, but old frankenstein here
+	 * is twice as fast as the old regex method was.
+	 * Seems worth it to me
+	 */
+	public static String removeTags(String str, boolean removeLevel)
+	{
+		int strLen = str.length();
+		if (removeLevel)
+		{
+			if (str.charAt(strLen - 1) == ')')
+			{
+				int levelStart = StringUtils.lastIndexOf(str, '(');
+				// if it's not in the string the while will act like a if
+				while (--levelStart >= 0)
+				{
+					if (str.charAt(levelStart) != ' ')
+					{
+						strLen = levelStart;
+					}
+				}
+			}
+		}
+
+		int open, close;
+		if ((open = StringUtils.indexOf(str, '<')) == -1
+			|| (close = StringUtils.indexOf(str, '>', open)) == -1)
+		{
+			return strLen == str.length() ? str : StringUtils.left(str, strLen);
+		}
+
+		// If the string starts with a < we can maybe take a shortcut if this
+		// is the only tag in the string (take the substring after it)
+		if (open == 0)
+		{
+			if ((open = close + 1) >= strLen)
+			{
+				return "";
+			}
+
+			if ((open = StringUtils.indexOf(str, '<', open)) == -1
+				|| (StringUtils.indexOf(str, '>', open)) == -1)
+			{
+				return StringUtils.substring(str, close + 1);
+			}
+
+			// Whoops, at least we know the last value so we can go back to where we were
+			// before :)
+			open = 0;
+		}
+
+		SB.setLength(0);
+		int i = 0;
+		do
+		{
+			while (open != i)
+			{
+				SB.append(str.charAt(i++));
+			}
+
+			i = close + 1;
+		}
+		while ((open = StringUtils.indexOf(str, '<', close)) != -1
+			&& (close = StringUtils.indexOf(str, '>', open)) != -1
+			&& i < strLen);
+
+		while (i < strLen)
+		{
+			SB.append(str.charAt(i++));
+		}
+
+		return SB.toString();
+	}
+
+	/**
+	 * In addition to removing all tags, replaces nbsp with space, trims string and lowercases it
+	 *
+	 * @param str The string to standardize
+	 * @return The given `str` that is standardized
+	 */
+	public static String standardize(String str, boolean removeLevel)
+	{
+		if (StringUtils.isBlank(str))
+		{
+			return str;
+		}
+
+		return removeTags(str, removeLevel).replace('\u00A0', ' ').trim().toLowerCase();
+	}
+
+	public static String standardize(String str)
+	{
+		return standardize(str, false);
+	}
+
+}

runelite-client/src/main/java/net/runelite/client/menus/BaseComparableEntry.java

@@ -29,6 +29,7 @@ import joptsimple.internal.Strings;
 import lombok.EqualsAndHashCode;
 import net.runelite.api.MenuEntry;
 import net.runelite.client.util.Text;
+import org.apache.commons.lang3.StringUtils;
 
 @EqualsAndHashCode(callSuper = true)
 public class BaseComparableEntry extends AbstractComparableEntry
@@ -51,9 +52,9 @@ public class BaseComparableEntry extends AbstractComparableEntry
 
 	public boolean matches(@Nonnull MenuEntry entry)
 	{
-		String opt = entry.getStandardizedOption();
+		String opt = entry.getOption();
 
-		if (strictOption && !opt.equals(option) || !strictOption && !opt.contains(option))
+		if (strictOption && !StringUtils.equalsIgnoreCase(opt, option) || !strictOption && !StringUtils.containsIgnoreCase(opt, option))
 		{
 			return false;
 		}

runelite-client/src/main/java/net/runelite/client/menus/ItemComparableEntry.java

@@ -7,6 +7,7 @@ import net.runelite.api.Client;
 import net.runelite.api.ItemDefinition;
 import net.runelite.api.MenuEntry;
 import net.runelite.client.util.Text;
+import org.apache.commons.lang3.StringUtils;
 
 @EqualsAndHashCode(callSuper = true)
 @Getter
@@ -28,7 +29,7 @@ abstract class ItemComparableEntry extends AbstractComparableEntry
 			assert client.isClientThread() : "You can only create these on the clientthread";
 
 			this.target = Text.standardize(itemName);
-			this.option = Text.standardize(option);
+			this.option = option;
 
 			short[] tmp = this.itemIds = new short[16];
 
@@ -38,7 +39,7 @@ abstract class ItemComparableEntry extends AbstractComparableEntry
 			for (short i = 0; i < itemCount; i++)
 			{
 				ItemDefinition def = client.getItemDefinition(i);
-				if (def.getNote() != -1 || !def.getName().toLowerCase().contains(target))
+				if (def.getNote() != -1 || !StringUtils.containsIgnoreCase(def.getName(), target))
 				{
 					continue;
 				}
@@ -46,7 +47,7 @@ abstract class ItemComparableEntry extends AbstractComparableEntry
 				boolean notValid = true;
 				for (String opt : def.getInventoryActions())
 				{
-					if (opt != null && Text.standardize(opt).contains(option))
+					if (opt != null && StringUtils.containsIgnoreCase(opt, option))
 					{
 						notValid = false;
 						break;
@@ -75,7 +76,7 @@ abstract class ItemComparableEntry extends AbstractComparableEntry
 
 		public boolean matches(MenuEntry entry)
 		{
-			if (!this.option.contains(Text.standardize(entry.getOption())))
+			if (!StringUtils.containsIgnoreCase(entry.getOption(), this.option))
 			{
 				return false;
 			}

runelite-client/src/main/java/net/runelite/client/menus/MenuManager.java

@@ -34,7 +34,6 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.ConcurrentModificationException;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
@@ -808,49 +807,43 @@ public class MenuManager
 	private void indexPriorityEntries(MenuEntry[] entries, int menuOptionCount)
 	{
 		// create a array of priority entries so we can sort those
-		try
+		final SortMapping[] prios = new SortMapping[entries.length - menuOptionCount];
-		{
-			final SortMapping[] prios = new SortMapping[entries.length - menuOptionCount];
 
-			int prioAmt = 0;
+		int prioAmt = 0;
-			for (int i = 0; i < menuOptionCount; i++)
+		for (int i = 0; i < menuOptionCount; i++)
+		{
+			final MenuEntry entry = entries[i];
+			for (AbstractComparableEntry prio : priorityEntries)
 			{
-				final MenuEntry entry = entries[i];
+				if (!prio.matches(entry))
-				for (AbstractComparableEntry prio : priorityEntries)
 				{
-					if (!prio.matches(entry))
+					continue;
-					{
-						continue;
-					}
-
-					final SortMapping map = new SortMapping(prio.getPriority(), entry);
-					prios[prioAmt++] = map;
-					entries[i] = null;
-					break;
 				}
+
+				final SortMapping map = new SortMapping(prio.getPriority(), entry);
+				prios[prioAmt++] = map;
+				entries[i] = null;
+				break;
 			}
-
-			if (prioAmt == 0)
-			{
-				return;
-			}
-
-			// Sort em!
-			Arrays.sort(prios, 0, prioAmt);
-			int i;
-
-			// Just place them after the standard entries. clientmixin ignores null entries
-			for (i = 0; i < prioAmt; i++)
-			{
-				entries[menuOptionCount + i] = prios[i].entry;
-			}
-
-			firstEntry = entries[menuOptionCount + i - 1];
 		}
-		catch (ConcurrentModificationException ignored)
+
+		if (prioAmt == 0)
 		{
-			//true band aid :)
+			return;
 		}
+
+		// Sort em!
+		Arrays.sort(prios, 0, prioAmt);
+		int i;
+
+		// Just place them after the standard entries. clientmixin ignores null entries
+		for (i = 0; i < prioAmt; i++)
+		{
+			entries[menuOptionCount + i] = prios[i].entry;
+		}
+
+		firstEntry = entries[menuOptionCount + i - 1];
+
 	}
 
 	private void indexSwapEntries(MenuEntry[] entries, int menuOptionCount)

runelite-client/src/main/java/net/runelite/client/plugins/menuentryswapper/BankComparableEntry.java

@@ -2,17 +2,18 @@ package net.runelite.client.plugins.menuentryswapper;
 
 import lombok.EqualsAndHashCode;
 import net.runelite.api.MenuEntry;
+import net.runelite.api.util.Text;
 import net.runelite.api.widgets.WidgetID;
 import net.runelite.api.widgets.WidgetInfo;
 import net.runelite.client.menus.AbstractComparableEntry;
-import net.runelite.client.util.Text;
+import org.apache.commons.lang3.StringUtils;
 
 @EqualsAndHashCode(callSuper = true)
 public class BankComparableEntry extends AbstractComparableEntry
 {
 	public BankComparableEntry(String option, String itemName, boolean strictTarget)
 	{
-		this.setOption(Text.standardize(option));
+		this.setOption(option);
 		this.setTarget(Text.standardize(itemName));
 		this.setStrictTarget(strictTarget);
 	}
@@ -26,11 +27,11 @@ public class BankComparableEntry extends AbstractComparableEntry
 			return false;
 		}
 
-		if (isStrictTarget() && Text.standardize(entry.getTarget()).equals(this.getTarget()))
+		if (isStrictTarget() && !Text.standardize(entry.getTarget()).equals(this.getTarget()))
 		{
 			return false;
 		}
 
-		return Text.standardize(entry.getOption()).contains(this.getOption()) && Text.standardize(entry.getTarget()).contains(this.getTarget());
+		return StringUtils.containsIgnoreCase(entry.getOption(), this.getOption()) && Text.standardize(entry.getTarget()).contains(this.getTarget());
 	}
 }

runelite-client/src/main/java/net/runelite/client/plugins/menuentryswapper/EquipmentComparableEntry.java

@@ -13,7 +13,7 @@ public class EquipmentComparableEntry extends AbstractComparableEntry
 {
 	public EquipmentComparableEntry(String option, String itemName)
 	{
-		this.setOption(Text.standardize(option));
+		this.setOption(option);
 		this.setTarget(Text.standardize(itemName));
 	}
 

runelite-client/src/main/java/net/runelite/client/plugins/menuentryswapper/InventoryComparableEntry.java

@@ -6,13 +6,14 @@ import net.runelite.api.widgets.WidgetID;
 import net.runelite.api.widgets.WidgetInfo;
 import net.runelite.client.menus.AbstractComparableEntry;
 import net.runelite.client.util.Text;
+import org.apache.commons.lang3.StringUtils;
 
 @EqualsAndHashCode(callSuper = true)
 public class InventoryComparableEntry extends AbstractComparableEntry
 {
 	public InventoryComparableEntry(String option, String itemName, boolean strictTarget)
 	{
-		this.setOption(Text.standardize(option));
+		this.setOption(option);
 		this.setTarget(Text.standardize(itemName));
 		this.setStrictTarget(strictTarget);
 	}
@@ -31,6 +32,6 @@ public class InventoryComparableEntry extends AbstractComparableEntry
 			return false;
 		}
 
-		return Text.standardize(entry.getOption()).contains(this.getOption()) && Text.standardize(entry.getTarget()).contains(this.getTarget());
+		return StringUtils.containsIgnoreCase(entry.getOption(), this.getOption()) && Text.standardize(entry.getTarget()).contains(this.getTarget());
 	}
 }

runelite-client/src/main/java/net/runelite/client/plugins/menuentryswapper/MenuEntrySwapperPlugin.java

@@ -58,6 +58,7 @@ import net.runelite.api.Varbits;
 import static net.runelite.api.Varbits.BUILDING_MODE;
 import net.runelite.api.WorldType;
 import net.runelite.api.coords.WorldPoint;
+import net.runelite.api.events.ClientTick;
 import net.runelite.api.events.ConfigChanged;
 import net.runelite.api.events.GameStateChanged;
 import net.runelite.api.events.MenuEntryAdded;
@@ -115,6 +116,8 @@ import org.apache.commons.lang3.ArrayUtils;
 public class MenuEntrySwapperPlugin extends Plugin
 {
 	private static final String CONFIG_GROUP = "shiftclick";
+	private static final String SHIFT = "menuentryswapper shift";
+	private static final String CONTROL = "menuentryswapper control";
 	private static final int PURO_PURO_REGION_ID = 10307;
 	private static final Set<MenuOpcode> NPC_MENU_TYPES = ImmutableSet.of(
 		MenuOpcode.NPC_FIRST_OPTION, MenuOpcode.NPC_SECOND_OPTION, MenuOpcode.NPC_THIRD_OPTION,
@@ -1434,6 +1437,11 @@ public class MenuEntrySwapperPlugin extends Plugin
 	}
 
 	void startShift()
+	{
+		eventBus.subscribe(ClientTick.class, SHIFT, this::addShift);
+	}
+
+	private void addShift(ClientTick event)
 	{
 		loadCustomSwaps(this.configCustomShiftSwaps, customShiftSwaps);
 
@@ -1443,12 +1451,19 @@ public class MenuEntrySwapperPlugin extends Plugin
 		}
 
 		menuManager.addPriorityEntry("climb-up").setPriority(100);
+		eventBus.unregister(SHIFT);
 	}
 
 	void stopShift()
+	{
+		eventBus.subscribe(ClientTick.class, SHIFT, this::remShift);
+	}
+
+	private void remShift(ClientTick event)
 	{
 		menuManager.removePriorityEntry("climb-up");
 		loadCustomSwaps("", customShiftSwaps);
+		eventBus.unregister(SHIFT);
 	}
 
 	void startControl()
@@ -1458,12 +1473,24 @@ public class MenuEntrySwapperPlugin extends Plugin
 			return;
 		}
 
+		eventBus.subscribe(ClientTick.class, CONTROL, this::addControl);
+	}
+
+	private void addControl(ClientTick event)
+	{
 		menuManager.addPriorityEntry("climb-down").setPriority(100);
+		eventBus.unregister(CONTROL);
 	}
 
 	void stopControl()
+	{
+		eventBus.subscribe(ClientTick.class, CONTROL, this::remControl);
+	}
+
+	private void remControl(ClientTick event)
 	{
 		menuManager.removePriorityEntry("climb-down");
+		eventBus.unregister(CONTROL);
 	}
 
 	private void setCastOptions(boolean force)

runelite-client/src/main/java/net/runelite/client/plugins/shiftwalker/ShiftWalkerPlugin.java

@@ -27,11 +27,15 @@ package net.runelite.client.plugins.shiftwalker;
 import com.google.inject.Provides;
 import javax.inject.Inject;
 import javax.inject.Singleton;
+import net.runelite.api.MenuEntry;
+import net.runelite.api.MenuOpcode;
+import net.runelite.api.events.ClientTick;
 import net.runelite.api.events.ConfigChanged;
 import net.runelite.api.events.FocusChanged;
 import net.runelite.client.config.ConfigManager;
 import net.runelite.client.eventbus.EventBus;
 import net.runelite.client.input.KeyManager;
+import net.runelite.client.menus.AbstractComparableEntry;
 import net.runelite.client.menus.MenuManager;
 import net.runelite.client.plugins.Plugin;
 import net.runelite.client.plugins.PluginDescriptor;
@@ -51,8 +55,76 @@ import net.runelite.client.plugins.PluginType;
 public class ShiftWalkerPlugin extends Plugin
 {
 
-	private static final String WALK_HERE = "Walk here";
+	private static final AbstractComparableEntry WALK = new AbstractComparableEntry()
-	private static final String TAKE = "Take";
+	{
+		private final int hash = "WALK".hashCode() * 79 + getPriority();
+
+		@Override
+		public int hashCode()
+		{
+			return hash;
+		}
+
+		@Override
+		public boolean equals(Object entry)
+		{
+			return entry.getClass() == this.getClass() && entry.hashCode() == this.hashCode();
+		}
+
+		@Override
+		public int getPriority()
+		{
+			return 99;
+		}
+
+		@Override
+		public boolean matches(MenuEntry entry)
+		{
+			return
+				entry.getOpcode() == MenuOpcode.WALK.getId() ||
+				entry.getOpcode() == MenuOpcode.WALK.getId() + MenuOpcode.MENU_ACTION_DEPRIORITIZE_OFFSET;
+		}
+	};
+
+	private static final AbstractComparableEntry TAKE = new AbstractComparableEntry()
+	{
+		private final int hash = "TAKE".hashCode() * 79 + getPriority();
+
+		@Override
+		public int hashCode()
+		{
+			return hash;
+		}
+
+		@Override
+		public boolean equals(Object entry)
+		{
+			return entry.getClass() == this.getClass() && entry.hashCode() == this.hashCode();
+		}
+
+		@Override
+		public int getPriority()
+		{
+			return 100;
+		}
+
+		@Override
+		public boolean matches(MenuEntry entry)
+		{
+			int opcode = entry.getOpcode();
+			if (opcode > MenuOpcode.MENU_ACTION_DEPRIORITIZE_OFFSET)
+			{
+				opcode -= MenuOpcode.MENU_ACTION_DEPRIORITIZE_OFFSET;
+			}
+
+			return
+				opcode >= MenuOpcode.GROUND_ITEM_FIRST_OPTION.getId() &&
+				opcode <= MenuOpcode.GROUND_ITEM_FIFTH_OPTION.getId();
+		}
+	};
+
+	private static final String EVENTBUS_THING = "shiftwalker_shift";
+
 	@Inject
 	private ShiftWalkerConfig config;
 
@@ -111,22 +183,34 @@ public class ShiftWalkerPlugin extends Plugin
 	}
 
 	void startPrioritizing()
+	{
+		eventBus.subscribe(ClientTick.class, EVENTBUS_THING, this::addEntries);
+	}
+
+	private void addEntries(ClientTick event)
 	{
 		if (this.shiftLoot)
 		{
-			menuManager.addPriorityEntry(TAKE).setPriority(100);
+			menuManager.addPriorityEntry(TAKE);
 		}
-		
 		if (this.shiftWalk)
 		{
-			menuManager.addPriorityEntry(WALK_HERE).setPriority(90);
+			menuManager.addPriorityEntry(WALK);
-		}	
+		}
+
+		eventBus.unregister(EVENTBUS_THING);
 	}
 
 	void stopPrioritizing()
+	{
+		eventBus.subscribe(ClientTick.class, EVENTBUS_THING, this::remEntries);
+	}
+
+	private void remEntries(ClientTick c)
 	{
 		menuManager.removePriorityEntry(TAKE);
-		menuManager.removePriorityEntry(WALK_HERE);
+		menuManager.removePriorityEntry(WALK);
+		eventBus.unregister(EVENTBUS_THING);
 	}
 
 	private void onConfigChanged(ConfigChanged event)
@@ -136,7 +220,13 @@ public class ShiftWalkerPlugin extends Plugin
 			return;
 		}
 
-		this.shiftWalk = config.shiftWalk();
+		if ("shiftWalk".equals(event.getKey()))
-		this.shiftLoot = config.shiftLoot();
+		{
+			this.shiftWalk = "true".equals(event.getNewValue());
+		}
+		else
+		{
+			this.shiftLoot = "true".equals(event.getNewValue());
+		}
 	}
 }