pajlads / DinkPlugin

Sends level up, clue, etc. notifications to a Discord webhook or a custom web server
https://runelite.net/plugin-hub/show/dink
BSD 2-Clause "Simplified" License
40 stars 17 forks source link

Dynamic Config QOL #594

Closed Nallieheai closed 1 day ago

Nallieheai commented 1 week ago

Checklist

Describe your Suggestion

Hello!

Ive been messing around with the Dynamic Config URL as i was adviced in the last issue i created. It is working great and doing everything we could possbily want, however i have some suggestions to potentially improve it. One issue i have ran into is that when we remove items from the lootItemAllowlist it does not get removed upon a restart of RuneLite for the players that use the config URL.

I went and did some digging in the repo and found this line: if (webhookConfigKeys.contains(key) || "ignoredNames".equals(key) || "lootItemAllowlist".equals(key) || "lootItemDenylist".equals(key))

  1. Would it be possible to add some checkbox to decide if you would like to override these settings individually?

  2. I understand this might make it more messy under the advanced tab so perhaps it would be good to add a new tab for Dynamic Config URL as a whole, it is a very powerful tool and it would be awesome to have some more configuration for it.

  3. A Reload button to fetch all the data from the Dynamic Config URL again without having to restart RuneLite or manually resetting the settings and then placing the URL in there again, closing the panel and opening it again. (I understand closing and opening the panel is not something we can get around, i think?)

  4. An option to change how often it fetches the config, for example:

    • Upon each login (not world hops)
    • Upon start of RuneLite (current setting?)
  5. If any of these settings seem reasonable and are added, it would be nice to have those added to the config as well so we can set that as a default right away so the user does not have to manually enable some of these settings.

Reasoning

I understand the reasoning why it merges for the ignoredNames, that is not something we have gotten around to using yet, but we would like to take advantage of it in the future so items do not post from people playing multiple accounts if possible. However this could also be solved with a custom webhook handler recommended by iProdigy in my last post.

I added "Bones" to the whitelist of items while testing and then removed it later, but some people ended up getting the updated config, but once the bones had been removed from the config it wouldnt remove it from the people that got that update within the 1-2 hours it was there during development of the Discord Bot. If an opsie happens during a Clan vs Clan bingo with an item, where there are hundreds of participants it would be great to be able to simply update the config and know that it will be gone upon the next restart for all of the users. This is also why it would be good to have a button to fetch all the settings again (though it is hard enough as it is to get people to even use a plugin to begin with)

Hence my suggestion, allow us to completely replace the lootItemAllowlist and lootItemDenylist while leaving the ignoredNames untouched or having it merge as it does now by using checkboxes. Currently these three are the only special cases so if all the settings that has to do with Dynmaic Config URL was placed in its own tab it wouldnt be too messy to have those 3 extra checkboxes i reckon.

The part about having it fetch the new data upon each login would be nice because often before a bingo we will ask all participants to relog to make sure websites such as WiseOldMan, TempleOSRS, etc will update so that XP/boss kc is tracked correctly. This would also allow us to make sure everyone receive the latest version of the config upon logging in just before the event starts w/o having to do anything special.

Thanks for reading!

iProdigy commented 1 week ago
  1. Via #595, users will be able to customize whether config imports (including the dynamic config url) should overwrite certain string lists or merge (current default). i didn't see a use case for overwriting ignoredNames rather than merging, so didn't include that as an option

Screenshot_20241115_231246

  1. What other customization are you imagining?

  2. The runelite config panel doesn't allow us to easily/cleanly add arbitrary buttons. Instead, people can toggle Dink off & on to pull the latest dynamic config

  3. imo if you make a bingo adjustment that you'd like to see immediately reflected, you should just ping @here in discord saying to toggle the plugin off & on

  4. imo the user should set the config overwrite vs. merge policy once & it shouldn't be modified by config imports

Nallieheai commented 1 week ago
  1. Via feat: allow overwriting string lists on import #595, users will be able to customize whether config imports (including the dynamic config url) should overwrite certain string lists or merge (current default). i didn't see a use case for overwriting ignoredNames rather than merging, so didn't include that as an option

Screenshot_20241115_231246

Oh this is great, i mustve missed it because i specifically looked for "Dynamic Config" when creating the issue, sorry for the duplicate but im glad you guys are already working on that!

  1. What other customization are you imagining?

I was mostly thinking that it could be "clean" to have all the settings related to Dynamic Config in a tab of its own in case it grows in the future, but no need if u guys can fit it nicely in the advanced section. But to be fair i did miss that this feature even existed originally because it feels a little bit hidden away in the settings.

  1. The runelite config panel doesn't allow us to easily/cleanly add arbitrary buttons. Instead, people can toggle Dink off & on to pull the latest dynamic config

I see, so buttons are not so easy to add. Honestly speaking i never actually thought of just turning on and off the plugin lol, thats my bad in hindsight, obviously it would load it upon reloading the plugin. That will be fine!

  1. imo if you make a bingo adjustment that you'd like to see immediately reflected, you should just ping @here in discord saying to toggle the plugin off & on

Thats a fair point, im just wondering if there would be any downside to having it read the config upon each login? (not including world hops) Or at least having it as an option that is turned off by default? Could that cause any problems? Personally i feel like it would be nice to make sure the people using Dynamic Configs always have the latest version upon logging in, i myself sometimes dont close the client for 2-3 days when i leave my PC on from time to time. Obviously turning the plugin on and off would be an option but it would be nice if it just happened in the backgroud if its not a too expensive task to perform?

  1. imo the user should set the config overwrite vs. merge policy once & it shouldn't be modified by config imports

Thats fair, as long as the option is there that would be great!

iProdigy commented 1 week ago
  1. Oh you didn't miss it! I only created that PR after you made this issue
  2. Yeah if we were doing separate checkboxes for "Overwrite webhooks" and "Overwrite item lists," I can see the argument for a separate config section. right now I think we can fit all the customization we need into that multi-select box within the advanced section
  3. Perfect, yeah we probably should add that to the README (and/or the tooltip for the config option) so it's more clearly documented
  4. Perhaps that can be a separate PR where: on login, if dynamic config hasn't been pulled in x hours, pull the latest config
  5. Yeah we're creating a release this weekend, so #595 will likely go into the release after that, pending maintainer feedback
Nallieheai commented 1 week ago

Sounds great, looking forward to the next release! Thanks for all the work you and the other contributors put into this!

  1. Perhaps that can be a separate PR where: on login, if dynamic config hasn't been pulled in x hours, pull the latest config

This would also be a good solution!