Sign updates #2581
Sign updates #2581
Conversation
…anisms to input a map to modify sign sides
| signSide.lines().clear(); | ||
| for (String s : text) { | ||
| signSide.lines().add(PaperModule.parseFormattedText((s == null ? "" : s), ChatColor.BLACK)); | ||
| } |
There was a problem hiding this comment.
The JavaDocs don't seem to specify the list being a live view (I.e. not just a copy or anything), so probably safer to set these like the other methods do
| @@ -122,6 +139,17 @@ public void sendSignUpdate(Player player, Location loc, String[] text) { | |||
| player.sendSignChange(loc, components); | |||
| } | |||
|
|
|||
| @Override | |||
| public void sendSignUpdate(Player player, Location loc, String[] text, Side side) { | |||
There was a problem hiding this comment.
Is there any reason this has a per-platfrom impl? the only platform specific code is setting the lines onto a Sign, which there's already PaperAPITools#setSignLine for - should be able to have this directly in the mechanism and just use that to set the lines
| if (mechanism.matches("sign_contents")) { | ||
| BlockState state = getBlockState(); | ||
| if (!(state instanceof Sign sign)) { | ||
| mechanism.echoError("'sign_contents' mechanism can only be called on Sign blocks."); | ||
| } else { |
There was a problem hiding this comment.
If you want to update this mechanism should move it to modern tagProcessor#registerMechanism.
Also should use early return for these error checks instead of nested if/else
| // @tags | ||
| // <LocationTag.sign_contents> | ||
| // --> | ||
| if (mechanism.matches("sign_sides_contents") && mechanism.requireObject(MapTag.class)) { |
There was a problem hiding this comment.
New mechanisms should be added using the modern registerMechanism.
Also instead of a map with keys for both sides, might be easier to understand as a mech+tag pair for the front (which already exists) and a mech+tag pair for the back? I.e. something like sign_back_contents
| // Returns the name of the glow-color of the sign at the location. | ||
| // Optionally provide a side (defaults to "front") |
There was a problem hiding this comment.
Same here, probably a separate tag to get the back's glow color, that way it can properly have a matching mechanism as well
| if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_20)) { | ||
| tagProcessor.registerTag(ElementTag.class, "sign_waxed", (attribute, object) -> { | ||
| BlockState state = object.getBlockStateForTag(attribute); | ||
| if (!(state instanceof Sign)) { |
There was a problem hiding this comment.
Should use instanceof pattern matching, that way you can use object.getBlockStateForTag(attribute) directly in the instanceof check instead of storing it separately and casting later
| @@ -3875,14 +3924,30 @@ else if (getPlayerEntity().getGameMode() == GameMode.SPECTATOR) { | |||
| // @name sign_update | |||
| // @input ElementTag | |||
| // @description | |||
| // Shows the player fake lines on a sign, with input in the format of LocationTag|ListTag. | |||
| // Shows the player fake lines on a sign, with input in the format of LocationTag|ListTag | |||
| // or LocationTag|MapTag, with keys front and/or back. | |||
There was a problem hiding this comment.
Using ListTags like this is usually a legacy thing that's replaced by MapTags, so the modern format should be a map with location/front/back keys (could split it up to front/back like other mechanisms, but since this is already a map input anyway, probably fine to keep it all in one mech)
| String[] textArr = text.toArray(new String[4]); | ||
| if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_20)) { | ||
| ElementTag sideElement = scriptEntry.getObjectTag("side"); | ||
| Utilities.setSignLines((Sign) signState, sideElement.asLowerString().toUpperCase(), textArr); |
There was a problem hiding this comment.
You... lowercase it then uppercase it? should just CoreUtilities#toUpperCase
| public static ElementTag sideString(String side) { | ||
| return new ElementTag(Side.valueOf(side)); | ||
| } |
There was a problem hiding this comment.
This method seems redundant? the the output ElementTag will always have the same value as String side - why not just create a new ElementTag as usual?
| public static boolean isSide(String side) { | ||
| return side != null && EnumHelper.get(Side.class).valuesMapLower.containsKey(EnumHelper.cleanKey(side)); | ||
| } |
There was a problem hiding this comment.
EnumHelper is not really meant to be used directly like that - although since this is only ever used for the sign command's input checking and it's just 2 values, I'd probably just manually compare it there/create a copypasta Side enum for input checking.
|
There's also the |
This is a re submission of the PR with the recommended changes
Changed sign related tags now have an optional argument to specify front or back (defaults to front)
added new mechs to take in map input for dealing with either front, back, or both at the same time (these are of the name sign_sides_mechname )
added 2 new mechs for the player for editing front and back sign
changed the sign command:
-added argument of whether the text is going to the front or back
-added hanging signs to the sign command (type:hanging_sign)
-material should recognize all signs now (added mangroves for 1.19 and cherry/bamboo for 1.20)