Add player whitelist events

[SageSphinx63920]

This pr add events when a player is being removed from the whitelist or added.

[SageSphinx63920]

Thx! I've done some patching before but first time on paper

[SageSphinx63920]

So we do have 2 opinions here: One is getting the player is up to the plugin, which would be useful for plugins that add custom behavior like an own whitelist system and the other saying we should pass an OfflinePlayer because other whitelist related API does it. Which one is the best now?

[emilyy-dev]

One doesn't really dismiss the other, a whitelist plugin might be using their own system but providing the OfflinePlayer/PlayerProfile is still valuable data.

[Leguan16]

i think if its no additional server load providing the OfflinePlayer is a good idea.

[SageSphinx63920]

Is using org.bukkit.Bukkit.getOfflinePlayer(profile.getId()) the way i should do this?

[lynxplay]

Is using org.bukkit.Bukkit.getOfflinePlayer(profile.getId()) the way i should do this?

yes :+1:

[Owen1212055]

I would say to provide the PlayerProfile the default, but then have a utility in the event to provide an OfflinePlayer if needed. That way it will only be retrieved if needed, and later calls will use the cached version.

[SageSphinx63920]

Do you mean the vanilla used player profile or the paper implementation? And should I only make a getter for the profile or also one that converts the profile into an offline player

[Owen1212055]

In the event, pass a paper PlayerProfile, where then there would be a utility method in the event that uses the provided player profile to fetch an OfflinePlayer.

So you'd have both

[SageSphinx63920]

@Owen1212055 done

[SageSphinx63920]

Is this ready to merge then?

[electronicboy]

I think that having target interfaces for events in general might be an idea; I’m also not 100% sure about separate events for add/remove, maybe a single event and an action?

[Leguan16]

maybe a single event and an action?

that's what i initially thought. Don't really see a reason for having 2 separate events. Just means unnecessary code on both sides (paper and plugin). Having a WhitelistChangeEvent with an ADD and REMOVE enum constant would do it imo.

[SageSphinx63920]

I decided to make this use a single event for both updates

[SageSphinx63920]

Isn't the event already called PlayerWhitelistUpdateEvent

[electronicboy]

The enum

[SageSphinx63920]

So i should call it WhitelistStatus?

[electronicboy]

Yes

On Sat, 3 Jun 2023, 21:14 Sage, @.***> wrote:

So i should call it WhitelistStatus?

— Reply to this email directly, view it on GitHub https://github.com/PaperMC/Paper/pull/9209#issuecomment-1575171202, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMAZESYHFK5N7YGS7K5L3XJOLKHANCNFSM6AAAAAAYBCZ3NE . You are receiving this because your review was requested.Message ID: @.***>

[SageSphinx63920]

Any changes i should do?

[SageSphinx63920]

Will this be merged before the 1.20 release of paper?

[Leguan16]

just keep it up to date and it will be merged eventually.