develop-chatReply #5
@ -19,17 +19,52 @@ import java.util.stream.Collectors;
|
||||
|
||||
public class PrivateMessage extends Appliance {
|
||||
public final int targetChangeTimeoutSeconds = 30;
|
||||
public final int conversationTimeoutMinutes = 30;
|
||||
|
||||
private record Conversation(UUID target, Long lastSet) {}
|
||||
private final Map<Player, List<Conversation>> replyMapping = new WeakHashMap<>();
|
||||
|
||||
public void reply(Player sender, String message) {
|
||||
|
Pupsi marked this conversation as resolved
Outdated
|
||||
if(this.replyMapping.get(sender) == null || this.replyMapping.get(sender).isEmpty()) throw new ApplianceCommand.Error("Du kannst nicht auf eine Konversation antworten, da keine vorhanden ist.");
|
||||
if(this.replyMapping.get(sender) != null) {
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
hier wird ganz oft hier wird ganz oft `this.replyMapping.get(sender)` verwendet, rausziehen in eine eigene variable z.B. `replyMap = this.replyMapping.get(sender)`
|
||||
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.");
|
||||
|
||||
|
||||
|
MineTec marked this conversation as resolved
Outdated
MineTec
commented
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
|
||||
Component privatePrefix = Component.text("[Privat] ", NamedTextColor.LIGHT_PURPLE);
|
||||
ChatMessages chatMessages = Main.instance().getAppliance(ChatMessages.class);
|
||||
|
||||
Conversation youngestEntry = this.replyMapping.get(sender).stream()
|
||||
.max(Comparator.comparingLong(o -> o.lastSet))
|
||||
.orElse(this.replyMapping.get(sender).getLast());
|
||||
|
||||
String youngestTargetName;
|
||||
if(Bukkit.getPlayer(youngestEntry.target()) == null) {
|
||||
youngestTargetName = Bukkit.getOfflinePlayer(youngestEntry.target()).getName();
|
||||
|
Pupsi marked this conversation as resolved
MineTec
commented
initialisierung und sofortig bedingtes überschreiben würde ich als bad practice einstufen... Umschreiben zu 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.");
```
|
||||
} else {
|
||||
youngestTargetName = Objects.requireNonNull(Bukkit.getPlayer(youngestEntry.target())).getName();
|
||||
}
|
||||
|
||||
|
||||
if(message.isBlank() && youngestTargetName != null) {
|
||||
sender.sendMessage(
|
||||
privatePrefix
|
||||
.append(Component.text("Du schreibst aktuell mit ", NamedTextColor.GRAY))
|
||||
.append(chatMessages.getReportablePlayerName(sender))
|
||||
);
|
||||
return;
|
||||
} else if(message.isBlank()) {
|
||||
sender.sendMessage(
|
||||
privatePrefix
|
||||
.append(Component.text("Du schreibst aktuell mit niemandem.", NamedTextColor.GRAY))
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
this.sendWhisper this.sendWhisper
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
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 Hier bietet sich ggf der ComponentBuilder an, dieser kann mit Anschließend kann darauf mit 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(youngestEntry.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000) || this.replyMapping.get(sender).size() == 1) {
|
||||
Player target = Bukkit.getPlayer(youngestEntry.target());
|
||||
if(target == null) throw new ApplianceCommand.Error("Der Spieler " + Bukkit.getOfflinePlayer(youngestEntry.target()).getName() + " ist nicht mehr verfügbar.");
|
||||
@ -42,7 +77,14 @@ public class PrivateMessage extends Appliance {
|
||||
List<Conversation> oldConversations = this.replyMapping.get(sender).stream()
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
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?
|
||||
.filter(conversation -> conversation.lastSet < System.currentTimeMillis() - (targetChangeTimeoutSeconds*1000))
|
||||
.toList();
|
||||
this.replyMapping.get(sender).removeAll(oldConversations);
|
||||
|
||||
|
Pupsi marked this conversation as resolved
MineTec
commented
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?
|
||||
Conversation youngestOldConversation = null;
|
||||
if(!oldConversations.isEmpty()) {
|
||||
youngestOldConversation = oldConversations.stream()
|
||||
.max(Comparator.comparingLong(o -> o.lastSet))
|
||||
.orElse(oldConversations.getLast());
|
||||
this.replyMapping.get(sender).removeAll(oldConversations);
|
||||
}
|
||||
|
||||
List<String> playerNames = this.replyMapping.get(sender).stream()
|
||||
.map(conversation -> {
|
||||
@ -55,34 +97,41 @@ public class PrivateMessage extends Appliance {
|
||||
.distinct()
|
||||
.toList();
|
||||
|
||||
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
String.format String.format
|
||||
sender.sendMessage("");
|
||||
sender.sendMessage(
|
||||
Component.text()
|
||||
.append(Component.text("Das Ziel für /r hat sich bei dir in den letzten "))
|
||||
.append(Component.text(String.valueOf(this.targetChangeTimeoutSeconds)))
|
||||
.append(Component.text(" Sekunden geändert. Wer soll deine Nachricht bekommen? "))
|
||||
.append(privatePrefix)
|
||||
.append(Component.text("Das Ziel für /r hat sich bei dir in den letzten ", NamedTextColor.RED))
|
||||
.append(Component.text(String.valueOf(this.targetChangeTimeoutSeconds), NamedTextColor.RED))
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
`.appendNewline()`
|
||||
.append(Component.text(" Sekunden geändert. Wer soll deine Nachricht bekommen? ", NamedTextColor.RED))
|
||||
);
|
||||
|
||||
|
||||
final Component[] finalComponent = {Component.text("")};
|
||||
|
||||
String firstTargetName;
|
||||
if(Bukkit.getPlayer(youngestEntry.target()) == null) {
|
||||
firstTargetName = Bukkit.getOfflinePlayer(youngestEntry.target()).getName();
|
||||
} else {
|
||||
firstTargetName = Objects.requireNonNull(Bukkit.getPlayer(youngestEntry.target())).getName();
|
||||
String firstTargetName = null;
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
klammer weg und klammer weg und `!=` verwenden
|
||||
if(youngestOldConversation != null && Bukkit.getPlayer(youngestOldConversation.target()) == null) {
|
||||
firstTargetName = Bukkit.getOfflinePlayer(youngestOldConversation.target()).getName();
|
||||
} else if(youngestOldConversation != null){
|
||||
firstTargetName = Objects.requireNonNull(Bukkit.getPlayer(youngestOldConversation.target())).getName();
|
||||
}
|
||||
|
||||
if(firstTargetName != null && !playerNames.contains(firstTargetName)) {
|
||||
finalComponent[0] = finalComponent[0].append(Component.text(firstTargetName, NamedTextColor.GOLD)
|
||||
.clickEvent(ClickEvent.runCommand("/msg " + firstTargetName + " " + message))
|
||||
.hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
|
||||
finalComponent[0] = finalComponent[0].append(
|
||||
Component.text("[")
|
||||
.append(Component.text(firstTargetName, NamedTextColor.GOLD))
|
||||
.append(Component.text("]"))
|
||||
.clickEvent(ClickEvent.runCommand("/msg " + firstTargetName + " " + message))
|
||||
|
Pupsi marked this conversation as resolved
Outdated
MineTec
commented
hier kann durchaus ein hier kann durchaus ein `else` verwendet werden beim if im zusammenhang mit dem vorhergehenden if
|
||||
.hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
|
||||
.append(Component.text(" "));
|
||||
}
|
||||
|
||||
playerNames.forEach(playerName -> finalComponent[0] = finalComponent[0].append(Component.text(playerName, NamedTextColor.GOLD)
|
||||
.clickEvent(ClickEvent.runCommand("/msg " + playerName + " " + message))
|
||||
.hoverEvent(HoverEvent.showText(Component.text("Klicke, um diesem Spieler zu schreiben.").color(NamedTextColor.GOLD))))
|
||||
playerNames.forEach(playerName -> finalComponent[0] = finalComponent[0].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("");
|
||||
@ -93,7 +142,6 @@ public class PrivateMessage extends Appliance {
|
||||
|
||||
public void sendWhisper(Player sender, ResolvedPmUserArguments userArguments) {
|
||||
|
||||
// Ältere Einträge bei replyMapping für den receiver entfernen
|
||||
if(!(this.replyMapping.get(userArguments.receiver) == null)) {
|
||||
List<Conversation> oldEntries = this.replyMapping.get(userArguments.receiver).stream()
|
||||
.filter(conversation -> conversation.target() == sender.getUniqueId())
|
||||
@ -101,7 +149,6 @@ public class PrivateMessage extends Appliance {
|
||||
this.replyMapping.get(userArguments.receiver).removeAll(oldEntries);
|
||||
}
|
||||
|
MineTec marked this conversation as resolved
MineTec
commented
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.
|
||||
|
||||
// Neuen Eintrag bei replyMapping für den receiver
|
||||
Conversation newReceiverConversation = new Conversation(
|
||||
sender.getUniqueId(),
|
||||
System.currentTimeMillis()
|
||||
@ -119,7 +166,6 @@ public class PrivateMessage extends Appliance {
|
||||
this.replyMapping.get(userArguments.receiver).add(newReceiverConversation);
|
||||
}
|
||||
|
||||
// Einträge für sender bei replyMapping überschreiben mit receiver
|
||||
List<Conversation> senderConversationList = new ArrayList<>();
|
||||
senderConversationList.add(
|
||||
new Conversation(
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user
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<>());