Menumanager: fixes (#1261)

* menumanager: 'remove' hidden entries on added

* menumanager: keep original index in mind when prio1 == prio2

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

@@ -572,6 +572,12 @@ public interface Client extends GameShell
 	 */
 	void setMenuEntries(MenuEntry[] entries);
 
+	/**
+	 * Set the amount of menu entries the client has.
+	 * If you decrement this count, it's the same as removing the last one
+	 */
+	void setMenuOptionCount(int count);
+
 	/**
 	 * Checks whether a right-click menu is currently open.
 	 *

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

@@ -24,7 +24,8 @@
  */
 package net.runelite.client.menus;
 
-import joptsimple.internal.Strings;
+import com.google.common.base.Strings;
+import javax.annotation.Nonnull;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.Setter;
@@ -95,7 +96,7 @@ public class ComparableEntry
 		this.priority = 0;
 	}
 
-	boolean matches(MenuEntry entry)
+	boolean matches(@Nonnull MenuEntry entry)
 	{
 		String opt = Text.standardize(entry.getOption());
 

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

@@ -30,25 +30,22 @@ import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
-
-import java.util.ArrayList;
 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.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
+import javax.annotation.Nonnull;
 import javax.inject.Inject;
 import javax.inject.Singleton;
-
+import lombok.AllArgsConstructor;
-import com.google.common.collect.Sets;
 import lombok.extern.slf4j.Slf4j;
 import net.runelite.api.Client;
 import net.runelite.api.MenuAction;
@@ -87,16 +84,11 @@ public class MenuManager
 	//Used to manage custom non-player menu options
 	private final Multimap<Integer, WidgetMenuOption> managedMenuOptions = HashMultimap.create();
 	private final Set<String> npcMenuOptions = new HashSet<>();
-
 	private final HashSet<ComparableEntry> priorityEntries = new HashSet<>();
-	private HashMap<MenuEntry, ComparableEntry> currentPriorityEntries = new HashMap<>();
+	private LinkedHashMap<MenuEntry, ComparableEntry> currentPriorityEntries = new LinkedHashMap<>();
-	private final ConcurrentHashMap<MenuEntry, ComparableEntry> safeCurrentPriorityEntries = new ConcurrentHashMap<>();
 	private final HashSet<ComparableEntry> hiddenEntries = new HashSet<>();
-	private HashSet<MenuEntry> currentHiddenEntries = new HashSet<>();
 	private final HashMap<ComparableEntry, ComparableEntry> swaps = new HashMap<>();
 
-	private final LinkedHashSet<MenuEntry> entries = Sets.newLinkedHashSet();
-
 	private MenuEntry leftClickEntry = null;
 	private MenuEntry firstEntry = null;
 
@@ -162,24 +154,13 @@ public class MenuManager
 
 		firstEntry = null;
 
-		client.sortMenuEntries();
-
 		List<MenuEntry> newEntries = Lists.newArrayList(oldEntries);
 
 		boolean shouldDeprioritize = false;
 
-		prioritizer: for (MenuEntry entry : oldEntries)
+		prioritizer:
+		for (MenuEntry entry : oldEntries)
 		{
-			// Remove hidden entries from menus
-			for (ComparableEntry p : hiddenEntries)
-			{
-				if (p.matches(entry))
-				{
-					newEntries.remove(entry);
-					continue prioritizer;
-				}
-			}
-
 			for (ComparableEntry p : priorityEntries)
 			{
 				// Create list of priority entries, and remove from menus
@@ -265,6 +246,15 @@ public class MenuManager
 
 	private void onMenuEntryAdded(MenuEntryAdded event)
 	{
+		for (ComparableEntry e : hiddenEntries)
+		{
+			if (e.matches(event.getMenuEntry()))
+			{
+				client.setMenuOptionCount(client.getMenuOptionCount() - 1);
+				return;
+			}
+		}
+
 		int widgetId = event.getActionParam1();
 		Collection<WidgetMenuOption> options = managedMenuOptions.get(widgetId);
 		MenuEntry[] menuEntries = client.getMenuEntries();
@@ -298,47 +288,36 @@ public class MenuManager
 			return null;
 		}
 
-		firstEntry = null;
+		int menuOptionCount = client.getMenuOptionCount();
-		entries.clear();
+		if (menuOptionCount <= 2)
-		entries.addAll(Arrays.asList(client.getMenuEntries()));
-
-		if (entries.size() < 2)
 		{
 			return null;
 		}
 
-		if (!hiddenEntries.isEmpty())
+		client.sortMenuEntries();
-		{
-			currentHiddenEntries.clear();
-			indexHiddenEntries(entries);
 
-			if (!currentHiddenEntries.isEmpty())
+		firstEntry = null;
-			{
+		MenuEntry[] entries = new MenuEntry[menuOptionCount + priorityEntries.size()];
-				entries.removeAll(currentHiddenEntries);
+		System.arraycopy(client.getMenuEntries(), 0, entries, 0, menuOptionCount);
-			}
-		}
 
 		if (!priorityEntries.isEmpty())
 		{
-			indexPriorityEntries(entries);
+			indexPriorityEntries(entries, menuOptionCount);
 		}
 
 		if (firstEntry == null && !swaps.isEmpty())
 		{
-			indexSwapEntries(entries);
+			indexSwapEntries(entries, menuOptionCount);
 		}
 
-		if (firstEntry != null)
+
+		if (firstEntry == null)
 		{
-			entries.remove(firstEntry);
+			// stop being null smh
-			entries.add(firstEntry);
+			firstEntry = entries[menuOptionCount - 1];
-		}
-		else if (!currentHiddenEntries.isEmpty())
-		{
-			firstEntry = Iterables.getLast(entries, null);
 		}
 
-		client.setMenuEntries(entries.toArray(new MenuEntry[0]));
+		client.setMenuEntries(entries);
 
 		return firstEntry;
 	}
@@ -793,48 +772,59 @@ public class MenuManager
 		hiddenEntries.remove(entry);
 	}
 
-	private void indexHiddenEntries(Set<MenuEntry> entries)
-	{
-		currentHiddenEntries = entries.parallelStream().filter(entry ->
-		{
-			for (ComparableEntry p : hiddenEntries)
-			{
-				if (p.matches(entry))
-				{
-					return true;
-				}
-			}
-			return false;
-		}).collect(Collectors.toCollection(HashSet::new));
-	}
-
 	// This could use some optimization
-	private void indexPriorityEntries(Set<MenuEntry> entries)
+	private void indexPriorityEntries(MenuEntry[] entries, int menuOptionCount)
 	{
-		safeCurrentPriorityEntries.clear();
+		// create a array of priority entries so we can sort those
-		entries.parallelStream().forEach(entry ->
+		SortMapping[] prios = new SortMapping[entries.length - menuOptionCount];
-		{
-			for (ComparableEntry p : priorityEntries)
-			{
-				if (p.matches(entry))
-				{
-					safeCurrentPriorityEntries.put(entry, p);
-					break;
-				}
-			}
-		});
 
-		firstEntry = Iterables.getLast(safeCurrentPriorityEntries.entrySet().stream()
+		int prioAmt = 0;
-			.sorted(Comparator.comparingInt(e -> e.getValue().getPriority()))
+		for (int i = 0; i < menuOptionCount; i++)
-			.map(Map.Entry::getKey)
+		{
-			.collect(Collectors.toList()), null);
+			final MenuEntry entry = entries[i];
+			for (ComparableEntry prio : priorityEntries)
+			{
+				if (!prio.matches(entry))
+				{
+					continue;
+				}
+
+				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];
 	}
 
-	private void indexSwapEntries(Set<MenuEntry> entries)
+	private void indexSwapEntries(MenuEntry[] entries, int menuOptionCount)
 	{
-		MenuEntry first = Iterables.getLast(entries);
+		// firstEntry was null, so it's the entry at count - 1
+		final MenuEntry first = entries[menuOptionCount - 1];
+		if (first == null)
+		{
+			log.debug("First entry is null");
+			return;
+		}
 
-		List<ComparableEntry> values = new ArrayList<>();
+		Set<ComparableEntry> values = new HashSet<>();
 
 		for (Map.Entry<ComparableEntry, ComparableEntry> pair : swaps.entrySet())
 		{
@@ -844,16 +834,42 @@ public class MenuManager
 			}
 		}
 
-		firstEntry = entries.parallelStream().filter(entry ->
+		if (values.isEmpty())
 		{
-			for (ComparableEntry value : values)
+			return;
+		}
+
+		// Backwards so we swap with the otherwise highest one
+		// Count - 2 so we don't compare the entry against itself
+		outer:
+		for (int i = menuOptionCount - 2; i > 0; i--)
+		{
+			final MenuEntry entry = entries[i];
+			for (ComparableEntry swap : values)
 			{
-				if (value.matches(entry))
+				if (!swap.matches(entry))
 				{
-					return true;
+					continue;
 				}
+
+				entries[i] = first;
+				entries[menuOptionCount - 1] = entry;
+				firstEntry = entry;
+				break outer;
 			}
-			return false;
+		}
-		}).findFirst().orElse(null);
+	}
+
+	@AllArgsConstructor
+	private class SortMapping implements Comparable<SortMapping>
+	{
+		private final int priority;
+		private final MenuEntry entry;
+
+		@Override
+		public int compareTo(@Nonnull SortMapping mapping)
+		{
+			return Integer.compare(this.priority, mapping.priority);
+		}
 	}
 }

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

@@ -682,6 +682,11 @@ public abstract class RSClientMixin implements RSClient
 
 		for (MenuEntry entry : entries)
 		{
+			if (entry == null)
+			{
+				continue;
+			}
+
 			menuOptions[count] = entry.getOption();
 			menuTargets[count] = entry.getTarget();
 			menuIdentifiers[count] = entry.getIdentifier();
@@ -1385,8 +1390,9 @@ public abstract class RSClientMixin implements RSClient
 
 	@Inject
 	@MethodHook("openMenu")
-	public void menuOpened(int var1, int var2)
+	public void menuOpened(int x, int y)
 	{
+		sortMenuEntries();
 		final MenuOpened event = new MenuOpened();
 		event.setMenuEntries(getMenuEntries());
 		callbacks.post(MenuOpened.class, event);

runescape-api/src/main/java/net/runelite/rs/api/RSClient.java

@@ -302,6 +302,7 @@ public interface RSClient extends RSGameShell, Client
 	int getMenuOptionCount();
 
 	@Import("menuOptionsCount")
+	@Override
 	void setMenuOptionCount(int menuOptionCount);
 
 	@Import("menuActions")