Fixed a bug where the fishing widget would not update (#1294)

* Fixed a bug where the fishing widget would not update, I really don't know how no one found this?

* Fixed according to Vic's instructions

* Fixed indentation

* Remove update call

* update positions if shouldRender of all widgets changed
also remove update from updatePositions because it can cause confusion when implementing features

* Refactor widget lists check

* Refactor update widgets and add more docs

---------

Co-authored-by: Antonio Navarro Esteban <teyxoesp@gmail.com>
Co-authored-by: Kevin <92656833+kevinthegreat1@users.noreply.github.com>
Co-authored-by: viciscat <51047087+viciscat@users.noreply.github.com>

10 files changed, 93 insertions, 44 deletions

diff --git a/src/main/java/de/hysky/skyblocker/skyblock/fishing/FishingHudWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/fishing/FishingHudWidget.java
index eb0cc331..70695270 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/fishing/FishingHudWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/fishing/FishingHudWidget.java
@@ -34,6 +34,11 @@ public class FishingHudWidget extends ComponentBasedWidget {
 	}
 
 	@Override
+	public boolean shouldUpdateBeforeRendering() {
+		return true;
+	}
+
+	@Override
 	public Set<Location> availableLocations() {
 		return Set.of(Location.values());
 	}
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/garden/FarmingHudWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/garden/FarmingHudWidget.java
index efe67cc4..bfa76c2a 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/garden/FarmingHudWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/garden/FarmingHudWidget.java
@@ -71,7 +71,7 @@ public class FarmingHudWidget extends ComponentBasedWidget {
 	}
 
 	@Override
-	protected boolean shouldUpdateBeforeRendering() {
+	public boolean shouldUpdateBeforeRendering() {
 		return true;
 	}
 
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/slayers/hud/SlayerHudWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/slayers/hud/SlayerHudWidget.java
index 252366c9..1ffe5f7d 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/slayers/hud/SlayerHudWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/slayers/hud/SlayerHudWidget.java
@@ -28,6 +28,11 @@ public class SlayerHudWidget extends ComponentBasedWidget {
 		update();
 	}
 
+	@Override
+	public boolean shouldUpdateBeforeRendering() {
+		return true;
+	}
+
 	public static SlayerHudWidget getInstance() {
 		return instance;
 	}
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/config/preview/PreviewTab.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/config/preview/PreviewTab.java
index 4b34230e..1883c434 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/config/preview/PreviewTab.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/config/preview/PreviewTab.java
@@ -122,8 +122,8 @@ public class PreviewTab implements Tab {
 		scoreboard.getOrCreateScore(createHolder(Text.literal("enough lines bye")), placeHolderObjective).setScore(-9);
 		scoreboard.getOrCreateScore(createHolder(Text.literal("NEVER GONNA GIVE Y-")), placeHolderObjective).setScore(-10);
 
-		WidgetManager.getScreenBuilder(getCurrentLocation()).positionWidgets(client.getWindow().getScaledWidth(), client.getWindow().getScaledHeight(), true);
 		locationDropdown = parent.createLocationDropdown(location -> updateWidgets());
+		updateWidgets();
 	}
 
 	private ScoreHolder createHolder(Text name) {
@@ -251,7 +251,9 @@ public class PreviewTab implements Tab {
 		ScreenBuilder screenBuilder = WidgetManager.getScreenBuilder(getCurrentLocation());
 		updatePlayerListFromPreview();
 		float scale = SkyblockerConfigManager.get().uiAndVisuals.tabHud.tabHudScale / 100.f;
-		screenBuilder.positionWidgets((int) (parent.width / scale), (int) (parent.height / scale), true);
+		screenBuilder.updateWidgetLists(true);
+		screenBuilder.updateWidgets(currentScreenLayer);
+		screenBuilder.positionWidgets((int) (parent.width / scale), (int) (parent.height / scale));
 	}
 
 	public enum Mode {
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/ScreenBuilder.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/ScreenBuilder.java
index 78b765ed..fbfd716a 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/ScreenBuilder.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/ScreenBuilder.java
@@ -20,14 +20,18 @@ import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 
 public class ScreenBuilder {
-	public static boolean positionsNeedsUpdating = true;
-
-	//private final String builderName;
+	// TODO: eliminate this static field completely?
+	// 	we can get rid of this field by moving the widget dimensions check into `updateWidgetLists`
+	private static boolean positionsNeedsUpdating = true;
 
 	private final Map<String, PositionRule> positioning = new Object2ObjectOpenHashMap<>();
 	private Map<String, PositionRule> positioningBackup = null;
 	private final Location location;
 
+	private List<HudWidget> hudScreen = new ArrayList<>();
+	private List<HudWidget> mainTabScreen = new ArrayList<>();
+	private List<HudWidget> secondaryTabScreen = new ArrayList<>();
+
 	/**
 	 * Create a ScreenBuilder from a json.
 	 */
@@ -63,16 +67,20 @@ public class ScreenBuilder {
 		positioning.putAll(positioningBackup);
 	}
 
-	private final List<HudWidget> hudScreen = new ArrayList<>();
-	private final List<HudWidget> mainTabScreen = new ArrayList<>();
-	private final List<HudWidget> secondaryTabScreen = new ArrayList<>();
-
-	public void positionWidgets(int screenW, int screenH, boolean config) {
-		hudScreen.clear();
-		mainTabScreen.clear();
-		secondaryTabScreen.clear();
+	public static void markDirty() {
+		positionsNeedsUpdating = true;
+	}
 
-		WidgetPositioner newPositioner = SkyblockerConfigManager.get().uiAndVisuals.tabHud.defaultPositioning.getNewPositioner(screenW, screenH);
+	/**
+	 * Updates the lists of widgets that should be rendered. This method runs every frame to check if any widgets have changed visibility (shouldRender).
+	 * @param config whether this render in happening in the config screen
+	 * @return true if the lists have changed and positioners should run, false if they are the same as before and repositioning is not needed
+	 */
+	public boolean updateWidgetLists(boolean config) {
+		// Save the hud widgets that should be rendered to new lists
+		final List<HudWidget> hudNew = new ArrayList<>();
+		final List<HudWidget> mainTabNew = new ArrayList<>();
+		final List<HudWidget> secondaryTabNew = new ArrayList<>();
 
 		for (HudWidget widget : WidgetManager.widgetInstances.values()) {
 			widget.setVisible(false);
@@ -80,16 +88,15 @@ public class ScreenBuilder {
 				// TODO maybe behavior to change? (having no position rule on a normal hud widget shouldn't quite be possible)
 				PositionRule rule = getPositionRule(widget.getInternalID());
 				if (rule == null) {
-					hudScreen.add(widget);
+					hudNew.add(widget);
 				} else {
 					switch (rule.screenLayer()) {
-						case MAIN_TAB -> mainTabScreen.add(widget);
-						case SECONDARY_TAB -> secondaryTabScreen.add(widget);
-						case null, default -> hudScreen.add(widget);
+						case MAIN_TAB -> mainTabNew.add(widget);
+						case SECONDARY_TAB -> secondaryTabNew.add(widget);
+						case null, default -> hudNew.add(widget);
 					}
 				}
 				widget.setVisible(true);
-				widget.update();
 				widget.setPositioned(false);
 			}
 		}
@@ -98,17 +105,40 @@ public class ScreenBuilder {
 			PositionRule rule = getPositionRule(widget.getInternalID());
 			widget.setVisible(true);
 			if (rule == null) {
-				mainTabScreen.add(widget);
+				mainTabNew.add(widget);
 			} else {
 				widget.setPositioned(false);
 				switch (rule.screenLayer()) {
-					case HUD -> hudScreen.add(widget);
-					case SECONDARY_TAB -> secondaryTabScreen.add(widget);
-					case null, default -> mainTabScreen.add(widget);
+					case HUD -> hudNew.add(widget);
+					case SECONDARY_TAB -> secondaryTabNew.add(widget);
+					case null, default -> mainTabNew.add(widget);
 				}
 			}
 		}
 
+		// Compare the newly generated lists with the old ones
+		if (hudScreen.equals(hudNew) && mainTabScreen.equals(mainTabNew) && secondaryTabScreen.equals(secondaryTabNew)) {
+			return false;
+		}
+		hudScreen = hudNew;
+		mainTabScreen = mainTabNew;
+		secondaryTabScreen = secondaryTabNew;
+
+		return true;
+	}
+
+	/**
+	 * Updates the widgets (if needed) after the new widget list has been generated and before positioners run.
+	 */
+	public void updateWidgets(WidgetManager.ScreenLayer screenLayer) {
+		for (HudWidget widget : getHudWidgets(screenLayer)) {
+			if (widget.shouldUpdateBeforeRendering()) widget.update();
+		}
+	}
+
+	public void positionWidgets(int screenW, int screenH) {
+		WidgetPositioner newPositioner = SkyblockerConfigManager.get().uiAndVisuals.tabHud.defaultPositioning.getNewPositioner(screenW, screenH);
+
 		// Auto positioning
 		for (HudWidget widget : mainTabScreen) {
 
@@ -160,19 +190,18 @@ public class ScreenBuilder {
 	}
 
 	/**
-	 * Run the pipeline to build a Screen
+	 * Builds and renders the given {@link de.hysky.skyblocker.skyblock.tabhud.screenbuilder.WidgetManager.ScreenLayer WidgetManager.ScreenLayer}, which
+	 * {@link #updateWidgetLists(boolean) updates the widget lists (for all screen layers)}, {@link #updateWidgets(WidgetManager.ScreenLayer) updates the widgets (for the current screen layer)},
+	 * {@link #positionWidgets(int, int) positions the widgets}, and {@link #renderWidgets(DrawContext, WidgetManager.ScreenLayer) renders the widgets}.
 	 */
 	public void run(DrawContext context, int screenW, int screenH, WidgetManager.ScreenLayer screenLayer) {
+		boolean widgetListsChanged = updateWidgetLists(false);
 
-        /*int i = 0;
-        for (TabHudWidget value : PlayerListMgr.tabWidgetInstances.values()) {
-            context.drawText(MinecraftClient.getInstance().textRenderer, value.getHypixelWidgetName(), 0, i, PlayerListMgr.tabWidgetsToShow.contains(value) ? Colors.LIGHT_YELLOW : -1, true);
-            i += 9;
-        }*/
+		updateWidgets(screenLayer);
 
-		if (positionsNeedsUpdating) {
+		if (widgetListsChanged || positionsNeedsUpdating) {
 			positionsNeedsUpdating = false;
-			positionWidgets(screenW, screenH, false);
+			positionWidgets(screenW, screenH);
 		}
 
 		renderWidgets(context, screenLayer);
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/WidgetManager.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/WidgetManager.java
index f2040975..b2b99adb 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/WidgetManager.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/screenbuilder/WidgetManager.java
@@ -8,7 +8,6 @@ import com.mojang.serialization.JsonOps;
 import de.hysky.skyblocker.SkyblockerMod;
 import de.hysky.skyblocker.annotations.Init;
 import de.hysky.skyblocker.config.SkyblockerConfigManager;
-import de.hysky.skyblocker.events.SkyblockEvents;
 import de.hysky.skyblocker.skyblock.tabhud.TabHud;
 import de.hysky.skyblocker.skyblock.tabhud.config.WidgetsConfigurationScreen;
 import de.hysky.skyblocker.skyblock.tabhud.screenbuilder.pipeline.PositionRule;
@@ -62,8 +61,6 @@ public class WidgetManager {
 	// we probably want this to run pretty early?
 	@Init(priority = -1)
 	public static void init() {
-		SkyblockEvents.LOCATION_CHANGE.register(location -> ScreenBuilder.positionsNeedsUpdating = true);
-
 		ClientLifecycleEvents.CLIENT_STARTED.register(client -> {
 
 			instantiateWidgets();
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListManager.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListManager.java
index 7525afa6..83cd1a60 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListManager.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListManager.java
@@ -4,7 +4,6 @@ import com.mojang.authlib.GameProfile;
 import de.hysky.skyblocker.config.SkyblockerConfigManager;
 import de.hysky.skyblocker.mixins.accessors.PlayerListHudAccessor;
 import de.hysky.skyblocker.skyblock.tabhud.config.WidgetsConfigurationScreen;
-import de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder;
 import de.hysky.skyblocker.skyblock.tabhud.screenbuilder.WidgetManager;
 import de.hysky.skyblocker.skyblock.tabhud.widget.TabHudWidget;
 import de.hysky.skyblocker.skyblock.tabhud.widget.component.PlainTextComponent;
@@ -62,10 +61,10 @@ public class PlayerListManager {
 	private static void reset() {
 		if (!tabWidgetsToShow.isEmpty()) {
 			tabWidgetsToShow.clear();
-			ScreenBuilder.positionsNeedsUpdating = true;
 		}
 	}
 
+	// TODO: check for changes instead of updating every second
 	public static void updateList() {
 		if (!Utils.isOnSkyblock()) {
 			reset();
@@ -195,7 +194,6 @@ public class PlayerListManager {
 		if (!tabWidgetsToShow.contains(tabWidgetInstances.get("Active Effects")) && SkyblockerConfigManager.get().uiAndVisuals.tabHud.effectsFromFooter) {
 			tabWidgetsToShow.add(getTabHudWidget("Active Effects", List.of()));
 		}
-		ScreenBuilder.positionsNeedsUpdating = true;
 	}
 
 	private static Text trim(Text text) {
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/ComponentBasedWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/ComponentBasedWidget.java
index 4e03dd8a..607b321d 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/ComponentBasedWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/ComponentBasedWidget.java
@@ -161,7 +161,7 @@ public abstract class ComponentBasedWidget extends HudWidget {
 		// min width is dependent on title
 		w = Math.max(w, BORDER_SZE_W + BORDER_SZE_E + txtRend.getWidth(title) + 4 + 4 + 1);
 		// update the positions so it doesn't wait for the next tick or something
-		if (h != prevH || w != prevW) ScreenBuilder.positionsNeedsUpdating = true;
+		if (h != prevH || w != prevW) ScreenBuilder.markDirty();
 		prevW = w;
 		prevH = h;
 	}
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/HudWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/HudWidget.java
index 59ecbafc..5a0a3354 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/HudWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/HudWidget.java
@@ -64,19 +64,18 @@ public abstract class HudWidget extends AbstractWidget {
 	 *
 	 * @return true if it should update
 	 */
-	protected boolean shouldUpdateBeforeRendering() {
+	public boolean shouldUpdateBeforeRendering() {
 		return false;
 	}
 
 	protected abstract void renderWidget(DrawContext context, int mouseX, int mouseY, float delta);
 
-	public void render(DrawContext context) {
+	public final void render(DrawContext context) {
 		render(context, -1, -1, MinecraftClient.getInstance().getRenderTickCounter().getDynamicDeltaTicks());
 	}
 
 	@Override
 	public final void render(DrawContext context, int mouseX, int mouseY, float delta) {
-		if (shouldUpdateBeforeRendering()) update();
 		renderWidget(context, mouseX, mouseY, delta);
 	}
 
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/TabHudWidget.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/TabHudWidget.java
index 8fac951c..677b60e5 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/TabHudWidget.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/widget/TabHudWidget.java
@@ -41,7 +41,9 @@ public abstract class TabHudWidget extends ComponentBasedWidget {
 	}
 
 	/**
-	 * Controlled by hypxiel and PlayerListMgr
+	 * Controlled by Hypixel and {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager PlayerListManager}.
+	 * {@link de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder#updateWidgetLists(boolean) ScreenBuilder#updateWidgetLists}
+	 * take the widgets directly from {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager#tabWidgetsToShow PlayerListManager#tabWidgetsToShow}.
 	 */
 	@Override
 	public final boolean shouldRender(Location location) {
@@ -49,16 +51,28 @@ public abstract class TabHudWidget extends ComponentBasedWidget {
 	}
 
 	/**
-	 * Controlled by hypxiel and PlayerListMgr
+	 * Controlled by Hypixel and {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager PlayerListManager}.
+	 * {@link de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder#updateWidgetLists(boolean) ScreenBuilder#updateWidgetLists}
+	 * take the widgets directly from {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager#tabWidgetsToShow PlayerListManager#tabWidgetsToShow}.
 	 */
 	@Override
 	public final Set<Location> availableLocations() {
 		return Set.of();
 	}
 
+	/**
+	 * Controlled by Hypixel and {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager PlayerListManager}.
+	 * {@link de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder#updateWidgetLists(boolean) ScreenBuilder#updateWidgetLists}
+	 * take the widgets directly from {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager#tabWidgetsToShow PlayerListManager#tabWidgetsToShow}.
+	 */
 	@Override
 	public final void setEnabledIn(Location location, boolean enabled) {}
 
+	/**
+	 * Controlled by Hypixel and {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager PlayerListManager}.
+	 * {@link de.hysky.skyblocker.skyblock.tabhud.screenbuilder.ScreenBuilder#updateWidgetLists(boolean) ScreenBuilder#updateWidgetLists}
+	 * take the widgets directly from {@link de.hysky.skyblocker.skyblock.tabhud.util.PlayerListManager#tabWidgetsToShow PlayerListManager#tabWidgetsToShow}.
+	 */
 	@Override
 	public final boolean isEnabledIn(Location location) {
 		return false;