EscapeRoomsCommunity / escape_roomba

A Discord bot to bring ease and joy to the Escape Room community discord.
MIT License
1 stars 2 forks source link

General revision, to manage an intro message for thread channels #7

Closed egnor closed 4 years ago

egnor commented 4 years ago

This is now available to demo! Visit Egnor's Omelette (https://discord.gg/ha2tKbq) and it should be running. You can try it out by adding a 🧵 reaction to any message and it should spawn a thread channel.

There's no role/visibility management yet, so everyone can see all thread channels (that's a TODO item). And there's a bunch of other management/usability features that would be nice. (Like archiving for example!) But basic creation (and removal, if you drop the reaction and there's no extra content in the thread channel) should work. It should also update the intro card correctly if you edit the original message. I have no doubt y'all will be able to break stuff though!

Changes in this PR:

Apologies for the size...!

dcsan commented 4 years ago

wow this is great!

Add an intro card to thread channels, and keep it synced with origin messages

you mean you somehow copy a reference to that 'starter' message, not just the text? it didn't seem to update when i edited... ?

what do you think about putting a back-reference to the linked message in the newly created thread?

you could do it much nicer with MD, but i've tested the links work. image

(I guess these are more feature requests, can make new sub-issues)

anaulin commented 4 years ago

Minor notes from playing around in the test server, noting here as possible later work (not in this PR):

Basic unicode handling seems to work well, which is great. Did not test with CJK, tho.

anaulin commented 4 years ago

Another thing from testing:

anaulin commented 4 years ago

I suggest renaming this PR to "kitchen sink, and stuff". Current name is... misleading.

egnor commented 4 years ago

Add an intro card to thread channels, and keep it synced with origin messages you mean you somehow copy a reference to that 'starter' message, not just the text? it didn't seem to update when i edited... ?

It copies the text, but keeps it updated when edits happen. (I've fixed the bug you discovered where it wasn't doing so.)

what do you think about putting a back-reference to the linked message in the newly created thread?

Done!

egnor commented 4 years ago

Minor notes from playing around in the test server, noting here as possible later work (not in this PR):

  • emojis get removed from the thread channel title: is that something that we want? i was a lil' sad about that
  • a message that had only emojis and a blank space between them got turned into a channel named "thinking-face-space", which was surprising; maybe deal with the spaces differently?

Basic unicode handling seems to work well, which is great. Did not test with CJK, tho.

Those emoji-related issues should be fixed now! Thanks for the testing!

egnor commented 4 years ago

Another thing from testing:

  • a message with just an image turned into a channel name that just had the thread and an empty name; maybe in this case you can use a title like "file: " (assuming that the file name is available via the API); or at least some way to indicate that it was just an image (could use the "picture" emoji)

Added an attachment indication in the thread intro (see what you think)! Doesn't copy the whole image but does link to it and shows the filename.

egnor commented 4 years ago

I suggest renaming this PR to "kitchen sink, and stuff". Current name is... misleading.

Renamed :-)

egnor commented 4 years ago

This looks fine. :shipit:

My main observation is that thread_manager.py now contains many many subtle, hard-to-read and (mostly?) untested conditions that might (likely will?) become brittle and hard to maintain. I confess I didn't fully double-check all of the logic, and don't necessarily understand all of it (e.g. why might intro messages shrink?). Assuming that there are no good ways to simplify the logic (which would be ideal), we can work on extracting some of that logic and testing/documenting it more thoroughly. (E.g. the intro message logic can all be in a helper thing that is exhaustively tested.)

I'd love to break things up, but the interface boundaries are a little tricky. I'll think about that in the future. I actually do think there is a pretty solid overall structure (in fact a lot of these changes were cleaning that up) but it's not obvious when you just read through the code. I've added a flow diagram block to try to lay that out.

I added some comments about the intro message shrinkage thing...!

anaulin commented 4 years ago

Thanks for adding all the detailed documentation, I think it will be helpful for future-us to understand how everything hangs together.

Unfortunately it looks like you used some fancy characters that don't seem to render well universally. E.g. in the GitHub code viewer things aren't quite aligned, and in my editor (VSCode 1.49.2) they don't even render: Screen Shot 2020-09-28 at 1 37 45 PM

anaulin commented 4 years ago

Correction, fancy symbols also don't show for me in the GitHub diff, using Chrome on Mac OS X: Screen Shot 2020-09-28 at 1 42 56 PM

egnor commented 4 years ago

Misaligned overly-fancy comments: Yeah, I see that. I'll fix it up in a subsequent change.

Archiving channels: Discord doesn't have a concept of "archived" channels per se (the way Slack does). We can always move thread channels to an "Archived Threads" category, but that category can get unwieldy, and the limits of 50 channels/category and 500 channels/server still apply.

The thread deletion that's here is expected to be kind of a corner case, just so that you can quick-undo thread creation when you didn't really want to make one or put it on the wrong message or something (intuitive UI), not something anyone would use after people have started actually using the thread. It won't delete if there's messages in it.

Longer term, my thinking is to move threads to an "Idle Threads" category after a few days of inactivity, and then after a longer period of inactivity archive the thread more permanently by dumping its contents into a single "Archived Threads" channel and deleting the original thread.