fix: avoid incrementing completed collections if not initialized (#374)

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## Unreleased
 
+- Bugfix: Don't report inaccurate completed collections count, when character summary tab was not selected. (#374)
 - Bugfix: Avoid undercounting diary completions for notifications that occur shortly after a teleport. (#373)
 
 ## 1.7.2

src/main/java/dinkplugin/notifiers/CollectionNotifier.java

@@ -79,27 +79,24 @@ public void onGameState(GameState newState) {
 
     public void onTick() {
         if (client.getGameState() != GameState.LOGGED_IN) {
-            // indicate that the latest completion count should be updated
+            // this shouldn't ever happen, but just in case
             completed.set(-1);
         } else if (completed.get() < 0) {
             // initialize collection log entry completion count
-            completed.set(client.getVarpValue(COMPLETED_VARP));
+            int varpValue = client.getVarpValue(COMPLETED_VARP);
+            if (varpValue > 0)
+                completed.set(varpValue);
         }
     }
 
     public void onVarPlayer(VarbitChanged event) {
         if (event.getVarpId() != COMPLETED_VARP)
             return;
 
-        // Currently, this varp is sent early enough to be read on the first logged-in tick.
-        // For robustness, we also allow initialization here just in case the varp is sent with greater delay.
-
-        // Note: upon a completion, this varp is not updated until a few ticks after the collection log message.
-        // However, this behavior could also change, which is why here we don't synchronize "completed" beyond initialization.
-
-        int old = completed.get();
-        if (old <= 0) {
-            completed.compareAndSet(old, event.getValue());
+        // we only care about this event when the notifier is disabled
+        // to keep `completed` updated when `handleNotify` is not being called
+        if (!config.notifyCollectionLog()) {
+            completed.set(event.getValue());
         }
     }
 
@@ -139,7 +136,7 @@ private void handleNotify(String itemName) {
         // varp isn't updated for a few ticks, so we increment the count locally.
         // this approach also has the benefit of yielding incrementing values even when
         // multiple collection log entries are completed within a single tick.
-        int completed = this.completed.incrementAndGet();
+        int completed = this.completed.updateAndGet(i -> i >= 0 ? i + 1 : i);
         int total = client.getVarpValue(TOTAL_VARP); // unique; doesn't over-count duplicates
         boolean varpValid = total > 0 && completed > 0;
         if (!varpValid) {

src/test/java/dinkplugin/notifiers/CollectionNotifierTest.java

@@ -12,12 +12,15 @@
 import net.runelite.api.ScriptID;
 import net.runelite.api.VarClientStr;
 import net.runelite.api.Varbits;
+import net.runelite.api.events.VarbitChanged;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 
+import java.util.function.BiFunction;
+
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.Mockito.never;
@@ -41,17 +44,22 @@ class CollectionNotifierTest extends MockedNotifierTest {
     protected void setUp() {
         super.setUp();
 
-        // init config mocks
-        when(config.notifyCollectionLog()).thenReturn(true);
-        when(config.collectionSendImage()).thenReturn(false);
-        when(config.collectionNotifyMessage()).thenReturn("%USERNAME% has added %ITEM% to their collection");
-
         // init client mocks
         when(client.getVarbitValue(Varbits.COLLECTION_LOG_NOTIFICATION)).thenReturn(1);
         when(client.getVarpValue(CollectionNotifier.COMPLETED_VARP)).thenReturn(0);
         when(client.getVarpValue(CollectionNotifier.TOTAL_VARP)).thenReturn(TOTAL_ENTRIES);
         when(client.getGameState()).thenReturn(GameState.LOGGED_IN);
         notifier.onTick();
+
+        VarbitChanged initCompleted = new VarbitChanged();
+        initCompleted.setVarpId(CollectionNotifier.COMPLETED_VARP);
+        initCompleted.setValue(0);
+        notifier.onVarPlayer(initCompleted);
+
+        // init config mocks
+        when(config.notifyCollectionLog()).thenReturn(true);
+        when(config.collectionSendImage()).thenReturn(false);
+        when(config.collectionNotifyMessage()).thenReturn("%USERNAME% has added %ITEM% to their collection");
     }
 
     @Test
@@ -121,6 +129,89 @@ void testNotifyPopup() {
         );
     }
 
+    @Test
+    void testNotifyFresh() {
+        notifier.reset();
+
+        /*
+         * first notification: no varbit data
+         */
+        when(client.getVarpValue(CollectionNotifier.COMPLETED_VARP)).thenReturn(0);
+        when(client.getVarpValue(CollectionNotifier.TOTAL_VARP)).thenReturn(0);
+
+        String item = "Seercull";
+        int price = 23_000;
+        when(itemSearcher.findItemId(item)).thenReturn(ItemID.SEERCULL);
+        when(itemManager.getItemPrice(ItemID.SEERCULL)).thenReturn(price);
+
+        // send fake message
+        notifier.onChatMessage("New item added to your collection log: " + item);
+
+        // verify handled
+        verify(messageHandler).createMessage(
+            PRIMARY_WEBHOOK_URL,
+            false,
+            NotificationBody.builder()
+                .text(
+                    Template.builder()
+                        .template(String.format("%s has added {{item}} to their collection", PLAYER_NAME))
+                        .replacement("{{item}}", Replacements.ofWiki(item))
+                        .build()
+                )
+                .extra(new CollectionNotificationData(item, ItemID.SEERCULL, (long) price, null, null))
+                .type(NotificationType.COLLECTION)
+                .build()
+        );
+
+        /*
+         * jagex sends varbit data shortly after the notification
+         */
+        BiFunction<Integer, Integer, VarbitChanged> varpEvent = (id, value) -> {
+            VarbitChanged e = new VarbitChanged();
+            e.setVarpId(id);
+            e.setValue(value);
+            return e;
+        };
+
+        when(client.getVarpValue(CollectionNotifier.COMPLETED_VARP)).thenReturn(1);
+        notifier.onVarPlayer(varpEvent.apply(CollectionNotifier.COMPLETED_VARP, 1));
+
+        when(client.getVarpValue(CollectionNotifier.TOTAL_VARP)).thenReturn(TOTAL_ENTRIES);
+        notifier.onVarPlayer(varpEvent.apply(CollectionNotifier.TOTAL_VARP, TOTAL_ENTRIES));
+
+        when(client.getVarpValue(CollectionNotifier.COMPLETED_VARP)).thenReturn(100);
+        notifier.onVarPlayer(varpEvent.apply(CollectionNotifier.COMPLETED_VARP, 100));
+
+        notifier.onTick();
+
+        /*
+         * a later notification occurs
+         */
+        String item2 = "Seers ring";
+        int price2 = 420_000;
+        when(itemSearcher.findItemId(item2)).thenReturn(ItemID.SEERS_RING);
+        when(itemManager.getItemPrice(ItemID.SEERS_RING)).thenReturn(price2);
+
+        // send fake message
+        notifier.onChatMessage("New item added to your collection log: " + item2);
+
+        // verify handled
+        verify(messageHandler).createMessage(
+            PRIMARY_WEBHOOK_URL,
+            false,
+            NotificationBody.builder()
+                .text(
+                    Template.builder()
+                        .template(String.format("%s has added {{item}} to their collection", PLAYER_NAME))
+                        .replacement("{{item}}", Replacements.ofWiki(item2))
+                        .build()
+                )
+                .extra(new CollectionNotificationData(item2, ItemID.SEERS_RING, (long) price2, 101, TOTAL_ENTRIES))
+                .type(NotificationType.COLLECTION)
+                .build()
+        );
+    }
+
     @Test
     void testIgnore() {
         // send unrelated message