Sidebar update - #2795
Conversation
| Debug.report(scriptEntry, getName(), action, elTitle, elScores, elValue, elIncrement, elStart, db("players", players)); | ||
| } | ||
| if (players == null) { | ||
| players = Utilities.entryHasPlayer(scriptEntry) ? Collections.singletonList(Utilities.getEntryPlayer(scriptEntry)) : Collections.emptyList(); |
There was a problem hiding this comment.
List.of methods are cleaner usually
| && !scriptEntry.hasObject("increment") && !scriptEntry.hasObject("start")) { | ||
| throw new InvalidArgumentsException("Must specify at least one of: value(s), title, increment, or start for that action!"); | ||
| if (action == Action.SET && stringValues == null && stringTitle == null) { | ||
| Debug.echoError("Must specify at least one of: value(s), title, increment, or start for that action!"); |
There was a problem hiding this comment.
increment has a default value now and isn't checked here anymore, either remove the default value and add back the check so that it's like before (is -1 a correct default value for this?) or remove it from the error.
| public static BukkitTagContext getContext(PlayerTag player, BukkitTagContext global, boolean perPlayer) { | ||
| if (perPlayer) { | ||
| BukkitTagContext context = new BukkitTagContext(global); | ||
| context.player = player; | ||
| return context; | ||
| } | ||
| return global; | ||
| } |
There was a problem hiding this comment.
This makes a new context every time, but you only really need the global context or a single copy to change the player on for per_player.
| else { | ||
| current.add(new Sidebar.SidebarLine(value.get(i), score)); | ||
| } | ||
| } else { |
|
Marking as draft for now until comments can be addressed |
|
|
| @ArgName("scores") @ArgPrefixed @ArgUnparsed @ArgDefaultNull String stringScores, | ||
| @ArgName("values") @ArgPrefixed @ArgUnparsed @ArgDefaultNull String stringValues, | ||
| @ArgName("start") @ArgPrefixed @ArgUnparsed @ArgDefaultNull String stringStart, | ||
| @ArgName("increment") @ArgPrefixed @ArgUnparsed @ArgDefaultText("-1") String stringIncrement, |
There was a problem hiding this comment.
Increment seems to default to -1 now (instead of nothing previously), and isn't in the initial checks anymore when SET is used.
stringStart is excluded from that check as well actually.
Is that an intentional change on both?
| context = perPlayerCtx; | ||
| } | ||
| Sidebar sidebar = createSidebar(player); | ||
| ListTag values = valuesTag == null ? null : ListTag.getListFor(valuesTag.parse(context), context); |
There was a problem hiding this comment.
Isn't this never null here because of the if check on stringValues at the start?
But more importantly, I think this would make it parse the values input every single time, even when per_player isn't used.
Same thing applies to some other inputs here, unless per_player is used they should only need to be parsed once.
2 participants
First part of updating
sidebarcommand. Related discussion - https://discord.com/channels/315163488085475337/1439931889405395027