menumanager: fix right click/swap issues (#866)

* menumanager: fix possible concurrent modification exception, add small
optimization for left click entries, and cleanup

* menumanager: stop entries from swapping in the right click menu if the
swapFrom is already the first option

* Revert "rsclientmixin: make use of edited menuopened event variables"

This reverts commit d2cd11a7

* menumanager: set event entries in menuopened event to prevent conflicts

* menumanager: remove type change, and improve performance

* menumanager: add field to set priority level to prevent conflicts

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

@@ -27,6 +27,7 @@ package net.runelite.client.menus;
 import joptsimple.internal.Strings;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
+import lombok.Setter;
 import net.runelite.api.MenuEntry;
 import static net.runelite.client.menus.MenuManager.LEVEL_PATTERN;
 import net.runelite.client.util.Text;
@@ -52,6 +53,16 @@ public class ComparableEntry
 	@Getter
 	private boolean strictTarget;
 
+	/**
+	 * If two entries are both suppose to be left click,
+	 * the entry with the higher priority will be selected.
+	 * This only effects left click priority entries.
+	 */
+	@Getter
+	@Setter
+	@EqualsAndHashCode.Exclude
+	private int priority;
+
 	public ComparableEntry(String option, String target)
 	{
 		this(option, target, -1, -1, true, true);
@@ -70,6 +81,7 @@ public class ComparableEntry
 		this.type = type;
 		this.strictOption = strictOption;
 		this.strictTarget = strictTarget;
+		this.priority = 0;
 	}
 
 	// This is only used for type checking, which is why it has everything but target
@@ -80,6 +92,7 @@ public class ComparableEntry
 		this.id = e.getIdentifier();
 		this.type = e.getType();
 		this.strictOption = true;
+		this.priority = 0;
 	}
 
 	boolean matches(MenuEntry entry)

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

@@ -34,14 +34,16 @@ import com.google.common.collect.Multimap;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
@@ -78,7 +80,6 @@ public class MenuManager
 
 	private final Client client;
 	private final EventBus eventBus;
-	private final Prioritizer prioritizer;
 
 	//Maps the indexes that are being used to the menu option.
 	private final Map<Integer, String> playerMenuIndexMap = new HashMap<>();
@@ -86,24 +87,22 @@ public class MenuManager
 	private final Multimap<Integer, WidgetMenuOption> managedMenuOptions = HashMultimap.create();
 	private final Set<String> npcMenuOptions = new HashSet<>();
 
-	private final Set<ComparableEntry> priorityEntries = new HashSet<>();
+	private final HashSet<ComparableEntry> priorityEntries = new HashSet<>();
-	private final Set<MenuEntry> currentPriorityEntries = new HashSet<>();
+	private HashMap<MenuEntry, ComparableEntry> currentPriorityEntries = new HashMap<>();
-	private final Set<ComparableEntry> hiddenEntries = new HashSet<>();
+	private final ConcurrentHashMap<MenuEntry, ComparableEntry> safeCurrentPriorityEntries = new ConcurrentHashMap<>();
-	private final Set<MenuEntry> currentHiddenEntries = new HashSet<>();
+	private final HashSet<ComparableEntry> hiddenEntries = new HashSet<>();
-	private final Map<ComparableEntry, ComparableEntry> swaps = new HashMap<>();
+	private HashSet<MenuEntry> currentHiddenEntries = new HashSet<>();
-	private final Map<ComparableEntry, MenuEntry> currentSwaps = new HashMap<>();
+	private final HashMap<ComparableEntry, ComparableEntry> swaps = new HashMap<>();
 
 	private final LinkedHashSet<MenuEntry> entries = Sets.newLinkedHashSet();
 
 	private MenuEntry leftClickEntry = null;
-	private int leftClickType = -1;
 
 	@Inject
 	private MenuManager(Client client, EventBus eventBus)
 	{
 		this.client = client;
 		this.eventBus = eventBus;
-		this.prioritizer = new Prioritizer();
 	}
 
 	/**
@@ -147,22 +146,11 @@ public class MenuManager
 	public void onMenuOpened(MenuOpened event)
 	{
 		currentPriorityEntries.clear();
-		currentHiddenEntries.clear();
 
 		// Need to reorder the list to normal, then rebuild with swaps
 		MenuEntry[] oldEntries = event.getMenuEntries();
 
-		for (MenuEntry entry : oldEntries)
-		{
-			if (entry == leftClickEntry)
-			{
-				entry.setType(leftClickType);
-				break;
-			}
-		}
-
 		leftClickEntry = null;
-		leftClickType = -1;
 
 		client.sortMenuEntries();
 
@@ -192,7 +180,7 @@ public class MenuManager
 					{
 						shouldDeprioritize = true;
 					}
-					currentPriorityEntries.add(entry);
+					currentPriorityEntries.put(entry, p);
 					newEntries.remove(entry);
 					continue prioritizer;
 				}
@@ -221,8 +209,8 @@ public class MenuManager
 						}
 					}
 
-					// Do not need to swap with itself
+					// Do not need to swap with itself or if the swapFrom is already the first entry
-					if (swapFrom != null && swapFrom != entry)
+					if (swapFrom != null && swapFrom != entry && swapFrom != Iterables.getLast(newEntries))
 					{
 						// Deprioritize entries if the swaps are not in similar type groups
 						if ((swapFrom.getType() >= 1000 && entry.getType() < 1000) || (entry.getType() >= 1000 && swapFrom.getType() < 1000) && !shouldDeprioritize)
@@ -250,12 +238,19 @@ public class MenuManager
 			}
 		}
 
-		if (!priorityEntries.isEmpty())
+		if (!currentPriorityEntries.isEmpty())
 		{
-			newEntries.addAll(currentPriorityEntries);
+			newEntries.addAll(currentPriorityEntries.entrySet().stream()
+				.sorted(Comparator.comparingInt(e -> e.getValue().getPriority()))
+				.map(Map.Entry::getKey)
+				.collect(Collectors.toList()));
 		}
 
-		event.setMenuEntries(newEntries.toArray(new MenuEntry[0]));
+		MenuEntry[] arrayEntries = newEntries.toArray(new MenuEntry[0]);
+
+		// Need to set the event entries to prevent conflicts
+		event.setMenuEntries(arrayEntries);
+		client.setMenuEntries(arrayEntries);
 	}
 
 	@Subscribe
@@ -282,13 +277,10 @@ public class MenuManager
 		}
 	}
 
-
-
 	@Subscribe
 	public void onBeforeRender(BeforeRender event)
 	{
 		leftClickEntry = null;
-		leftClickType = -1;
 
 		if (client.isMenuOpen())
 		{
@@ -296,7 +288,6 @@ public class MenuManager
 		}
 
 		entries.clear();
-
 		entries.addAll(Arrays.asList(client.getMenuEntries()));
 
 		if (entries.size() < 2)
@@ -304,56 +295,35 @@ public class MenuManager
 			return;
 		}
 
-		currentPriorityEntries.clear();
+		if (!hiddenEntries.isEmpty())
-		currentHiddenEntries.clear();
-		currentSwaps.clear();
-
-		prioritizer.prioritize();
-
-		while (prioritizer.isRunning())
 		{
-			// wait
+			currentHiddenEntries.clear();
-		}
+			indexHiddenEntries(entries);
 
-		entries.removeAll(currentHiddenEntries);
+			if (!currentHiddenEntries.isEmpty())
-
-
-		for (MenuEntry entry : currentPriorityEntries)
-		{
-			if (entries.contains(entry))
 			{
-				leftClickEntry = entry;
+				entries.removeAll(currentHiddenEntries);
-				leftClickType = entry.getType();
-				entries.remove(leftClickEntry);
-				leftClickEntry.setType(MenuAction.WIDGET_DEFAULT.getId());
-				entries.add(leftClickEntry);
-				break;
 			}
 		}
 
-
+		if (!priorityEntries.isEmpty())
-		if (leftClickEntry == null)
 		{
-			MenuEntry first = Iterables.getLast(entries);
+			indexPriorityEntries(entries);
-
-			for (ComparableEntry swap : currentSwaps.keySet())
-			{
-				if (swap.matches(first))
-				{
-					leftClickEntry = currentSwaps.get(swap);
-					leftClickType = leftClickEntry.getType();
-					entries.remove(leftClickEntry);
-					leftClickEntry.setType(MenuAction.WIDGET_DEFAULT.getId());
-					entries.add(leftClickEntry);
-					break;
-				}
-			}
 		}
 
-		client.setMenuEntries(entries.toArray(new MenuEntry[0]));
+		if (leftClickEntry == null && !swaps.isEmpty())
+		{
+			indexSwapEntries(entries);
+		}
+
+		if (leftClickEntry != null)
+		{
+			entries.remove(leftClickEntry);
+			entries.add(leftClickEntry);
+			client.setMenuEntries(entries.toArray(new MenuEntry[0]));
+		}
 	}
 
-
 	public void addPlayerMenuItem(String menuText)
 	{
 		Preconditions.checkNotNull(menuText);
@@ -458,9 +428,8 @@ public class MenuManager
 	@Subscribe
 	public void onMenuOptionClicked(MenuOptionClicked event)
 	{
-		if (leftClickEntry != null && leftClickType != -1)
+		if (!client.isMenuOpen() && leftClickEntry != null)
 		{
-			leftClickEntry.setType(leftClickType);
 			event.setMenuEntry(leftClickEntry);
 			leftClickEntry = null;
 		}
@@ -534,7 +503,7 @@ public class MenuManager
 	/**
 	 * Adds to the set of menu entries which when present, will remove all entries except for this one
 	 */
-	public void addPriorityEntry(String option, String target)
+	public ComparableEntry addPriorityEntry(String option, String target)
 	{
 		option = Text.standardize(option);
 		target = Text.standardize(target);
@@ -542,6 +511,8 @@ public class MenuManager
 		ComparableEntry entry = new ComparableEntry(option, target);
 
 		priorityEntries.add(entry);
+
+		return entry;
 	}
 
 	public void removePriorityEntry(String option, String target)
@@ -559,13 +530,15 @@ public class MenuManager
 	 * Adds to the set of menu entries which when present, will remove all entries except for this one
 	 * This method will add one with strict option, but not-strict target (contains for target, equals for option)
 	 */
-	public void addPriorityEntry(String option)
+	public ComparableEntry addPriorityEntry(String option)
 	{
 		option = Text.standardize(option);
 
 		ComparableEntry entry = new ComparableEntry(option, "", false);
 
 		priorityEntries.add(entry);
+
+		return entry;
 	}
 
 	public void removePriorityEntry(String option)
@@ -789,115 +762,57 @@ public class MenuManager
 		hiddenEntries.remove(entry);
 	}
 
-	private class Prioritizer
+	private void indexHiddenEntries(Set<MenuEntry> entries)
 	{
-		private MenuEntry[] entries;
+		currentHiddenEntries = entries.parallelStream().filter(entry ->
-		private AtomicInteger state = new AtomicInteger(0);
-
-		boolean isRunning()
 		{
-			return state.get() != 0;
+			for (ComparableEntry p : hiddenEntries)
-		}
-
-		void prioritize()
-		{
-			if (state.get() != 0)
 			{
-				return;
+				if (p.matches(entry))
-			}
-
-			entries = client.getMenuEntries();
-
-			state.set(3);
-
-			if (!hiddenEntries.isEmpty())
-			{
-				hiddenFinder.run();
-			}
-			else
-			{
-				state.decrementAndGet();
-			}
-
-			if (!priorityEntries.isEmpty())
-			{
-				priorityFinder.run();
-			}
-			else
-			{
-				state.decrementAndGet();
-			}
-
-			if (!swaps.isEmpty())
-			{
-				swapFinder.run();
-			}
-			else
-			{
-				state.decrementAndGet();
-			}
-		}
-
-		private Thread hiddenFinder = new Thread()
-		{
-			@Override
-			public void run()
-			{
-				Arrays.stream(entries).parallel().forEach(entry ->
 				{
-					for (ComparableEntry p : hiddenEntries)
+					return true;
-					{
+				}
-						if (p.matches(entry))
-						{
-							currentHiddenEntries.add(entry);
-							return;
-						}
-					}
-				});
-				state.decrementAndGet();
 			}
-		};
+			return false;
-
+		}).collect(Collectors.toCollection(HashSet::new));
-		private Thread priorityFinder = new Thread()
-		{
-			@Override
-			public void run()
-			{
-				Arrays.stream(entries).parallel().forEach(entry ->
-				{
-					for (ComparableEntry p : priorityEntries)
-					{
-						if (p.matches(entry))
-						{
-							currentPriorityEntries.add(entry);
-							return;
-						}
-					}
-				});
-
-				state.decrementAndGet();
-			}
-		};
-
-		private Thread swapFinder = new Thread()
-		{
-			@Override
-			public void run()
-			{
-				Arrays.stream(entries).parallel().forEach(entry ->
-				{
-					for (Map.Entry<ComparableEntry, ComparableEntry> p : swaps.entrySet())
-					{
-						if (p.getValue().matches(entry))
-						{
-							currentSwaps.put(p.getKey(), entry);
-							return;
-						}
-					}
-				});
-
-				state.decrementAndGet();
-			}
-		};
 	}
-}
+
+	// This could use some optimization
+	private void indexPriorityEntries(Set<MenuEntry> entries)
+	{
+		safeCurrentPriorityEntries.clear();
+		entries.parallelStream().forEach(entry ->
+		{
+			for (ComparableEntry p : priorityEntries)
+			{
+				if (p.matches(entry))
+				{
+					safeCurrentPriorityEntries.put(entry, p);
+					break;
+				}
+			}
+		});
+
+		leftClickEntry = Iterables.getLast(safeCurrentPriorityEntries.entrySet().stream()
+			.sorted(Comparator.comparingInt(e -> e.getValue().getPriority()))
+			.map(Map.Entry::getKey)
+			.collect(Collectors.toList()));
+	}
+
+	private void indexSwapEntries(Set<MenuEntry>  entries)
+	{
+		MenuEntry first = Iterables.getLast(entries);
+
+		leftClickEntry = entries.parallelStream().filter(entry ->
+		{
+			for (Map.Entry<ComparableEntry, ComparableEntry> p : swaps.entrySet())
+			{
+				if (p.getKey().matches(first) && p.getValue().matches(entry))
+				{
+					return true;
+				}
+			}
+			return false;
+		}).findFirst().orElse(null);
+	}
+}

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

@@ -102,12 +102,12 @@ public class ShiftWalkerPlugin extends Plugin
 	{
 		if (this.shiftLoot)
 		{
-			menuManager.addPriorityEntry(TAKE);
+			menuManager.addPriorityEntry(TAKE).setPriority(100);
 		}
 		
 		if (this.shiftWalk)
 		{
-			menuManager.addPriorityEntry(WALK_HERE);
+			menuManager.addPriorityEntry(WALK_HERE).setPriority(90);
 		}	
 	}
 

runelite-mixins/src/main/java/net/runelite/mixins/RSClientMixin.java

@@ -1304,7 +1304,6 @@ public abstract class RSClientMixin implements RSClient
 		final MenuOpened event = new MenuOpened();
 		event.setMenuEntries(getMenuEntries());
 		callbacks.post(event);
-		client.setMenuEntries(event.getMenuEntries());
 	}
 
 	@Inject