From b0c07a9e5212ce38c2237b0b1294c344602d5d6f Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Tue, 28 Apr 2020 00:31:41 +0200 Subject: Insert help channels at the bottom of the category This commit reintroduces bottom sorting for help channels during a category move, but in a more reliable way that also causes far fewer "channel list glitches". This is accomplished by using the "bulk channels update" endpoint of the Discord API. ----------- The Problem ----------- Discord's positioning system is not that easy to work with for developers: Instead of having separate pools of position integers for each category, all text channels are considered to be part of the same "position pool" (or "bucket" in discord.py terms). This also means that changing the position integer of one channel may cause the position integer of another to change, regardless of if the channels share a category or even of if they are close to each other in the guild. As clients receive the position update for each channel as separate CHANNEL UPDATE events, this means that moving one channel may cause other channels to (temporarily) jump around as the client receives the EVENTS from the API. As some position changes affect all the channels in the guild, this will also trigger a nice "channel wave" rolling down the channel list from time to time. For our use case, this was exacerbated by the way `discord.py` handles position changes: It will enumerate the entire, sorted channel list whenever a position change occurs and send a "bulk request" with updated position integers for the entire guild to Discord. This was the reason that all of the sorting methods we've tried resulted in a lot of those "wave" glitches as clients would get a lot of CHANNEL UPDATE events. In addition, the way `discord.py` inserted channels into the payload also meant that our "high integer" methods did not work reliably. ------------ The Solution ------------ Fortunately, there is a solution that will work well most of the time: Making a `bulk channels update` request with only channels of the category we're currently interested in. By providing the current position of the channels that are already in the category, combined with the correct position of the channel moving into the category, we effectively "lock in" the existing channels at the location they already have. The new channel is simply moved into the right position in relation to the existing channels. This means that effectively, we only communicate one channel position change to Discord, making sure that as few channels as possible actually change their formal "position int". From there on, there are two options: 1. Keep the existing channels in place, add the new channel at the bottom (new highest int) 2. Keep the existing channels in place, add the new channel at the top (new lowest int) Both methods work, but option two has a flaw: The position int will get smaller and smaller, until it reaches `0`. Since negative position integers are not allowed, the entire category now has to be shifted upwards to make room for new top channels. This comes at the cost of a "wave" glitch within the category. My initial instinct was to solve this by giving the channels in the category a "really high" straight of position ints, but as Discord recalculates the ints from time to time anyway, this does not work. That's why I opted for the `bottom sort` option, which does not suffer from that issue. I've also asked the question of `top` vs `bottom` in #admins, without the context above, and the preferred method seemed to be `bottom` in any case. ----------- Limitations ----------- While Discord doesn't care that much about duplicates or neatly ascending integers, some channel move actions will inevitably result in a recalculation of the positions ints. This means that "wave" glitches may still happen from time to time, but they should be infrequent. (They also happen if you drag channels in your client; it seems to be a fundamental part of how positioning works.) I think this is something we'll have to live with. Another thing that I suspect may happen is that during times of API lag in the middle of help channel rush hour, some CHANNEL UPDATE events belonging to previous channel moves will not be received/processed yet by the time we make the next move. As we rely on cached position integers, this could mean that from time to time a channel is inserted near the bottom but not at the bottom. As Discord sends these CHANNEL UPDATE replies as individual events in an asynchronous manner instead of as a single response to our `bulk channels update` request, there's nothing much we can do about this. However, I have yet to observe this, so maybe it will never happen. --- bot/cogs/help_channels.py | 57 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index ef58ca9a1..3dea3b013 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -478,6 +478,45 @@ class HelpChannels(Scheduler, commands.Cog): self.schedule_task(channel.id, data) + async def move_to_bottom_position(self, channel: discord.TextChannel, category_id: int, **options) -> None: + """ + Move the `channel` to the bottom position of `category` and edit channel attributes. + + To ensure "stable sorting", we use the `bulk_channel_update` endpoint and provide the current + positions of the other channels in the category as-is. This should make sure that the channel + really ends up at the bottom of the category. + + If `options` are provided, the channel will be edited after the move is completed. This is the + same order of operations that `discord.TextChannel.edit` uses. For information on available + options, see the documention on `discord.TextChannel.edit`. While possible, position-related + options should be avoided, as it may interfere with the category move we perform. + """ + # Get a fresh copy of the category from the bot to avoid the cache mismatch issue we had. + category = await self.try_get_channel(category_id) + + payload = [{"id": c.id, "position": c.position} for c in category.channels] + + # Calculate the bottom position based on the current highest position in the category. If the + # category is currently empty, we simply use the current position of the channel to avoid making + # unnecessary changes to positions in the guild. + bottom_position = payload[-1]["position"] + 1 if payload else channel.position + + payload.append( + { + "id": channel.id, + "position": bottom_position, + "parent_id": category.id, + "lock_permissions": True, + } + ) + + # We use d.py's method to ensure our request is processed by d.py's rate limit manager + await self.bot.http.bulk_channel_update(category.guild.id, payload) + + # Now that the channel is moved, we can edit the other attributes + if options: + await channel.edit(**options) + async def move_to_available(self) -> None: """Make a channel available.""" log.trace("Making a channel available.") @@ -489,10 +528,10 @@ class HelpChannels(Scheduler, commands.Cog): log.trace(f"Moving #{channel} ({channel.id}) to the Available category.") - await channel.edit( + await self.move_to_bottom_position( + channel=channel, + category_id=constants.Categories.help_available, name=f"{AVAILABLE_EMOJI}{NAME_SEPARATOR}{self.get_clean_channel_name(channel)}", - category=self.available_category, - sync_permissions=True, topic=AVAILABLE_TOPIC, ) @@ -506,10 +545,10 @@ class HelpChannels(Scheduler, commands.Cog): """ log.info(f"Moving #{channel} ({channel.id}) to the Dormant category.") - await channel.edit( + await self.move_to_bottom_position( + channel=channel, + category_id=constants.Categories.help_dormant, name=self.get_clean_channel_name(channel), - category=self.dormant_category, - sync_permissions=True, topic=DORMANT_TOPIC, ) @@ -540,10 +579,10 @@ class HelpChannels(Scheduler, commands.Cog): """Make a channel in-use and schedule it to be made dormant.""" log.info(f"Moving #{channel} ({channel.id}) to the In Use category.") - await channel.edit( + await self.move_to_bottom_position( + channel=channel, + category_id=constants.Categories.help_in_use, name=f"{IN_USE_UNANSWERED_EMOJI}{NAME_SEPARATOR}{self.get_clean_channel_name(channel)}", - category=self.in_use_category, - sync_permissions=True, topic=IN_USE_TOPIC, ) -- cgit v1.2.3 From 634dbc93645aebf87d102b1321001f2021def979 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Tue, 28 Apr 2020 01:29:09 +0200 Subject: Add option to ingore channels in help categories As we want to add an "informational" channel to the `Python Help: Available` category, we need to make sure that the Help Channel System ignores that channel. To do that, I've added an `is_excluded_channel` staticmethod that returns `True` if a channel is not a TextChannel or if it's in a special EXCLUDED_CHANNELS constant. This method is then used in the method that yields help channels from a category and in the `on_message` event listener that determines if a channel should be moved from `Available` to `Occupied`. --- bot/cogs/help_channels.py | 13 ++++++++++--- bot/constants.py | 1 + config-default.yml | 3 +++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 3dea3b013..7aeaa2194 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -10,6 +10,7 @@ from datetime import datetime from pathlib import Path import discord +import discord.abc from discord.ext import commands from bot import constants @@ -21,6 +22,7 @@ log = logging.getLogger(__name__) ASKING_GUIDE_URL = "https://pythondiscord.com/pages/asking-good-questions/" MAX_CHANNELS_PER_CATEGORY = 50 +EXCLUDED_CHANNELS = (constants.Channels.how_to_get_help,) AVAILABLE_TOPIC = """ This channel is available. Feel free to ask a question in order to claim this channel! @@ -283,13 +285,18 @@ class HelpChannels(Scheduler, commands.Cog): return name + @staticmethod + def is_excluded_channel(channel: discord.abc.GuildChannel) -> bool: + """Check if a channel should be excluded from the help channel system.""" + return not isinstance(channel, discord.TextChannel) or channel.id in EXCLUDED_CHANNELS + def get_category_channels(self, category: discord.CategoryChannel) -> t.Iterable[discord.TextChannel]: """Yield the text channels of the `category` in an unsorted manner.""" log.trace(f"Getting text channels in the category '{category}' ({category.id}).") # This is faster than using category.channels because the latter sorts them. for channel in self.bot.get_guild(constants.Guild.id).channels: - if channel.category_id == category.id and isinstance(channel, discord.TextChannel): + if channel.category_id == category.id and not self.is_excluded_channel(channel): yield channel @staticmethod @@ -670,8 +677,8 @@ class HelpChannels(Scheduler, commands.Cog): await self.check_for_answer(message) - if not self.is_in_category(channel, constants.Categories.help_available): - return # Ignore messages outside the Available category. + if not self.is_in_category(channel, constants.Categories.help_available) or self.is_excluded_channel(channel): + return # Ignore messages outside the Available category or in excluded channels. log.trace("Waiting for the cog to be ready before processing messages.") await self.ready.wait() diff --git a/bot/constants.py b/bot/constants.py index 49098c9f2..a00b59505 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -383,6 +383,7 @@ class Channels(metaclass=YAMLGetter): dev_log: int esoteric: int helpers: int + how_to_get_help: int message_log: int meta: int mod_alerts: int diff --git a/config-default.yml b/config-default.yml index b0165adf6..78a2ff853 100644 --- a/config-default.yml +++ b/config-default.yml @@ -132,6 +132,9 @@ guild: meta: 429409067623251969 python_discussion: 267624335836053506 + # Python Help: Available + how_to_get_help: 704250143020417084 + # Logs attachment_log: &ATTACH_LOG 649243850006855680 message_log: &MESSAGE_LOG 467752170159079424 -- cgit v1.2.3 From 288ec414f6cc67068a2ed91887bd29d24a82cdcd Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Tue, 28 Apr 2020 01:37:59 +0200 Subject: Log ID of member who claimed a help channel --- bot/cogs/help_channels.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 7aeaa2194..b5cb37015 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -694,6 +694,7 @@ class HelpChannels(Scheduler, commands.Cog): ) return + log.info(f"Channel #{channel} was claimed by `{message.author.id}`.") await self.move_to_in_use(channel) await self.revoke_send_permissions(message.author) # Add user with channel for dormant check. -- cgit v1.2.3