Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

20+ reaction support #86

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

20+ reaction support #86

wants to merge 16 commits into from

Conversation

MaxKosi
Copy link
Collaborator

@MaxKosi MaxKosi commented May 26, 2021

closes #65 one day

Done:

messageID is a list now
createEmbed returns a list of embeds
reactions are added to the embeds
adding roles
updating the event
signups / reactions work
command functions
dead code parts of messageID
Saving/Loading/snycing
on_raw_reaction_add

To Check:

make linter happy (:

@Gehock
Copy link
Member

Gehock commented May 26, 2021

This is a very promising start. I simplified createAdditionalEmbed a bit, now there's slightly less duplicate code.

I think it would be more beneficial to try to add as many additional roles to the main message as possible. Normally the main message can fit quite a few additional roles before needing extra space, with this current setup adding even a single additional role will result in an extra message. I think the basic idea here is good, but instead of always splitting the Additional group into a separate message, we'd have to count the number of roles/reactions and create a new message only if that number exceeds 20.

Notes about my changes: I commented out some functionality related to the message handling so that I could make the bot display the embeds generated by createAdditionalEmbed. The logic for handling multiple messages and their IDs is currently missing completely, see the TODO notes in my commit.

@MaxKosi
Copy link
Collaborator Author

MaxKosi commented May 27, 2021

This is much better than my start. I didn't really have time to improve the code myself. BTW I might not have too much time until next week, because of another project. So if you want to continue this, feel free to do so. I don't have any local changes atm. I will look at any changes though so I can continue or help if necessary

@MaxKosi
Copy link
Collaborator Author

MaxKosi commented Jun 7, 2021

I re-added the emojis - they dont have a function yet again but theyre added correctly.
You might want to look over it and maybe improve it. I have made quiet a few changes to messageFunctions

And added a TODO for the updateReactions function because I scrapped the more efficient part for now. Needs to be implemented again

BTW I havent got the Linter plugin yet, because it doesnt work yet. Ill fix the errors in the next commit
^ I didnt

@MaxKosi MaxKosi self-assigned this Jun 18, 2021
@MaxKosi
Copy link
Collaborator Author

MaxKosi commented Jul 13, 2021

This pull request needs in-depth testing. I haven't found anything so far. But there might be a few things that I haven't seen.

There are 2 TODOs in messageFunctions.py. The one for syncMessages is obsolete but I forgot to remove the comment. Just saw that. The other one needs improvement but works,

@MaxKosi MaxKosi marked this pull request as ready for review July 13, 2021 14:23
@MaxKosi MaxKosi requested a review from Gehock July 13, 2021 14:23
@Gehock Gehock force-pushed the 20+-reaction-support branch 7 times, most recently from c6b148d to ce5efad Compare September 21, 2021 22:10
@Gehock
Copy link
Member

Gehock commented Sep 21, 2021

This feature is slowly starting to take shape. Creating and sorting multi-message events works now. Some things to still implement:

  • Adding/removing only updated reactions instead of everything. Updating reactions is so slow that we should only touch the ones we have to
  • The !removereaction command. There doesn't seem to be any kind of multi-message support in it yet Replaced with !removerole and !removemainrole
  • Message sorting when adding a new message:
    • when adding a new embed, both messages are created at the bottom and not sorted afterwards
    • instead, should only create one extra message and shuffle everything else forwards

@MaxKosi
Copy link
Collaborator Author

MaxKosi commented Sep 22, 2021

Hey, as you've probably noticed, currently I dont have time to improve this PR. I have to put it on hold until decembre (probably). I would prefer if you would take care of this, if you have time and motivation, since this code has to be checked on every other change made to the master.

@Gehock
Copy link
Member

Gehock commented Sep 22, 2021

Yeah, no worries. There's not really a rush with feature, I've been improving it every now and then when I've had the time.

@Gehock Gehock force-pushed the 20+-reaction-support branch 2 times, most recently from 8fb305e to bc9e541 Compare September 25, 2021 15:22
MaxKosi and others added 11 commits September 28, 2021 20:25
The feature is still very much in progress. This commit isn't meant for
merging as-is, see various TODO markers. Doesn't reorder or synchronise
messages at all yet, just reposts the event message every time
All functions that handle multiple messages have been renamed to use
plural in the function name. Additionally, there is a singular
getEventMessage that's used by some functions, including the plural
getEventMessages
Embeds are quick to update so they're updated first. Then the slow
process of adding reactions begins.
Create removerole, removemainrole, deprecate removereaction
For example, when the 21st reaction gets removed.
@Gehock Gehock force-pushed the 20+-reaction-support branch from bc9e541 to 5feee2f Compare September 28, 2021 18:29
@Gehock Gehock marked this pull request as draft September 30, 2021 20:36
@Gehock Gehock force-pushed the main branch 2 times, most recently from 2dedfd6 to 6a5fb32 Compare April 2, 2024 14:29
@Gehock Gehock force-pushed the main branch 2 times, most recently from 2931dc1 to 2b67941 Compare January 23, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for more than 20 reactions per event
2 participants