Fix wrong pings due to partial matches

[Phoenix616]

This fixes the issue that depending on the order of the member, role and channel list mentions might ping the wrong one e.g. @phoenix616 could ping a user with the name phoenix if they are in the member list before phoenix616 as the StringUtil#replaceTarget method is called with requireSeparation = false (which is fine if mentions that are followed by other characters like commas are meant to be replaced but it also means that wrong users might be matched).

This is done by first only checking for mentions that are separated by spaces/line endings and only after that trying the original, partial match to handle special cases that aren't easily distinguishable as user tags (e.g. commas after a ping).

While this is obviously less efficient than the original method I still believe that it can be more efficient than trying to actually determine a possible mention from the input string as that would mean searching through all users, roles and channels for every possible mention. Granted most messages might only include one mention so maybe a complete rework of how mentions are parsed could indeed be better in the long term but my approach is just a quicker fix for the underlying issue right now.

A completely different approach could be to sort the member/role/channel list by their tag string length in order to replace the longest first but I feel like that sorting could be even slower than this approach.

[zachbr]

Can you include a test case for this please? Probably just a new method in src/test/kotlin/io/zachbr/dis4irc/bridge/pier/discord/DiscordPierTest.kt

I have not done too good of a job with them but avoiding this in the future would be nice. Thanks

[Phoenix616]

While I would love to do that I'm unsure how I would go on about testing this seeing as it requires a JDA Guild object and it's related role/member/channel caches. Also the behaviour of the StringUtil#replaceTarget method with the requireSeparation parameter set to true is already tested so I don't think it's really that necessary to also test the loops itself seeing as all they will do is call that method with two different values for the parameter so the only thing a test could catch here is if someone changes the DiscordPier#sendMessage method in the future and doesn't account for this case. I think I'll add some comments there to mention why the duplicate replaceMentions call is there and what it solves so that that can't happen.

But I guess this is a sign that the code might have to be done differently in a more abstract and therefore testable way (which I'm not sure is easily doable or performant here but if you'd prefer that then I could try to take a look at that but I'm not sure if I can manage that seeing as kotlin isn't really my forte) or use some mock JDA implementation (which I'm not aware if such a thing exists). But I guess it's up to you here if you see this as a necessary overall goal.

[Phoenix616]

Any chance of getting this pulled/the issue fixed at some point? 👀

[zachbr]

Hi! 👋 Definitely should get this fixed soon. This issue, and many others, have sat for too long. Things are pretty busy, but clearly this needs some attention. I hope to have it added soon along with other changes.

[zachbr]

Thanks

[Phoenix616]

This somehow seems to be broken again 🤔

EDIT: Actually seems to be a different issue now as it partially matches the ID pings. wat