Rework Discord Webhooks with pictures #239
Conversation
## Create Multiple Webhooks, one for each color This is so that we don't have to edit Webhooks all the time and thus not run into a rate-limit (as often). ## Don't let discordgo retry on rate limit Instead just send the message through the compact-mode
|
I still want to clean-up the code a bit before it is ready to merge but i would love some feedback. |
|
Some remarks:
|
|
Merging the Webhook list and image cache would prob be possible but tedious and not worth it imo. The resulting list would need to account for possible many to one entries as e.g. two users using The webhooks should also be kept in a list that can be recovered from discord so that we don't need to delete all webhooks on startup. |
|
The code is hard to read. Needs some refactoring. Also the |
| func (i *imgCache) add(user, image string) { | ||
| if len(*i) >= cacheSize { | ||
| // remove the first value | ||
| for k := range *i { | ||
| delete(*i, k) | ||
| break | ||
| } | ||
| } | ||
| (*i)[user] = image | ||
| } |
There was a problem hiding this comment.
I don't think you need to delete the previous value in a map. You can set it directly.
There was a problem hiding this comment.
i am trying to keep the map from getting to large, as the images themselves are stored in the map and users can easily change color or name and thus create extra entries in the cache. For that reason i believe there should be a limit as to not allow the ram usage to increase that much.
There was a problem hiding this comment.
Having the cache linked to a list of online members would be better but would also mean deeper changes into devzat's code that is not related to the discord bridge.
There was a problem hiding this comment.
ah nvm I see what the code does. we should make this an LRU cache.
|
Creating an image is very resource intensive. So eg. we cannot be calculating the hash of the avatar every message. |
There is a cache for those images, so it is just a look up into a map after the user has sent the first message |
| users := make([]string, 0, maxWebhookCount) | ||
| for _, room := range Rooms { | ||
| for _, user := range room.users { | ||
| users = append(users, user.Name) | ||
| } | ||
| } | ||
| users = append(users, "", Devbot) |
There was a problem hiding this comment.
Doesn't feel right that this list is generated every message. Can we instead just update this list whenever users are added or removed?
There was a problem hiding this comment.
Also, what happens when plugins send messages?
There was a problem hiding this comment.
We could have one webhook that handles all miscellaneous situations
There was a problem hiding this comment.
Doesn't feel right that this list is generated every message. Can we instead just update this list whenever users are added or removed?
This would need deeper changes outside of the discord bridge... i can try adding this in an expandable style or just for discord 🤷♀️
There was a problem hiding this comment.
We could have one webhook that handles all miscellaneous situations
i could use the empy name webhook that is used for system commands, or add a difference between the max number of webhooks and the max number of users to use webhooks for so that webhooks are created for plugins, they might just get deleted after a user changes color or another plugin/the same plugin sends a message with different colors...
Create Multiple Webhooks, one for each color
This is so that we don't have to edit Webhooks all the time and thus not run into a rate-limit (as often).
Don't let discordgo retry on rate limit
Instead just send the message through the compact-mode