feat: Add Herbiboar to Kill Count Notifier

[jdanthdavis]

Allows Heribobar kill counts to send kill count notifications. Wintertodt and Tempoross are acceptable so we should include Herbiboar to keep the "skilling" NPCs consistent. We should also consider adding Hunter Rumour completions.

[jdanthdavis]

Should a unique test case be created for in-game messages that contain "harvest"?

[iProdigy]

Should a unique test case be created for in-game messages that contain "harvest"?

I think the MatchersTest case is sufficient - the only special part of herbiboar kc is there's no time, but that edge case is already covered in KillCountNotifierTest

[iProdigy]

Code looks good to me - only question for other maintainers is whether there should be a config option to disable hunter-related kc notifs

[jdanthdavis]

Should a unique test case be created for in-game messages that contain "harvest"?

I think the MatchersTest case is sufficient - the only special part of herbiboar kc is there's no time, but that edge case is already covered in KillCountNotifierTest

Hmm, maybe I'm misunderstanding how the test cases work then. Is the updated portion of MatchersTest.java responsible for making sure text=Template(template=dank dank has defeated {{boss}} with a completion count of 1, replacements={{{boss}}=Replacements.TextWithLink(text=herbiboar, link=https://oldschool.runescape.wiki/w/Special:Search?search=herbiboar)} is getting correctly formatted because of Herbi's unique harvest count message?

[iProdigy]

Correct, MatchersTest is checking whether KillCountNotifier.parseBoss yields the correct "boss" name and completion count, which directly populates that template

[jdanthdavis]

Code looks good to me - only question for other maintainers is whether there should be a config option to disable hunter-related kc notifs

Not directed towards me so take this with a grain of salt. If I was a new user coming in I would question why only hunter-related (Herbiboar & Rumours) notifications can be toggled (even more confused if they're disabled by default) but not other skilling mini-games. Unless, you all don't consider either of those hunter activities mini-games then that's sufficient enough. I do think this question could also lead to a cluttered UI with all the skilling mini-games to toggle (if they weren't grouped by skill and listed individually.)

[iProdigy]

If I was a new user coming in I would question why only hunter-related (Herbiboar & Rumours) notifications can be toggled (even more confused if they're disabled by default) but not other skilling mini-games. Unless, you all don't consider either of those hunter activities mini-games then that's sufficient enough. I do think this question could also lead to a cluttered UI with all the skilling mini-games to toggle (if they weren't grouped by skill and listed individually.)

The notifier is originally meant for bosses and I think this is the first (pure) skilling item added to the notifier (gauntlet still involves a combat boss, and tempoross/wintertodt is considered as a boss rather than a minigame) - as such, we don't want the notifier to be too spammy for easily completed items (hence the question about a config toggle)

[jdanthdavis]

If I was a new user coming in I would question why only hunter-related (Herbiboar & Rumours) notifications can be toggled (even more confused if they're disabled by default) but not other skilling mini-games. Unless, you all don't consider either of those hunter activities mini-games then that's sufficient enough. I do think this question could also lead to a cluttered UI with all the skilling mini-games to toggle (if they weren't grouped by skill and listed individually.)

The notifier is originally meant for bosses and I think this is the first (pure) skilling item added to the notifier (gauntlet still involves a combat boss, and tempoross/wintertodt is considered as a boss rather than a minigame) - as such, we don't want the notifier to be too spammy for easily completed items (hence the question about a config toggle)

Very fair point. I actually didn't even know Temp/Todt were consider bosses in-game rather than mini-games. Looking at it from that perspective I do see the concern with adding Herbiboar. Perhaps, this was just a nice learning experience instead 🙂

[iProdigy]

Perhaps, this was just a nice learning experience instead 🙂

Glad to hear it was fulfilling! Did you have a use case for these hunter kc's or were just implementing it as a perceived shortcoming in general? If the former, we can consider how to best accommodate the use case (e.g., config options). If the latter, perhaps we defer this PR until somebody needs it

[jdanthdavis]

Did you have a use case for these hunter kc's or were just implementing it as a perceived shortcoming in general? If the former, we can consider how to best accommodate the use case (e.g., config options). If the latter, perhaps we defer this PR until somebody needs it

We have some members with with over 10k Herbi KC so it's a fun thing to keep tabs on. Otherwise, I definitely thought I found a miss 😅. It's nothing needed on our end. I'd only bark up this tree for this feature to help with the implementation of adding the other non-boss KCs and how we'd adjust the configs.

[pajlada]

Looks good to me, I don't think an option is needed until someone ask for one

[pajlada]

We should probably add this to the in game changelog though, to ensure it gets visibility

[Felanbird]

[jdanthdavis]

We should probably add this to the in game changelog though, to ensure it gets visibility

For clarification and learning purposes, is VersionManager.java responsible for the in-game chat message on login?

cc: @iProdigy

[iProdigy]

For clarification and learning purposes, is VersionManager.java responsible for the in-game chat message on login?

Yep! The current version (with a message attached) is stored in config & on login we tell the user if there's been any notable changes since that version

[iProdigy]

Thanks for your contribution!