Refactor Channels #1306
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,118 @@ | ||||||||
| package tc.oc.pgm.api.channels; | ||||||||
|
|
||||||||
| import static net.kyori.adventure.text.Component.text; | ||||||||
|
|
||||||||
| import cloud.commandframework.arguments.standard.StringArgument; | ||||||||
| import cloud.commandframework.paper.PaperCommandManager; | ||||||||
| import java.util.Collection; | ||||||||
| import java.util.HashMap; | ||||||||
| import java.util.Map; | ||||||||
| import java.util.function.Predicate; | ||||||||
| import net.kyori.adventure.text.Component; | ||||||||
| import net.kyori.adventure.text.format.NamedTextColor; | ||||||||
| import org.bukkit.command.CommandSender; | ||||||||
| import org.jetbrains.annotations.Nullable; | ||||||||
| import tc.oc.pgm.api.PGM; | ||||||||
| import tc.oc.pgm.api.event.ChannelMessageEvent; | ||||||||
| import tc.oc.pgm.api.player.MatchPlayer; | ||||||||
| import tc.oc.pgm.api.setting.SettingValue; | ||||||||
| import tc.oc.pgm.util.Players; | ||||||||
| import tc.oc.pgm.util.named.NameStyle; | ||||||||
|
|
||||||||
| public interface Channel<T> { | ||||||||
|
There was a problem hiding this comment. This API needs to be fully documented, explaining each method's purpose and params |
||||||||
|
|
||||||||
| default String getDisplayName() { | ||||||||
| return getAliases()[0]; | ||||||||
| } | ||||||||
|
|
||||||||
| String[] getAliases(); | ||||||||
|
|
||||||||
| @Nullable | ||||||||
| default Character getShortcut() { | ||||||||
| return null; | ||||||||
| } | ||||||||
|
|
||||||||
| default SettingValue getSetting() { | ||||||||
| return null; | ||||||||
| } | ||||||||
|
|
||||||||
| default boolean supportsRedirect() { | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| default boolean canSendMessage(MatchPlayer sender) { | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| T getTarget(MatchPlayer sender, Map<String, ?> arguments); | ||||||||
|
|
||||||||
| Collection<MatchPlayer> getViewers(T target); | ||||||||
|
|
||||||||
| default Collection<MatchPlayer> getBroadcastViewers(T target) { | ||||||||
| return getViewers(target); | ||||||||
| } | ||||||||
|
|
||||||||
| default void sendMessage(ChannelMessageEvent<T> event) {} | ||||||||
|
There was a problem hiding this comment.
Suggested change
either force the downstream to implement this, or have an actual default impl, but don't just leave an empty no-op method for something this critical. |
||||||||
|
|
||||||||
| default Component formatMessage(T target, @Nullable MatchPlayer sender, Component message) { | ||||||||
|
There was a problem hiding this comment. i believe this method should not default to global-chat behavior, force downstream to implement it too |
||||||||
| if (sender == null) return message; | ||||||||
| return text() | ||||||||
| .append(text("<", NamedTextColor.WHITE)) | ||||||||
| .append(sender.getName(NameStyle.VERBOSE)) | ||||||||
| .append(text(">: ", NamedTextColor.WHITE)) | ||||||||
| .append(message) | ||||||||
| .build(); | ||||||||
| } | ||||||||
|
|
||||||||
| default void registerCommand(PaperCommandManager<CommandSender> manager) { | ||||||||
| String name = getAliases()[0]; | ||||||||
|
There was a problem hiding this comment. if aliases is an empty array, this throws an out of bounds exception. Throw your own illegal argument exception instead |
||||||||
| String[] aliases = new String[getAliases().length - 1]; | ||||||||
| System.arraycopy(getAliases(), 1, aliases, 0, aliases.length); | ||||||||
|
Comment on lines
+69
to
+70
There was a problem hiding this comment. This needs to go into some util, you could also use
Suggested change
|
||||||||
|
|
||||||||
| manager.command( | ||||||||
| manager | ||||||||
| .commandBuilder(name, aliases) | ||||||||
| .handler( | ||||||||
| context -> { | ||||||||
| MatchPlayer sender = | ||||||||
| context.inject(MatchPlayer.class).orElseThrow(IllegalStateException::new); | ||||||||
| PGM.get().getChannelManager().setChannel(sender, this); | ||||||||
| })); | ||||||||
|
|
||||||||
| manager.command( | ||||||||
| manager | ||||||||
| .commandBuilder(name, aliases) | ||||||||
| .argument( | ||||||||
| StringArgument.<CommandSender>builder("message") | ||||||||
| .greedy() | ||||||||
| .withSuggestionsProvider(Players::suggestPlayers) | ||||||||
| .build()) | ||||||||
| .handler( | ||||||||
| context -> { | ||||||||
| MatchPlayer sender = | ||||||||
| context.inject(MatchPlayer.class).orElseThrow(IllegalStateException::new); | ||||||||
| PGM.get().getChannelManager().process(this, sender, context.asMap()); | ||||||||
| })); | ||||||||
| } | ||||||||
|
|
||||||||
| default Map<String, Object> processChatShortcut(MatchPlayer sender, String message) { | ||||||||
|
There was a problem hiding this comment.
Suggested change
you're not processing the shortcut (with a chat), you're processing the message (with a shortcut). Make sure the docs explain the rationale of why this is even needed cause it's nonobvious |
||||||||
| Map<String, Object> arguments = new HashMap<String, Object>(); | ||||||||
| if (message.length() == 1) { | ||||||||
| PGM.get().getChannelManager().setChannel(sender, this); | ||||||||
| return arguments; | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| arguments.put("message", message.substring(1).trim()); | ||||||||
| return arguments; | ||||||||
|
Comment on lines
+105
to
+106
There was a problem hiding this comment.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| default void broadcastMessage(Component component, T target) { | ||||||||
| broadcastMessage(component, target, player -> true); | ||||||||
| } | ||||||||
|
|
||||||||
| default void broadcastMessage(Component component, T target, Predicate<MatchPlayer> filter) { | ||||||||
| Collection<MatchPlayer> viewers = getBroadcastViewers(target); | ||||||||
| Component finalMessage = formatMessage(target, null, component); | ||||||||
| viewers.stream().filter(filter).forEach(player -> player.sendMessage(finalMessage)); | ||||||||
| } | ||||||||
| } | ||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package tc.oc.pgm.api.event; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import org.bukkit.event.HandlerList; | ||
| import org.jetbrains.annotations.Nullable; | ||
| import tc.oc.pgm.api.channels.Channel; | ||
| import tc.oc.pgm.api.player.MatchPlayer; | ||
| import tc.oc.pgm.util.event.PreemptiveEvent; | ||
|
|
||
| public class ChannelMessageEvent<T> extends PreemptiveEvent { | ||
|
|
||
| private final Channel<T> channel; | ||
| private @Nullable MatchPlayer sender; | ||
| private final T target; | ||
| private final List<MatchPlayer> viewers; | ||
|
There was a problem hiding this comment. you probably should be able to set this, to be able to change who sees the message |
||
| private String message; | ||
|
|
||
| public ChannelMessageEvent( | ||
| Channel<T> channel, | ||
| @Nullable MatchPlayer sender, | ||
| T target, | ||
| Collection<MatchPlayer> viewers, | ||
| String message) { | ||
| this.channel = channel; | ||
| this.sender = sender; | ||
| this.target = target; | ||
| this.viewers = new ArrayList<MatchPlayer>(viewers); | ||
|
There was a problem hiding this comment. i'd avoid the array list, just keep the collection passed in |
||
| this.message = message; | ||
| } | ||
|
|
||
| public Channel<T> getChannel() { | ||
| return channel; | ||
| } | ||
|
|
||
| @Nullable | ||
| public MatchPlayer getSender() { | ||
| return sender; | ||
| } | ||
|
|
||
| public void setSender(@Nullable MatchPlayer sender) { | ||
|
There was a problem hiding this comment. probably no reason to override the sender |
||
| this.sender = sender; | ||
| } | ||
|
|
||
| public T getTarget() { | ||
| return target; | ||
| } | ||
|
|
||
| public List<MatchPlayer> getViewers() { | ||
| return viewers; | ||
| } | ||
|
|
||
| public String getMessage() { | ||
| return message; | ||
| } | ||
|
|
||
| public void setMessage(String message) { | ||
| this.message = message; | ||
| } | ||
|
|
||
| private static final HandlerList handlers = new HandlerList(); | ||
|
|
||
| @Override | ||
| public HandlerList getHandlers() { | ||
| return handlers; | ||
| } | ||
|
|
||
| public static HandlerList getHandlerList() { | ||
| return handlers; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could extend Adventure's
Audience.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the extra overhead that a channel implies, it probably shouldn't.
You don't want someone treating this like it was just another casual audience, when it may not act like one at all (eg: requirement for a target to send within the channel). Effectively speaking the only channel that could properly implement audience would be global chat