Skip to content

Chat events refactor #1125

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Emirlol
Copy link
Collaborator

@Emirlol Emirlol commented Jan 4, 2025

Refactored most usages of ClientReceiveMessageEvents.GAME to use ChatEvents.RECEIVE_TEXT and ChatEvents.RECEIVE_STRING, or the overlay variants (which are also added in this PR) if they required so, with some exceptions because either their logic was complicated due to handling both overlay and chat messages or they already had a cancelled message fix which worked (I presume).

Some usages of ClientReceiveMessageEvents.GAME weren't clear in whether they used overlay messages or not, they didn't do anything with the variable. This refactor also clarifies those by using the relevant event.

As for why the overlay events are separate instead of being similar to fabric's events, it's because there were way too few uses of action bar messages, it's roughly 4 uses in total compared to 25 something chat message listeners. Each chat message listener had an ugly if check that increased the brain power required to understand. Each overlay message listener had the same if check, but reversed. It's just not good for anybody.
This fix also encourages any future dev to separate their overlay listener and chat listener logic because they're almost always different anyway, keeping them in the same method was complicating it.

Haven't tested.


If you're not satisfied with the event names, please lmk because I feel the same way yet I don't know what would be better alternatives to them.

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jan 4, 2025
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Looks fine but I don't think we should strip the formatting by default. I'd rather keep them.


import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class SkyblockXpMessages {
private static final MinecraftClient CLIENT = MinecraftClient.getInstance();
private static final Pattern SKYBLOCK_XP_PATTERN = Pattern.compile("§b\\+\\d+ SkyBlock XP §7\\([^()]+§7\\)§b \\(\\d+\\/\\d+\\)");
private static final Pattern SKYBLOCK_XP_PATTERN = Pattern.compile("\\+\\d+ SkyBlock XP \\([^()]+\\) \\(\\d+/\\d+\\)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we're losing information here by striping the formatting codes. There's a reason hypixel put them in so I think we should keep them too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think functionality should depend on formatting codes, as they might be just artifacts left from an older era and stuff would break once they migrate to text objects with styles.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I do agree with the above, in order to make the pet cache work with autopet it'd require that section symbols are present for parsing the rarity and whatnot so maybe it would be better to have a note saying to not rely on them unless absolutely necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If some features have to use formatting codes, those specific usages could do Text#getString with the RECEIVE_TEXT event and do their own thing anyway.

@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. and removed reviews needed This PR needs reviews labels Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflicts This PR has merge conflicts that need solving.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants