Converts ExprColorOf into a Type Property - #8852
Conversation
skriptlang-automation
Bot
added
needs reviews
This comment has been minimized.
This comment has been minimized.
skriptlang-automation
Bot
removed
the
needs reviews
skriptlang-automation
Bot
added
the
needs reviews
|
|
skriptlang-automation
Bot
added
enhancement
…work effect colors
…eat/new-type-properties
Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>
…eat/new-type-properties
skriptlang-automation
Bot
removed
the
needs reviews
…/expressions/PropExprColor.java Co-authored-by: SirSmurfy2 <82696841+Absolutionism@users.noreply.github.com>
…ing and removed deprecated code
…tClassInfo.java Co-authored-by: SirSmurfy2 <82696841+Absolutionism@users.noreply.github.com>
skriptlang-automation
Bot
added
the
needs reviews
…eat/new-type-properties
|
If you have some time I can also work on passing some other properties with the new system; those includes: (those may appear a bit random but I'm facing several conflicts with their pattenrs while working on an addon) |
| test "color of firework effects": | ||
| set {_effect} to ball firework effect coloured red and blue | ||
| set {_colors::*} to colours of {_effect} | ||
| # firework colours are reported as raw rgb, which does not match the dye colour they were built from | ||
| assert amount of {_colors::*} is 2 with "firework effect should have 2 colours" | ||
| assert {_colors::1} is set with "first firework effect colour failed" | ||
| assert {_colors::2} is set with "second firework effect colour failed" | ||
| assert {_colors::1} is not {_colors::2} with "firework effect colours should differ" |
There was a problem hiding this comment.
I can't write a passing changer test here: FireworkEffect is immutable. getColors() returns this.colors, declared private final ImmutableList, so the add/remove/clear calls this handler inherits from ExprColorOf all throw UnsupportedOperationException at runtime. That's pre-existing, ExprColorOf has always been broken here, and I kept the logic as-is for parity. I can't change the value of the changing expression itself either, what should I do?
There was a problem hiding this comment.
So why does it still accept changes? remove that
| if (state instanceof Colorable colorable) { | ||
| try { | ||
| colorable.setColor(dyeColor); | ||
| } catch (UnsupportedOperationException ex) { | ||
| // https://github.com/SkriptLang/Skript/issues/2931 | ||
| Skript.error("Tried setting the color of a bed, but this isn't possible in your Minecraft version, " + | ||
| "since different colored beds are different materials. " + | ||
| "Instead, set the block to right material, such as a blue bed."); | ||
| } |
There was a problem hiding this comment.
likewise I don't think this functions anymore either.
There was a problem hiding this comment.
Kinda; On 26.2, Bed is deprecated (since = "26.2", forRemoval = true) with "bed block entity no longer exists", so getState() never returns one and the catch is unreachable there. But it still implements Colorable and its setColor is @contract("_ -> fail"), and Skript still supports 1.21.x where the bed block entity does exist, so I'd keep the catch until the minimum version rises. What do you think?
Also the Colorable branch itself is definitely still live though: Sign implements it, and that's the sign text dye colour. While checking I found the ported code never called state.update(), so setting a sign's colour silently did nothing. Fixed, and I replaced the # test "color of blocks" TODO with a real sign test covering read and write. Is it good?
There was a problem hiding this comment.
Ok but does changing the color of a bed actually work on 1.21.x
skriptlang-automation
Bot
removed
the
needs reviews
…seSyntax.java Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Returning an empty class array does not accept any type, it accepts no types while still allowing the change, as opposed to returning null. Name the flag after that rather than after 'any type'.
Callers should use the three-argument variant so the owning syntax can pass itself as the parent expression, which handlers need to emit runtime errors.
…pertype getPossiblePropertyInfos only put the nearest class info at or above each of the expression's declared return types into the property map, so a value whose runtime type had a more specific handler never reached it. Collect the class infos below the declared type as well; PropertyMap already picks the most specific assignable key at runtime. This lets 'color of event-entity' find the display handler on its own, so the delegation the entity handler used as a workaround is no longer needed.
The block color handler mutated the block state but never called update(), so setting the color of a colorable block, a sign for instance, silently did nothing. Replace the block color test TODO with a sign test covering both reading and writing.
Adds the OWNER, LOCATION, AUTHOR and CONTENT properties, each with a PropExpr syntax gated behind 'use type properties', and keeps the expression it replaces registered and deprecated when the option is off. - owner: handled by the entity type, ExprEntityOwner gated - location: handled by the location type as an identity, so anything with a converter to a location keeps working; ExprLocationOf gated - author: handled by the item type, ExprBookAuthor gated - content: handled by the item type, only the "pages of" patterns of ExprBookPages are gated since the property cannot express a page index PropExprContent overrides isSingle because PropertyBaseExpression only looks at the source expression, while the pages of a book are multi valued. getPages, setPages and the sign-if-needed logic move out of ExprBookPages into BookUtils so the item type handler does not depend on a deprecated expression.
…eat/new-type-properties
skriptlang-automation
Bot
added
the
needs reviews
5 participants
Problem
ExprColorOfis still a classicPropertyExpressionwith a seven branchinstanceofchain overBlock,ItemType,Entity,FireworkEffect,PotionEffectType,DisplayandBossBar.The logic for each type lives in the expression instead of with the type, and addons cannot hook their own types into
color ofwithout making tricky things or conflicts.While converting it, i've faced two bugs in the property framework; Both block this
property from working at all, and both affect every existing type property. I tried to fix them on my own, please tell me what you think of it:
PropertyBaseExpression#acceptChangereturnsnew Class[0]from inside its loop onRESET/DELETE. Handlers after the first accepting one never get their types stored inchangeDetails, sochange()silently skips them. With seven types in an unorderedHashMap,reset color of {_display}would fail depending on iteration order.PropertyBaseSyntax#getPossiblePropertyInfospasses the source expression toPropertyHandler#init, although the javadoc says "the expression that is using this handler". Source expressions (Variable,ConvertedExpression) do not implementRuntimeErrorProducer, so a handler can never emit a runtime error. That breaks the bossbar "Could not round to a color that is similar to ..." error asserted byExprSecCreateBossBar.sk(when I ran the tests after my changes)ExprColorOfalso has some "bugs" of its own: an&&/||precedence mistake that makesacceptChangeaccept every change mode for bossbars whilechangehandles only SET/RESET,delta[0]dereferenced without a null or length guard, and a firework read path that only fires when the expression is statically typed as a firework effect (not sur eit that was intentional tho).Solution
Follows the
ExprName/PropExprName(and other new type properties') pattern:New system: I added
Property.COLORandPropExprColor(+ its registration), and registered properties of that type property in the related class info (BlockClassInfo,ItemTypeClassInfo, ...)Old system:
ExprColorOfgets@Deprecated(forRemoval = true)and an early return inregister(SyntaxRegistry)whenSkriptConfig.useTypePropertiesis on. No other line of it changes, so the two latent bugs stay put there and are fixed in the new handlers only.Note
Display extends EntityandPropertyMapresolves handlers from the declared types of an expression. The old code did a runtimeinstanceof Displaycheck regardless of the declared type, socolor of event-entityworked on a text display. Without delegation that silently regresses, soEntityColorHandlerforwards toDisplayColorHandlerwhen the entity is a display. This is whyDisplayColorHandleris a top level class instead of nested inDisplayModule. Maybe there's another/better way to do this?Testing Completed
src/test/skript/tests/syntaxes/expressions/ExprColorOf.skextended. It previously only covered displays, with the rest commented out as TODO:color of entities: sheep read and set, covers theColorableentity path.color of firework effects: read of a multi valued property. The equality assertion is against presence and difference rather thanred/blue, because firework colors come back as raw rgb (rgb 179, 49, 44) which does not match the dye color they were built from.color of displays through an entity expression: locks in the Entity to Display delegation.Supporting Information
Behaviour differences (which are all improvements):
source instanceof FireworkEffect[], which only matches a statically typed expression, socolor of {_effect}on a variable returned nothing.Questions to sharpen this PR
getPossiblePropertyInfos(Property, Expression), to avoid breakingPropCondContainsandPropertyBaseCondition, which have no parent expression. Would you rather widenPropertyHandler#initto take aSyntaxElementso conditions get the same capability?bukkit.entity.displaysthe right home forDisplayColorHandler, or should shared handlers live somewhere more central?color of event-entityon displays worth the Entity to Display coupling, or would you rather accept the narrowing and require adisplaytyped expression?ExprColorOf's values, so it reads{"1.2", "2.10 (displays)", "2.16 (boss bars)"}, on the grounds that the property documents a capability that has existed since 1.2. The alternative is a single concrete version such as2.17, since the property itself is new. Which convention do you want for converted syntaxes?@Deprecated(since = ...)onExprColorOfis left as"INSERT VERSION". Should it be the next release number, or is the placeholder the expected convention?Completes: none
Related: none
AI assistance: Inline GitHub Copilot + NES (Next Edit Suggestion) from IntelliJ IDEA ; Claude Code for helping structure the PR description.