Add SchematicSelector #132
Keine Reviewer
Label
Kein Label
Bug
Codeverbesserung
Einsteiger Freundlich
Idee
In Arbeit
Neues Feature
Prio A
Security Breach
Überprüfung notwendig
Verbesserung
Zu Beobachten
Kein Meilenstein
Niemand zuständig
3 Beteiligte
Fällig am
Kein Fälligkeitsdatum gesetzt.
Abhängigkeiten
Keine Abhängigkeiten gesetzt.
Referenz: SteamWar/SpigotCore#132
Laden…
In neuem Issue referenzieren
Einen Benutzer sperren
Keine Beschreibung angegeben.
Branch "schematic_selector" löschen
Das Löschen eines Branches ist permanent. Obwohl der Branch für eine kurze Zeit weiter existieren könnte, kann diese Aktion in den meisten Fällen NICHT rückgängig gemacht werden. Fortfahren?
Signed-off-by: Chaoscaot chaoscaot444@gmail.com
@ -0,0 +34,4 @@
import java.util.List;
import java.util.function.Consumer;
public class SchematicSelector {
M U L T I L I N G
@ -0,0 +79,4 @@
openList(null, publicMode == PublicMode.PUBLIC_ONLY);
}
private void openList(SchematicNode parent, boolean publics) {
Könntest du evtl die Megamethoden in kleinere Methoden aufteilen, damit man einen besseren Überblick hat? Und ggf. schauen, ob man nicht noch etwas vereinfachen kann.
@ -0,0 +210,4 @@
inv.open();
}
private void openFilter(boolean publics) {
Ich würde evtl. nur einen Filter/eine Suche anbieten, welche dafür sowohl nach passenden Besitzern, Namen, Typen und Items zeitgleich durchsucht. Das dürfte für 99% aller Useranfragen völlig gleichwertig sein und den Usern stets einen Klick ersparen.
@ -0,0 +329,4 @@
return node.getParentNode();
}
public static SelectorTarget selectSchematic() {
Wenn diese Funktionen public sind, sollte auch SelectorTarget public sein
@ -0,0 +362,4 @@
}
@AllArgsConstructor
private enum Target {
Klasse in der Klasse der Klasse? :/
@ -0,0 +364,4 @@
@AllArgsConstructor
private enum Target {
SCHEMATIC("Schematic", false),
DIRECTORY("Ordner", true),
Multiling.
@ -0,0 +366,4 @@
SCHEMATIC("Schematic", false),
DIRECTORY("Ordner", true),
SCHEMATIC_NODE("Schematic/Ordner", true),
SCHEMATIC_TYPE("Schematic", false);
Dopplung von SCHEMATIC?
@ -0,0 +24,4 @@
import de.steamwar.sql.SchematicNode;
import org.bukkit.entity.Player;
public abstract class SchematicSelectorInjectableAdapter implements SchematicSelectorInjectable {
Oh je. Jetzt wird das langsam wirklich BigCorporationJavaClassFactory.
Ok, statt das so zu machen: Mach lieber im SchematicSelectorInjectable jede Funktion mit default void ... Default-Implementierungen, dann brauchst du keine separate abstract Class dafür...
@ -0,0 +33,4 @@
public class UtilGui {
public static void openMaterialSelector(Player player, Consumer<Material> callback) {
openMaterialSelector(player, "Material auswählen", callback);
Multiling?
@ -0,0 +41,4 @@
for(Material material : Material.values()){
if(material.name().startsWith(Material.LEGACY_PREFIX))
continue;
SWItem item = new SWItem(material, "§7" + material.name());
Multling? (Es müsste theoretisch möglich sein, dem Minecraft-Client das Material so zu übergeben, dass der Client das selbst übersetzt)
Du machst viel zu viele Verhaltensunterscheidungen mit boolean-Flags. Ich empfehle dir mal diesen Talk anzusehen: https://www.youtube.com/watch?v=29MAL8pJImQ
@ -0,0 +27,4 @@
SCHEM_SELECTOR_NEW_DIR=§7Neuer Ordner
SCHEM_SELECTOR_FILTER=§7Filter
SCHEM_SELECTOR_CREATE_DIR_TITLE=Ordner Erstellen
erstellen
@ -0,0 +32,4 @@
SCHEM_SELECTOR_FILTER_TITLE=Filter
SCHEM_SELECTOR_FILTER_ENTER_NAME=Name eingeben
SCHEM_SELECTOR_FILTER_NAME=§7Nach Namen suchen...
SCHEM_SELECTOR_FILTER_NAME_SEARCH=§7Suchwort: §e{0}
Typischer wäre Suchbegriff
@ -0,0 +37,4 @@
SCHEM_SELECTOR_FILTER_OWNER=§7Nach Besitzer suchen...
SCHEM_SELECTOR_FILTER_OWNER_SEARCH=§7Besitzer: §e{0}
SCHEM_SELECTOR_FILTER_SEL_TYPE=Typ wählen...
SCHEM_SELECTOR_FILTER_TYPE=§7Nach Typ Filtern...
filtern
@ -0,0 +39,4 @@
SCHEM_SELECTOR_FILTER_SEL_TYPE=Typ wählen...
SCHEM_SELECTOR_FILTER_TYPE=§7Nach Typ Filtern...
SCHEM_SELECTOR_FILTER_TYPE_SEARCH=§7Typ: §e{0}
SCHEM_SELECTOR_FILTER_MAT=§7Nach Item Filtern...
filtern
@ -0,0 +48,4 @@
SCHEM_SELECTOR_DIRECTORY=Ordner
SCHEM_SELECTOR_SCHEMATIC_NODE=Schematic/Ordner
MATERIAL_SELECTOR_TITLE=Material auswählen
Mit Material hat der User eher weniger am Hut. Item?
@ -0,0 +38,4 @@
public class SchematicSelector {
public static final String DEFAULT_TITLE = "SCHEM_SELECTOR_TITLE";
Warum public?
@ -0,0 +46,4 @@
private final SteamwarUser user;
@Getter
private final Consumer<SchematicNode> callback;
@Getter
Getter macht keinen Sinn, keine Klasse außerhalb kann ohne Reflections auf Parameter von SelectorTarget zugreifen.
@ -0,0 +60,4 @@
private boolean singleDirOpen;
@Setter
@Getter
private Function<Player, String> title;
Hast du für solche Sachen nicht eigentlich den SchematicSelectorInjectable?
@ -0,0 +73,4 @@
public SchematicSelector(Player player, SelectorTarget target, SchematicSelectorInjectable injectable, Consumer<SchematicNode> callback) {
this(player, target, callback);
this.useHooks = true;
useHooks ist eine ganz schlechte Variable, weil du dafür, dass du diese Variable hast, jede menge ifs zur Verhaltensänderung brauchst. Wie wäre es damit, immer einen default-SchematicSelectorInjectable zu instanziieren? Dann brauchst du kein useHooks und kannst dir auch die ganzen ifs sparen.
@ -0,0 +84,4 @@
openList(null, publicMode == PublicMode.PUBLIC_ONLY);
}
private void openList(SchematicNode parent, boolean publics) {
Statt überall publics als Parameter durchzutragen: Wie wäre es damit, im Falle einer Publicauflistung den SteamwarUser auf Public (aka id 0) zu setzen? Ein Parameter und viele Fallunterscheidungen weniger, oder?
@ -0,0 +85,4 @@
}
private void openList(SchematicNode parent, boolean publics) {
List<SchematicNode> nodes = filter.isFilter()?getFilteredSchematics(publics):getSchematicList(publics, parent);
Warum hier unterscheiden und nicht vereinfachen, indem immer gefiltert (ggf. mit leerem Filter) wird?
Beim Filtern werden Alle Schematics gefiltert, was es etwas unnötig machen würde für normales anzeigen von Ordnern, alle Schematics zu laden.
@ -0,0 +103,4 @@
list.add(renderItem(node));
}
SWListInv<SchematicNode> inv = new SWListInv<>(player, MessageFormat.format(title==null?DEFAULT_TITLE:title.apply(player), target.target.getName(player), filter.getName().isEmpty()?(parent == null?"/":parent.generateBreadcrumbs(user)):filter.getName()), false, list, (clickType, node) -> handleClick(publics, node, parent));
Statt title==null? evtl. title defaultmäßig = () -> DEFAULT_TITLE; setzen?
@ -0,0 +160,4 @@
name = name.replace(filter.getName(), "§e" + filter.getName() + "§7");
}
SWItem item = new SWItem(m, name, Collections.singletonList(node.isDir() ? ("§9" + Core.MESSAGE.parse("SCHEM_SELECTOR_DIR", player)) : "§7" + node.getSchemtype().name()), !node.isDir() && !node.getSchemtype().writeable(), click -> {
Eigentlich gehören auch ColorCodes in die Messages mit rein... Weil du kannst dir nicht sicher sein, dass die Farbe in einem anderen Kulturraum nicht eine andere Bedeutung hat...
@ -0,0 +369,4 @@
return new SelectorTarget(Target.SCHEMATIC_TYPE, type, rank);
}
public static class SelectorTarget {
Nachdem alles private im Inneren ist, macht das keinerlei Sinn, das public zu stellen
@ -0,0 +375,4 @@
private final SchematicType type;
private final int rank;
private SelectorTarget(Target target, SchematicType type, int rank) {
Hast du hierfür nicht deinen schönen @AllArgsConstructor?
@ -0,0 +391,4 @@
@Getter
private String rawName;
@Getter
Die Klasse ist private. Wozu getter?
@ -0,0 +394,4 @@
@Getter
private boolean dirs;
public String getName(Player player) {
Klasse private, wozu public?
@ -0,0 +406,4 @@
private boolean filter;
private String name = "";
Warum bei name mit "", bei allen anderen Werten aber mit null?
@ -0,0 +422,4 @@
public boolean matches(SchematicNode node) {
boolean matches = true;
if(!name.isEmpty()) {
if(!node.getName().contains(name)) {
Bitte die geschachtelten ifs zusammenfassen.
Bitte noch den Code aus dem Package util in das Package inventory verschieben (denn da gehört der Code eigentlich hin).
Ansonsten schaut der Code jetzt deutlich angenehmer zu lesen aus, ich kann keine Korrektheit garantieren, aber zumindest sollte er jetzt deutlich angenehmer zu warten sein.