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

feature: Decouple channel from messages #1901

Closed

Conversation

SubZero0
Copy link
Member

@SubZero0 SubZero0 commented Jul 30, 2021

With the arrival of Threads, some messages would be ignored as no known channel was found, I thought about two ways to fix this:

  • Add threads to the cache
  • Make possible to process messages without an attached channel

Since the first would make the library even more reliant on the cache, I decided to go with the 2nd, but this also comes with a heavy cost as the assumption of the channel being there is widely used internally as well, so I had to do several changes that might require a lot of changes by who's using the library.

Even now that channels are decoupled, the currently only known channel that could be missing are Threads (other guild channels are always cached and dms are created on the fly with only the user).

This is the first step on implementing threads for discord.net and this change will, at least, let bots process messages sent in those.

I'll leave this PR open for a few days so if anyone is interested, they can review or give feedback.

Possible changes before merging

  • Add a better way to send messages without the channel instead of context.Client.SendMessageAsync(channelId, ...

Changes

Changes done to the interface will also propagate to classes that implement them, obviously.

  • IDiscordClient: added a new method, SendMessageAsync (to send messages without the channel object, required for ReplyAsync)
  • DiscordShardedClient: GetShardFor and GetShardIdFor overloads for ids are now public
  • ICommandContext:
    Added a GuildId property as ulong?
    Changed the Channel property from IMessageChannel to Cacheable<IMessageChannel, ulong>
    IsPrivate now checks for the presence of a guild id instead of channel type
  • IMessage:
    Added a GuildId property as ulong?
    Changed the Channel property from IMessageChannel to Cacheable<IMessageChannel, ulong>
  • ModuleBase: reorderer the parameters so RequestOptions is the last
  • ContextType.Group: removed since bots cant be in groups and it's impossible to know the difference between a dm and a group, I decide to remove it entirely to not need to download the channel
  • RequireBotPermissionAttribute: now can download the channel to retrieve the permissions
  • RequireUserPermissionAttribute: now can download the channel to retrieve the permissions
  • RequireNsfwAttribute: now can download the channel to check for the property
  • RequireContextAttribute: will use the presence of GuildId instead of channel type
  • MessageExtensions.GetJumpUrl: will use the presence of GuildId instead of channel type
  • MessageTypeReader: will ignore channels that aren't cached as their message won't be as well
  • UserTypeReader: won't use the channel if not present

The example for the MessageReceived event was updated to not rely on the channel being present.

This was referenced Jul 30, 2021
@quinchs
Copy link
Member

quinchs commented Nov 23, 2021

In #1923 I've opted for method 1 here. With the later stage of threads being released they we're added to the extended guild payload like channels we're -- so there's no need to worry about them not being cached.

Later down the line I'm going to be mixing in option 2 with cache providers, It essentially decouples entities from relying on each other. Entity properties within other entities become essentially getters for the cache based off of the said models id.

@quinchs quinchs closed this Nov 23, 2021
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.

2 participants