Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

akiraveliara
Copy link
Member

@akiraveliara akiraveliara commented Jul 27, 2024

  • removes corelib dependence on AsyncEvent`2 for dispatching events and instead replaces it with a type-agnostic dispatcher assuming immutable events (removing most of the cruft from AsyncEvent`2 is also nice for perf)
  • enables event handlers expressed as types by implementing IEventHandler<TEventArgs>. these can be registered using EventHandlingBuilder.AddEventHandlers<T>(ServiceLifetime); a type can contain an indefinite amount of handlers and the type will have a respected service lifetime according to the parameter.
  • fixes a memory leak in Interactivity
    • interactivity generally is rather janky and remains rather janky here
  • changes every extension setup to be along the lines of IServiceCollection.AddExtension(Action<Extension> setup, ExtensionConfiguration);, with matching methods on DiscordClientBuilder, following an UseExtension scheme according to past convention (open to changing this).
    • the setup parameter fills the need for calling additional configuration on the extension, so for example the following code would be refactored as follows
    • in the future, extensions fully supported throughout the v5 lifecycle should add first-class support without needing a setup parameter
// old
DiscordClientBuilder builder = // we get a client here;
DiscordClient client = builder.Build();

CommandsExtension extension = client.UseCommands(myConfig);
extension.AddCommands([typeof(Command)]);
extension.AddConverters(...);

await client.ConnectAsync();
// new
DiscordClientBuilder builder = // we get a client here;
builder.UseCommands(extension => 
    {
        extension.AddCommands([typeof(MyCommand)]);
        extension.AddConverters(...);
    },
    myConfig
);

await builder.ConnectAsync();

closes #1820 as complete

@akiraveliara akiraveliara added enhancement commands-next For issues related to DSharpPlus.CommandsNext voicenext interactivity core big-change obsolete: lavalink obsolete: slash-commands commands For issues related to DSharpPlus.Commands epic-ioc for pull requests and issues relating to IOC in v5 labels Jul 27, 2024
@akiraveliara akiraveliara added this to the v5.0 milestone Jul 27, 2024
@akiraveliara akiraveliara mentioned this pull request Jul 27, 2024
36 tasks
@akiraveliara akiraveliara marked this pull request as ready for review July 30, 2024 20:44
Copy link
Member

@Naamloos Naamloos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Some discussion points but nothing major. Ping me in the server if you want to discuss any.

Seems ready for testing, at least.

DSharpPlus/Clients/DefaultEventDispatcher.cs Show resolved Hide resolved
DSharpPlus/IEventHandler.cs Show resolved Hide resolved
@akiraveliara
Copy link
Member Author

tested, documented, should be rtm as far as i'm concerned

Copy link
Member

@VelvetToroyashi VelvetToroyashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! Some small things I caught at a glance, but this is definitely a huge step forward.

DSharpPlus.Tests/Commands/CommandFiltering/Tests.cs Outdated Show resolved Hide resolved
DSharpPlus/Clients/EventHandlerCollection.cs Outdated Show resolved Hide resolved
Copy link
Member

@VelvetToroyashi VelvetToroyashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@akiraveliara
Copy link
Member Author

:shipit:

@akiraveliara akiraveliara merged commit e810e2c into master Aug 11, 2024
@akiraveliara akiraveliara deleted the aki/new-events branch August 11, 2024 17:27
OoLunar pushed a commit that referenced this pull request Oct 3, 2024
* implement canonicalization of events and rewire Dispatch

* this is miserable

* remove legacy event hooks

* type safety is an obstacle /s

* CommandsNext

* VoiceNext

* SlashCommands

* lavalink is dead, so just make sure it compiles

* actually remove the old event handling code

* interactivity support

* update tests

* delete legacy BaseExtension

* internal convenience for interactivity

* make the extensions disposable

* shaking my head

* fix the return type

* this was solved differently in master

* update docs

* pr feedback
Instellate pushed a commit that referenced this pull request Nov 4, 2024
* implement canonicalization of events and rewire Dispatch

* this is miserable

* remove legacy event hooks

* type safety is an obstacle /s

* CommandsNext

* VoiceNext

* SlashCommands

* lavalink is dead, so just make sure it compiles

* actually remove the old event handling code

* interactivity support

* update tests

* delete legacy BaseExtension

* internal convenience for interactivity

* make the extensions disposable

* shaking my head

* fix the return type

* this was solved differently in master

* update docs

* pr feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

big-change commands For issues related to DSharpPlus.Commands commands-next For issues related to DSharpPlus.CommandsNext core enhancement epic-ioc for pull requests and issues relating to IOC in v5 interactivity obsolete: slash-commands voicenext

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v5 extensions and events design

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.