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

Add guide for AsyncChatEvent #264

Merged
merged 6 commits into from
Jan 27, 2024
Merged

Conversation

Leguan16
Copy link
Contributor

@Leguan16 Leguan16 commented Nov 5, 2023

This PR adds a guide for the AsyncChatEvent as people seem to strugle with it. Especially with the renderer.

Copy link
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

This should also mention the use of ChatRenderer.viewerUnaware as something that should be used if the message will not be different depending on who is getting it. This is more performant because the message will only be rendered once per player it's being sent to.

docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
Comment on lines 22 to 28
- The `render` method is called when a chat message is sent to the player.
- The `source` parameter is the player that sent the message.
- The `sourceDisplayName` parameter is the display name of the player that sent the message.
- The `message` parameter is the message that was sent.
- The `viewer` parameter is the player that is receiving the message.

Now that we understand how the renderer works, we can start using it.
Copy link
Member

Choose a reason for hiding this comment

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

I think before this happens, it needs to be made clear that the render method is something that plugins implement, they generally do not call it themselves. And they don't supply each of those arguments, its given to them when the render method is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do that in the next section.

docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
Copy link
Member

@olijeffers0n olijeffers0n left a comment

Choose a reason for hiding this comment

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

Maybe some comments on the fact that it is Asynchronous as well? Looks like a good addition though

docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
@Leguan16
Copy link
Contributor Author

Leguan16 commented Nov 7, 2023

I addressed the requested changes.
Feedback about the placement of the docs/wiki link of LuckPerms and MiniMessage would be appreciated.

docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
@Leguan16
Copy link
Contributor Author

Leguan16 commented Nov 7, 2023

I added a note regarding the event being async, added a tip about the usage of ChatRenderer.ViewerUnaware and addressed kezz's comment about the renderer being instantiated every time the event is called.

I did not yet change the last section.

docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
@Leguan16 Leguan16 changed the title Added guide for the AsyncChatEvent Add guide for AsyncChatEvent Jan 25, 2024
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
docs/paper/dev/api/event-api/chat-event.md Outdated Show resolved Hide resolved
Copy link
Member

@olijeffers0n olijeffers0n left a comment

Choose a reason for hiding this comment

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

image
I think that the styling on some of the links is a bit wonkey, but looks generally good. Thats obviously not your fault

@olijeffers0n olijeffers0n merged commit 3c08de1 into PaperMC:main Jan 27, 2024
1 check passed
@Leguan16 Leguan16 deleted the async-chat-event branch January 27, 2024 17:28
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.

5 participants