test: ensure quest steps have sidebar step (#1909)

* test: ensure quest steps have sidebar steps

* Cold War: fix step adding itself as a substep

* Misthalin Mystery: Fix 2 unreachable steps

* The Restless Ghost: Add step to sidepanel

* remove misthalin mystery as a "must pass" test

* feat: Add boolean containsSteps to QuestStep

This ensures we have a single location for checking steps contained within a QuestStep, which should make it easier to ensure we're doing the checks correctly.

* fix: Fix incorrect order of usage of steps in The Feud

---------

Co-authored-by: Zoinkwiz

src/main/java/com/questhelper/QuestHelperPlugin.java

@@ -131,6 +131,7 @@ public class QuestHelperPlugin extends Plugin
 	private QuestBankManager questBankManager;
 
 	@Inject
+	@Getter
 	private QuestManager questManager;
 
 	@Inject

src/main/java/com/questhelper/helpers/quests/coldwar/ColdWar.java

@@ -410,7 +410,7 @@ public void setupSteps()
 		killIcelords = new NpcStep(this, NpcID.ICELORD, new WorldPoint(2647, 10425, 0), "Kill icelords until you are able to leave through the door to the west. May take up to 3 kills.", true);
 		((NpcStep) killIcelords).addAlternateNpcs(NpcID.ICELORD_853, NpcID.ICELORD_854, NpcID.ICELORD_855);
 		exitIcelordPen = new ObjectStep(this, ObjectID.DOOR_21167, new WorldPoint(2639, 10424, 0), "Leave through the door to the west.");
-		killIcelords.addSubSteps(killIcelords);
+		killIcelords.addSubSteps(exitIcelordPen);
 
 		useChasm = new ObjectStep(this, ObjectID.CHASM, new WorldPoint(2657, 10423, 0), "Use the chasm to exit the cave.");
 

src/main/java/com/questhelper/helpers/quests/misthalinmystery/MisthalinMystery.java

@@ -400,6 +400,7 @@ public void setupSteps()
 		playA = new WidgetStep(this, "Play the A key.", 554, 25);
 		playDAgain = new WidgetStep(this, "Play the D key again.", 554, 21);
 		restartPiano = new DetailedQuestStep(this, "Unfortunately you've played an incorrect key. Restart.");
+		playPiano.addSubSteps(restartPiano);
 
 		searchThePiano = new ObjectStep(this, NullObjectID.NULL_29658, new WorldPoint(1647, 4842, 0), "Right-click search the piano for the emerald key.");
 
@@ -418,6 +419,7 @@ public void setupSteps()
 		searchFireplace = new ObjectStep(this, NullObjectID.NULL_29659, new WorldPoint(1647, 4836, 0), "Search the fireplace.");
 
 		restartGems = new DetailedQuestStep(this, "You've clicked a gem in the wrong order. Try restarting.");
+		searchFireplace.addSubSteps(restartGems);
 
 		clickSapphire = new WidgetStep(this, "Click the sapphire.", 555, 19);
 		clickDiamond = new WidgetStep(this, "Click the diamond.", 555, 4);

src/main/java/com/questhelper/helpers/quests/thefeud/TheFeud.java

@@ -447,13 +447,13 @@ public void setupSteps()
 		//Tell Ali The Operator Poisoned
 		tellAliOperatorPoisoned = new NpcStep(this, NpcID.ALI_THE_OPERATOR, new WorldPoint(3332, 2948, 0), "Talk to Ali the Operator and tell him he is dead.");
 
+		killMenaphiteThug = new DetailedQuestStep(this, "Kill the Menaphite Thug. You can safespot him inside the tent by using a chair, if he's not spawned then talk to the Menaphite Leader again.");
+
 		//Step 24
 		//Kill Thug
 		talkToMenaphiteLeader = new NpcStep(this, NpcID.MENAPHITE_LEADER, "Talk to the Menaphite Leader and prepare for a fight against a tough guy. You can safespot him inside the tent by using a chair.");
 		talkToMenaphiteLeader.addSubSteps(killMenaphiteThug);
 
-		killMenaphiteThug = new DetailedQuestStep(this, "Kill the Menaphite Thug. You can safespot him inside the tent by using a chair, if he's not spawned then talk to the Menaphite Leader again.");
-
 		//Step 25
 		//Kill Champion - Talk to Villager
 		talkToAVillager = new NpcStep(this, NpcID.VILLAGER, "Talk to a villager and they will be angry you broke the balance of power.", true);

src/main/java/com/questhelper/helpers/quests/therestlessghost/TheRestlessGhost.java

@@ -192,7 +192,7 @@ public List<PanelDetails> getPanels()
 
 		allSteps.add(new PanelDetails("Talk to Father Aereck", Collections.singletonList(talkToAereck)));
 		allSteps.add(new PanelDetails("Get a ghostspeak amulet", Collections.singletonList(talkToUrhney)));
-		allSteps.add(new PanelDetails("Talk to the ghost", Arrays.asList(openCoffin, speakToGhost)));
+		allSteps.add(new PanelDetails("Talk to the ghost", Arrays.asList(openCoffin, searchCoffin, speakToGhost)));
 		allSteps.add(new PanelDetails("Return the ghost's skull", Arrays.asList(enterWizardsTowerBasement, searchAltarAndRun, exitWizardsTowerBasement, openCoffinToPutSkullIn, putSkullInCoffin)));
 		return allSteps;
 	}

src/main/java/com/questhelper/managers/QuestManager.java

@@ -139,7 +139,7 @@ private void handleSelectedQuest()
 			if (selectedQuest.getCurrentStep() != null)
 			{
 				panel.updateStepsTexts();
-				QuestStep currentStep = selectedQuest.getCurrentStep().getSidePanelStep();
+				QuestStep currentStep = selectedQuest.getCurrentStep().getActiveStep();
 				if (currentStep != null && currentStep != lastStep && panel.questActive)
 				{
 					lastStep = currentStep;

src/main/java/com/questhelper/panel/PanelDetails.java

@@ -29,11 +29,9 @@
 import com.questhelper.requirements.conditional.Conditions;
 import com.questhelper.requirements.util.LogicType;
 import com.questhelper.steps.QuestStep;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Collection;
-import java.util.Objects;
+
+import java.util.*;
+
 import lombok.Getter;
 import lombok.Setter;
 
@@ -131,10 +129,6 @@ public boolean contains(QuestStep currentStep)
 
 	private boolean containsSubStep(QuestStep currentStep, QuestStep check)
 	{
-		if (currentStep.getSubsteps().contains(check) || currentStep == check)
-		{
-			return true;
-		}
-		return currentStep.getSubsteps().stream().anyMatch(step -> containsSubStep(step, check));
+		return currentStep.containsSteps(check, new HashSet<>());
 	}
 }

src/main/java/com/questhelper/panel/QuestOverviewPanel.java

@@ -261,7 +261,7 @@ public void addQuest(QuestHelper quest, boolean isActive)
 		QuestStep currentStep;
 		if (isActive)
 		{
-			currentStep = quest.getCurrentStep().getSidePanelStep();
+			currentStep = quest.getCurrentStep().getActiveStep();
 		}
 		else
 		{
@@ -387,7 +387,7 @@ void updateCollapseText()
 		collapseBtn.setSelected(isAllCollapsed());
 	}
 
-	private boolean isAllCollapsed()
+	public boolean isAllCollapsed()
 	{
 		return questStepPanelList.stream()
 			.filter(QuestStepPanel::isCollapsed)

src/main/java/com/questhelper/panel/QuestStepPanel.java

@@ -35,9 +35,10 @@
 import java.util.ArrayList;
 import java.util.HashMap;
 import com.questhelper.steps.QuestStep;
+
+import java.util.HashSet;
 import java.util.List;
 import net.runelite.api.Client;
-import net.runelite.api.Item;
 import net.runelite.client.ui.ColorScheme;
 import javax.annotation.Nullable;
 import javax.swing.*;
@@ -223,7 +224,7 @@ public void updateHighlightCheck(Client client, QuestStep newStep, QuestHelper c
 			for (QuestStep step : getSteps())
 			{
 				if (step.getConditionToHide() != null && step.getConditionToHide().check(client)) continue;
-				if (step == newStep || step.getSubsteps().contains(newStep))
+				if (step.containsSteps(newStep, new HashSet<>()))
 				{
 					highlighted = true;
 					updateHighlight(step);
@@ -401,6 +402,6 @@ public void updateStepVisibility(Client client)
 
 	private QuestStep currentlyActiveQuestSidebarStep()
 	{
-		return questHelperPlugin.getSelectedQuest().getCurrentStep().getSidePanelStep();
+		return questHelperPlugin.getSelectedQuest().getCurrentStep().getActiveStep();
 	}
 }

src/main/java/com/questhelper/steps/ConditionalStep.java

@@ -31,12 +31,7 @@
 import com.questhelper.requirements.runelite.RuneliteRequirement;
 import com.questhelper.requirements.conditional.InitializableRequirement;
 import java.awt.Graphics2D;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Objects;
+import java.util.*;
 import java.util.function.Consumer;
 import lombok.NonNull;
 import lombok.Setter;
@@ -417,18 +412,19 @@ public QuestStep getActiveStep()
 	}
 
 	@Override
-	public QuestStep getSidePanelStep()
+	public boolean containsSteps(QuestStep questStep, Set<QuestStep> checkedSteps)
 	{
-		if (text != null)
-		{
-			return this;
-		}
-		else if (currentStep != null)
+		if (super.containsSteps(questStep, checkedSteps)) return true;
+
+		Set<QuestStep> stepSet = new HashSet<>(steps.values());
+		stepSet.removeAll(checkedSteps);
+
+		for (QuestStep child : stepSet)
 		{
-			return currentStep.getSidePanelStep();
+			if (child.containsSteps(questStep, checkedSteps)) return true;
 		}
 
-		return this;
+		return false;
 	}
 
 	public Collection<Requirement> getConditions()

src/main/java/com/questhelper/steps/QuestStep.java

@@ -47,16 +47,13 @@
 import com.questhelper.steps.overlay.IconOverlay;
 import java.awt.Graphics2D;
 import java.awt.image.BufferedImage;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.List;
+import java.util.*;
 import java.util.regex.Pattern;
 
 import lombok.Getter;
 import lombok.NonNull;
 import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
 import net.runelite.api.ChatMessageType;
 import net.runelite.api.Client;
 import net.runelite.api.SpriteID;
@@ -73,6 +70,7 @@
 import net.runelite.client.ui.overlay.outline.ModelOutlineRenderer;
 import net.runelite.client.ui.overlay.tooltip.TooltipManager;
 
+@Slf4j
 public abstract class QuestStep implements Module
 {
 	@Inject
@@ -522,9 +520,19 @@ public QuestStep getActiveStep()
 		return this;
 	}
 
-	public QuestStep getSidePanelStep()
+	public boolean containsSteps(QuestStep questStep, Set<QuestStep> checkedSteps)
 	{
-		return getActiveStep();
+		if (checkedSteps.contains(this)) return false;
+		checkedSteps.add(this);
+		return this == questStep || this.getSubsteps().stream().anyMatch((subStep) ->
+		{
+			if (subStep == null)
+			{
+				log.warn("Substep null for " + getText());
+				return false;
+			}
+			return subStep.containsSteps(questStep, checkedSteps);
+		});
 	}
 
 	protected void setupIcon()