Repository: pajlads/DinkPlugin
Pull request #373 (2023-11-25 23:49:52)

fix: avoid undercounting completed diaries after teleports
When client goes into `LOADING` state, we would clear the completed diary state. As a result, if a diary completion occurred shortly after a teleport, the notifier would think there were 0 previous diaries completed. This PR avoids clearing diary state on teleports (and other potentially-small movements that trigger the loading state).



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1c45359..15ad47b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Unreleased
 
+- Bugfix: Avoid undercounting diary completions for notifications that occur shortly after a teleport. (#373)
+
 ## 1.7.2
 
 - Dev: Utilize runelite event for unsired loot instead of custom widget handler (#375).
diff --git a/src/main/java/dinkplugin/DinkPlugin.java b/src/main/java/dinkplugin/DinkPlugin.java
index afd963d..54c8982 100644
--- a/src/main/java/dinkplugin/DinkPlugin.java
+++ b/src/main/java/dinkplugin/DinkPlugin.java
@@ -22,6 +22,7 @@ import dinkplugin.notifiers.SpeedrunNotifier;
 import dinkplugin.util.Utils;
 import lombok.extern.slf4j.Slf4j;
 import net.runelite.api.ChatMessageType;
+import net.runelite.api.GameState;
 import net.runelite.api.events.AccountHashChanged;
 import net.runelite.api.events.ActorDeath;
 import net.runelite.api.events.ChatMessage;
@@ -52,6 +53,7 @@ import net.runelite.client.util.ColorUtil;
 
 import javax.inject.Inject;
 import java.awt.Color;
+import java.util.concurrent.atomic.AtomicReference;
 
 @Slf4j
 @PluginDescriptor(
@@ -86,6 +88,8 @@ public class DinkPlugin extends Plugin {
     private @Inject LeaguesNotifier leaguesNotifier;
     private @Inject MetaNotifier metaNotifier;
 
+    private final AtomicReference<GameState> gameState = new AtomicReference<>();
+
     @Override
     protected void startUp() {
         log.debug("Started up Dink");
@@ -98,6 +102,7 @@ public class DinkPlugin extends Plugin {
     protected void shutDown() {
         log.debug("Shutting down Dink");
         this.resetNotifiers();
+        gameState.lazySet(null);
     }
 
     void resetNotifiers() {
@@ -145,12 +150,24 @@ public class DinkPlugin extends Plugin {
 
     @Subscribe
     public void onGameStateChanged(GameStateChanged gameStateChanged) {
-        settingsManager.onGameState(gameStateChanged);
-        collectionNotifier.onGameState(gameStateChanged);
+        GameState newState = gameStateChanged.getGameState();
+        if (newState == GameState.LOADING) {
+            // an intermediate state that is irrelevant for our notifiers; ignore
+            return;
+        }
+
+        GameState previousState = gameState.getAndSet(newState);
+        if (previousState == newState) {
+            // no real change occurred (just momentarily went through LOADING); ignore
+            return;
+        }
+
+        settingsManager.onGameState(previousState, newState);
+        collectionNotifier.onGameState(newState);
         levelNotifier.onGameStateChanged(gameStateChanged);
         diaryNotifier.onGameState(gameStateChanged);
         grandExchangeNotifier.onGameStateChange(gameStateChanged);
-        metaNotifier.onGameState(gameStateChanged);
+        metaNotifier.onGameState(previousState, newState);
     }
 
     @Subscribe
diff --git a/src/main/java/dinkplugin/SettingsManager.java b/src/main/java/dinkplugin/SettingsManager.java
index 0777c07..a629be5 100644
--- a/src/main/java/dinkplugin/SettingsManager.java
+++ b/src/main/java/dinkplugin/SettingsManager.java
@@ -200,14 +200,19 @@ public class SettingsManager {
         }
     }
 
-    void onGameState(GameStateChanged event) {
-        if (event.getGameState() != GameState.LOGGED_IN) {
+    void onGameState(GameState oldState, GameState newState) {
+        if (newState != GameState.LOGGED_IN) {
             justLoggedIn.set(false);
             return;
         } else {
             justLoggedIn.set(true);
         }
 
+        if (oldState == GameState.HOPPING) {
+            // avoid repeated warnings after login
+            return;
+        }
+
         // Since varbit values default to zero and no VarbitChanged occurs if the
         // newly received value is equal to the existing value, we must manually
         // check those where 0 is an invalid value deserving of a warning.
diff --git a/src/main/java/dinkplugin/notifiers/CollectionNotifier.java b/src/main/java/dinkplugin/notifiers/CollectionNotifier.java
index cca81f0..9bfbd54 100644
--- a/src/main/java/dinkplugin/notifiers/CollectionNotifier.java
+++ b/src/main/java/dinkplugin/notifiers/CollectionNotifier.java
@@ -15,7 +15,6 @@ import net.runelite.api.ScriptID;
 import net.runelite.api.VarClientStr;
 import net.runelite.api.Varbits;
 import net.runelite.api.annotations.Varp;
-import net.runelite.api.events.GameStateChanged;
 import net.runelite.api.events.VarbitChanged;
 import net.runelite.client.callback.ClientThread;
 import net.runelite.client.game.ItemManager;
@@ -73,8 +72,8 @@ public class CollectionNotifier extends BaseNotifier {
         this.popupStarted.set(false);
     }
 
-    public void onGameState(GameStateChanged event) {
-        if (event.getGameState() != GameState.LOGGED_IN)
+    public void onGameState(GameState newState) {
+        if (newState != GameState.HOPPING && newState != GameState.LOGGED_IN)
             this.reset();
     }
 
diff --git a/src/main/java/dinkplugin/notifiers/MetaNotifier.java b/src/main/java/dinkplugin/notifiers/MetaNotifier.java
index 4269fb6..18d2085 100644
--- a/src/main/java/dinkplugin/notifiers/MetaNotifier.java
+++ b/src/main/java/dinkplugin/notifiers/MetaNotifier.java
@@ -42,7 +42,6 @@ public class MetaNotifier extends BaseNotifier {
     static final @VisibleForTesting int INIT_TICKS = 10; // 6 seconds after login
 
     private final AtomicInteger loginTicks = new AtomicInteger(-1);
-    private final AtomicReference<GameState> gameState = new AtomicReference<>();
 
     @Inject
     private ClientThread clientThread;
@@ -63,13 +62,8 @@ public class MetaNotifier extends BaseNotifier {
         return config.metadataWebhook();
     }
 
-    public void onGameState(GameStateChanged event) {
-        GameState newState = event.getGameState();
-        if (newState == GameState.LOADING) {
-            // ignore this intermediate state
-            return;
-        }
-        GameState oldState = gameState.getAndSet(newState);
+    public void onGameState(GameState oldState, GameState newState) {
+        // inspect oldState because we don't want a notification on each world hop
         if (oldState == GameState.LOGGING_IN && newState == GameState.LOGGED_IN) {
             loginTicks.set(INIT_TICKS);
         }


Test patch:
diff --git a/src/test/java/dinkplugin/notifiers/MetaNotifierTest.java b/src/test/java/dinkplugin/notifiers/MetaNotifierTest.java
index 23c186b..994918c 100644
--- a/src/test/java/dinkplugin/notifiers/MetaNotifierTest.java
+++ b/src/test/java/dinkplugin/notifiers/MetaNotifierTest.java
@@ -12,7 +12,6 @@ import net.runelite.api.ItemID;
 import net.runelite.api.Skill;
 import net.runelite.api.VarPlayer;
 import net.runelite.api.Varbits;
-import net.runelite.api.events.GameStateChanged;
 import net.runelite.client.config.RuneLiteConfig;
 import org.apache.commons.lang3.StringUtils;
 import org.junit.jupiter.api.Assertions;
@@ -87,9 +86,7 @@ class MetaNotifierTest extends MockedNotifierTest {
     @Test
     void testNotify() {
         // fire event
-        notifier.onGameState(event(GameState.LOGGING_IN));
-        notifier.onGameState(event(GameState.LOADING));
-        notifier.onGameState(event(GameState.LOGGED_IN));
+        notifier.onGameState(GameState.LOGGING_IN, GameState.LOGGED_IN);
         IntStream.rangeClosed(0, MetaNotifier.INIT_TICKS).forEach(i -> notifier.onTick());
 
         // verify handled
@@ -127,9 +124,7 @@ class MetaNotifierTest extends MockedNotifierTest {
         when(client.getVarpValue(CollectionNotifier.TOTAL_VARP)).thenReturn(0);
 
         // fire events
-        notifier.onGameState(event(GameState.LOGGING_IN));
-        notifier.onGameState(event(GameState.LOADING));
-        notifier.onGameState(event(GameState.LOGGED_IN));
+        notifier.onGameState(GameState.LOGGING_IN, GameState.LOGGED_IN);
         IntStream.rangeClosed(0, MetaNotifier.INIT_TICKS).forEach(i -> notifier.onTick());
 
         // verify handled
@@ -166,9 +161,7 @@ class MetaNotifierTest extends MockedNotifierTest {
         when(config.metadataWebhook()).thenReturn("");
 
         // fire event
-        GameStateChanged event = new GameStateChanged();
-        event.setGameState(GameState.LOGGED_IN);
-        notifier.onGameState(event);
+        notifier.onGameState(GameState.LOGGING_IN, GameState.LOGGED_IN);
         IntStream.rangeClosed(0, MetaNotifier.INIT_TICKS).forEach(i -> notifier.onTick());
 
         // ensure no notification
@@ -192,10 +185,4 @@ class MetaNotifierTest extends MockedNotifierTest {
         );
         Assertions.assertEquals(expected, notifier.getPets());
     }
-
-    private static GameStateChanged event(GameState state) {
-        GameStateChanged event = new GameStateChanged();
-        event.setGameState(state);
-        return event;
-    }
 }