develop-chatReply #5

Merged
Pupsi merged 11 commits from develop-chatReply into master 2024-10-06 13:52:59 +00:00
2 changed files with 48 additions and 77 deletions
Showing only changes of commit 985b36ddc8 - Show all commits

View File

@ -7,6 +7,8 @@ import eu.mhsl.craftattack.spawn.appliances.chatMessages.ChatMessages;
import eu.mhsl.craftattack.spawn.appliances.privateMessage.commands.PrivateMessageCommand;
import eu.mhsl.craftattack.spawn.appliances.privateMessage.commands.PrivateReplyCommand;
import net.kyori.adventure.text.Component;
import net.kyori.adventure.text.ComponentBuilder;
import net.kyori.adventure.text.TextComponent;
import net.kyori.adventure.text.event.ClickEvent;
import net.kyori.adventure.text.event.HoverEvent;
import net.kyori.adventure.text.format.NamedTextColor;
@ -25,27 +27,29 @@ public class PrivateMessage extends Appliance {
private final Map<Player, List<Conversation>> replyMapping = new WeakHashMap<>();
Pupsi marked this conversation as resolved Outdated

die null checks überall sind nicht schön, es macht vielleicht eher sinn am anfang einmal zu prüfen ob ein Eintrag vorhanden ist und wenn nicht ne leere Liste zu initialisieren..

this.replyMapping.computeIfNotPresent(() -> new ArrayList<>());

die null checks überall sind nicht schön, es macht vielleicht eher sinn am anfang einmal zu prüfen ob ein Eintrag vorhanden ist und wenn nicht ne leere Liste zu initialisieren.. `this.replyMapping.computeIfNotPresent(() -> new ArrayList<>());`
Pupsi marked this conversation as resolved Outdated

hier wird ganz oft this.replyMapping.get(sender) verwendet, rausziehen in eine eigene variable z.B. replyMap = this.replyMapping.get(sender)

hier wird ganz oft `this.replyMapping.get(sender)` verwendet, rausziehen in eine eigene variable z.B. `replyMap = this.replyMapping.get(sender)`
public void reply(Player sender, String message) {
if(this.replyMapping.get(sender) != null) {
this.replyMapping.computeIfAbsent(sender, player -> new ArrayList<>());
List<Conversation> tooOldConversations = this.replyMapping.get(sender).stream()
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (conversationTimeoutMinutes*60*1000))
.toList();
this.replyMapping.get(sender).removeAll(tooOldConversations);
}
if(this.replyMapping.get(sender) == null || this.replyMapping.get(sender).isEmpty()) throw new ApplianceCommand.Error("Du führst aktuell keine Konversation.");
List<Conversation> replyMap = this.replyMapping.get(sender);
MineTec marked this conversation as resolved Outdated

das kann noch über tooOldConversations und dort verwendet werden, auch bei dem removeAll

das kann noch über tooOldConversations und dort verwendet werden, auch bei dem removeAll
if(replyMap.isEmpty()) throw new ApplianceCommand.Error("Du führst aktuell keine Konversation.");
Component privatePrefix = Component.text("[Privat] ", NamedTextColor.LIGHT_PURPLE);
Conversation youngestEntry = this.replyMapping.get(sender).stream()
Conversation youngestEntry = replyMap.stream()
.max(Comparator.comparingLong(o -> o.lastSet))
.orElse(this.replyMapping.get(sender).getLast());
.orElse(replyMap.getLast());
if(message.isBlank()) {
Pupsi marked this conversation as resolved
Review

initialisierung und sofortig bedingtes überschreiben würde ich als bad practice einstufen...

Umschreiben zu

Component currentTargetComponent = currentTargetPlayer != null
    ? Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer)
    : Component.text("niemandem.");
initialisierung und sofortig bedingtes überschreiben würde ich als bad practice einstufen... Umschreiben zu ``` Component currentTargetComponent = currentTargetPlayer != null ? Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer) : Component.text("niemandem."); ```
Component currentTargetComponent = Component.text("niemandem.");
Player currentTargetPlayer = Bukkit.getPlayer(youngestEntry.target());
if(currentTargetPlayer != null) {
currentTargetComponent = Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer);
}
Component currentTargetComponent = currentTargetPlayer != null
? Main.instance().getAppliance(ChatMessages.class).getReportablePlayerName(currentTargetPlayer)
: Component.text("niemandem.");
sender.sendMessage(
privatePrefix
@ -55,84 +59,79 @@ public class PrivateMessage extends Appliance {
return;
}
if(youngestEntry.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000) || this.replyMapping.get(sender).size() == 1) {
List<Conversation> oldConversations = replyMap.stream()
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000))
Pupsi marked this conversation as resolved Outdated

this.sendWhisper

this.sendWhisper
.toList();
if(oldConversations.contains(youngestEntry) || replyMap.size() == 1) {
Player target = Bukkit.getPlayer(youngestEntry.target());
Pupsi marked this conversation as resolved Outdated

das senden einzelner messages ist problematisch...

in der theorie könnten andere messages welche parallel gesendet werden so zwischen die messages rutschen. Außerdem ist jeder sendMessage aufruf ein eigenes paket, welches an den Spieler gesendet wird.

die Aufrufe sollten zusammengezogen werden zu einem einzelnen aufruf, leerzeilen können mit .appendNewLine eingefügt werden.

Hier bietet sich ggf der ComponentBuilder an, dieser kann mit Component.text() erstellt werden.

Anschließend kann darauf mit component.append(otherComponent) gearbeitet werden. Zum schluss dann sender.sendMessage(component.build())

das senden einzelner messages ist problematisch... in der theorie könnten andere messages welche parallel gesendet werden so zwischen die messages rutschen. Außerdem ist jeder sendMessage aufruf ein eigenes paket, welches an den Spieler gesendet wird. die Aufrufe sollten zusammengezogen werden zu einem einzelnen aufruf, leerzeilen können mit `.appendNewLine` eingefügt werden. Hier bietet sich ggf der ComponentBuilder an, dieser kann mit `Component.text()` erstellt werden. Anschließend kann darauf mit `component.append(otherComponent)` gearbeitet werden. Zum schluss dann `sender.sendMessage(component.build())`
if(target == null) throw new ApplianceCommand.Error("Der Spieler " + Bukkit.getOfflinePlayer(youngestEntry.target()).getName() + " ist nicht mehr verfügbar.");
this.replyMapping.get(sender).clear();
sendWhisper(sender, new ResolvedPmUserArguments(target, message));
this.sendWhisper(sender, new ResolvedPmUserArguments(target, message));
return;
}
Pupsi marked this conversation as resolved Outdated

"erhalten" statt "bekommen" klingt denke ich besser

"erhalten" statt "bekommen" klingt denke ich besser
sender.sendMessage("");
sender.sendMessage(
Component.text()
ComponentBuilder<TextComponent, TextComponent.Builder> component = Component.text();
component.append(
Pupsi marked this conversation as resolved Outdated

hier steht die gleiche logik wie in zeile 58

lässt sich das vielleicht schlau kombinieren, dass du oldConversations schon oben erstellst und die andere prüfung auf der bestehenden Liste dann ausführst?

hier steht die gleiche logik wie in zeile 58 lässt sich das vielleicht schlau kombinieren, dass du oldConversations schon oben erstellst und die andere prüfung auf der bestehenden Liste dann ausführst?
Component.newline()
.append(privatePrefix)
.append(Component.text("Das Ziel für /r hat sich bei dir in den letzten ", NamedTextColor.RED))
Pupsi marked this conversation as resolved
Review

gibt es einen Grund warum die Variable hier außerhalb allokiert wird aber nur innerhalb des if statements genutzt wird?

gibt es einen Grund warum die Variable hier außerhalb allokiert wird aber nur innerhalb des if statements genutzt wird?
.append(Component.text(String.valueOf(this.targetChangeTimeoutSeconds), NamedTextColor.RED))
.append(Component.text(" Sekunden geändert. Wer soll deine Nachricht bekommen? ", NamedTextColor.RED))
.append(Component.text(" Sekunden geändert. Wer soll deine Nachricht erhalten? ", NamedTextColor.RED))
.appendNewline()
.appendNewline()
);
List<Conversation> oldConversations = this.replyMapping.get(sender).stream()
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000))
.toList();
Conversation youngestOldConversation;
if(!oldConversations.isEmpty()) {
youngestOldConversation = oldConversations.stream()
Conversation youngestOldConversation = oldConversations.stream()
.max(Comparator.comparingLong(o -> o.lastSet))
.orElse(oldConversations.getLast());
this.replyMapping.get(sender).removeAll(oldConversations);
this.replyMapping.get(sender).add(youngestOldConversation);
replyMap = this.replyMapping.get(sender);
}
Pupsi marked this conversation as resolved Outdated

(ㆆ _ ㆆ)

(ㆆ _ ㆆ)
List<String> playerNames = this.replyMapping.get(sender).stream()
List<String> playerNames = replyMap.stream()
Pupsi marked this conversation as resolved Outdated

wenn du einen weg findest finalComponent nicht überschreiben zu müssen brauchst du den array quark nicht.

ComponentBuilder<TextComponent, TextComponent.Builder> component = Component.text();
                            playerNames.forEach(playerName -> component.append(
                                        Component.text("[")
                                        .append(Component.text(playerName, NamedTextColor.GOLD))
                                        .append(Component.text("]"))
                                        .clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message))
                                        .hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
                                .append(Component.text("  ")));

                            sender.sendMessage("");
                            sender.sendMessage(component.build());
                            sender.sendMessage("");

wenn du einen weg findest finalComponent nicht überschreiben zu müssen brauchst du den array quark nicht. ``` ComponentBuilder<TextComponent, TextComponent.Builder> component = Component.text(); playerNames.forEach(playerName -> component.append( Component.text("[") .append(Component.text(playerName, NamedTextColor.GOLD)) .append(Component.text("]")) .clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message)) .hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD)))) .append(Component.text(" "))); sender.sendMessage(""); sender.sendMessage(component.build()); sender.sendMessage(""); ```
.map(conversation -> Bukkit.getOfflinePlayer(conversation.target()).getName())
.distinct()
.toList();
Pupsi marked this conversation as resolved Outdated

String.format

String.format
final Component[] finalComponent = {Component.text("")};
playerNames.forEach(playerName -> finalComponent[0] = finalComponent[0].append(
playerNames.forEach(playerName -> component.append(
Component.text("[")
.append(Component.text(playerName, NamedTextColor.GOLD))
.append(Component.text("]"))
.clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message))
.clickEvent(ClickEvent.runCommand(String.format("/msg %s %s", playerName, message)))
.hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
Pupsi marked this conversation as resolved Outdated

.appendNewline()

`.appendNewline()`
.append(Component.text(" ")));
.append(Component.text(" "))
);
component.appendNewline();
sender.sendMessage("");
sender.sendMessage(finalComponent[0]);
sender.sendMessage("");
sender.sendMessage(component.build());
Pupsi marked this conversation as resolved Outdated

klammer weg und != verwenden

klammer weg und `!=` verwenden
}
public void sendWhisper(Player sender, ResolvedPmUserArguments userArguments) {
if(!(this.replyMapping.get(userArguments.receiver) == null)) {
List<Conversation> oldEntries = this.replyMapping.get(userArguments.receiver).stream()
.filter(conversation -> conversation.target() == sender.getUniqueId())
.toList();
this.replyMapping.get(userArguments.receiver).removeAll(oldEntries);
}
Conversation newReceiverConversation = new Conversation(
sender.getUniqueId(),
System.currentTimeMillis()
);
if(this.replyMapping.get(userArguments.receiver) == null) {
List<Conversation> receiverConversationList = new ArrayList<>();
receiverConversationList.add(newReceiverConversation);
if(this.replyMapping.get(userArguments.receiver) != null) {
List<Conversation> oldEntries = this.replyMapping.get(userArguments.receiver).stream()
.filter(conversation -> conversation.target() == sender.getUniqueId())
.toList();
Pupsi marked this conversation as resolved Outdated

hier kann durchaus ein else verwendet werden beim if im zusammenhang mit dem vorhergehenden if

hier kann durchaus ein `else` verwendet werden beim if im zusammenhang mit dem vorhergehenden if
this.replyMapping.get(userArguments.receiver).removeAll(oldEntries);
} else {
this.replyMapping.put(
userArguments.receiver,
receiverConversationList
new ArrayList<>()
);
} else {
this.replyMapping.get(userArguments.receiver).add(newReceiverConversation);
}
this.replyMapping.get(userArguments.receiver).add(newReceiverConversation);
List<Conversation> senderConversationList = new ArrayList<>();
senderConversationList.add(
new Conversation(
@ -146,7 +145,6 @@ public class PrivateMessage extends Appliance {
senderConversationList
);
ChatMessages chatMessages = Main.instance().getAppliance(ChatMessages.class);
Component privatePrefix = Component.text("[Privat] ", NamedTextColor.LIGHT_PURPLE);
MineTec marked this conversation as resolved
Review

wird in der reply methode auch aufgerufen, macht ggf sinn die chatMessages referenz als objektvariable zu halten.

wird in der reply methode auch aufgerufen, macht ggf sinn die chatMessages referenz als objektvariable zu halten.

View File

@ -1,27 +0,0 @@
package eu.mhsl.craftattack.spawn.util;
import java.util.ArrayList;
public class LimitedSizedList<T> extends ArrayList<T> {
private final int maxSize;
public LimitedSizedList(int size){
this.maxSize = size;
}
public boolean add(T element){
boolean r = super.add(element);
if (size() > maxSize){
removeRange(0, size() - maxSize);
}
return r;
}
public T getYoungest() {
return get(size() - 1);
}
public T getOldest() {
return get(0);
}
}