Conversation
…es and replacing GameProfile instead
WalkthroughVersion bumps for Minecraft/Fabric and dependency versions; input handling migrated to MouseButtonEvent; GameProfile property access switched from getProperties() to properties(); skin storage refactored to use GameProfile properties and PropertyMap; new mixin accessor AAvatar added and APlayer accessor changed; a localization key and mixins list updated. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Screen as SkinChangeScreen
participant Client as ClientTailor
participant Server as Minecraft Server (via Player.level().getServer())
participant Profile as GameProfile
rect rgb(230, 248, 255)
UI->>Screen: MouseButtonEvent (x,y,button)
Screen->>Screen: handle click, determine action
end
alt apply/clear skin
Screen->>Profile: profile.properties() read/modify
Profile-->>Screen: updated PropertyMap / Multimap
Screen->>Client: send network packet to set skin
Client->>Server: server applies profile change / reloads skin
else fetch default
Screen->>Client: request fetchSkinByName(profile.name())
Client->>Server: lookup and respond
end
note over Server,Profile: Skin data now stored/manipulated via GameProfile.properties()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/samo_lego/fabrictailor/network/NetworkHandler.java (1)
35-59: Move skin application back to the server thread.Skin fetch is fine off-thread, but fabrictailor_setSkin()/packet sends must run on the main server thread. Wrap only the mutation/apply block in server.execute to avoid race conditions and rare crashes.
Apply this diff:
THREADPOOL.submit(() -> { Optional<String> value = ((TailoredPlayer) player).fabrictailor_getSkinValue(); Optional<String> signature = ((TailoredPlayer) player).fabrictailor_getSkinSignature(); Optional<Property> skinData = Optional.empty(); if (value.isEmpty() || signature.isEmpty()) { if (!config.defaultSkin.applyToAll) { - skinData = fetchSkinByName(player.getGameProfile().name()); + skinData = fetchSkinByName(player.getGameProfile().name()); } if (skinData.isEmpty()) { var defValue = config.defaultSkin.value; var defSignature = config.defaultSkin.signature; if (!defValue.isEmpty() && !defSignature.isEmpty()) { skinData = Optional.of(new Property(TailoredPlayer.PROPERTY_TEXTURES, defValue, defSignature)); } } - // Try to set skin now - skinData.ifPresent(property -> ((TailoredPlayer) player).fabrictailor_setSkin(property, true)); - ((TailoredPlayer) player).fabrictailor_resetLastSkinChange(); + // Apply on the main server thread + _server.execute(() -> { + skinData.ifPresent(property -> ((TailoredPlayer) player).fabrictailor_setSkin(property, true)); + ((TailoredPlayer) player).fabrictailor_resetLastSkinChange(); + }); } });src/main/java/org/samo_lego/fabrictailor/client/screen/SkinChangeScreen.java (1)
171-177: Likely crash: properties() is immutable in 1.21.10.Calling removeAll(...) on profile.properties() can throw UnsupportedOperationException on non-tailored servers. Rebuild a new GameProfile with an updated PropertyMap via APlayer.setGameProfile(...).
Apply this diff:
- } else { - ((AAbstractClientPlayer) this.minecraft.player).ft_getPlayerInfo().getProfile().properties().removeAll(TailoredPlayer.PROPERTY_TEXTURES); - // Reload skin - todo - } + } else { + var info = ((AAbstractClientPlayer) this.minecraft.player).ft_getPlayerInfo(); + var profile = info.getProfile(); + var props = com.google.common.collect.ArrayListMultimap.<String, Property>create(profile.properties()); + props.removeAll(TailoredPlayer.PROPERTY_TEXTURES); + ((org.samo_lego.fabrictailor.mixin.accessors.APlayer) this.minecraft.player) + .setGameProfile(new com.mojang.authlib.GameProfile(profile.id(), profile.name(), + new com.mojang.authlib.properties.PropertyMap(props))); + // TODO: trigger clientside skin reload if needed + }Add imports:
- com.google.common.collect.ArrayListMultimap
- com.mojang.authlib.GameProfile
- com.mojang.authlib.properties.PropertyMap
- org.samo_lego.fabrictailor.mixin.accessors.APlayer
🧹 Nitpick comments (9)
src/main/java/org/samo_lego/fabrictailor/client/screen/tabs/SkinTabType.java (1)
75-77: Encode with UTF‑8 for symmetry and portability.Decoding uses UTF_8; encoding should match to avoid platform-default discrepancies.
Apply this diff:
- String value = new String(Base64.getEncoder().encode(jsonPayload.toString().getBytes())); + String value = Base64.getEncoder().encodeToString(jsonPayload.toString().getBytes(StandardCharsets.UTF_8));src/main/resources/fabrictailor.mixins.json (1)
5-5: Verify Java compatibility level for 1.21.10.Minecraft 1.21.x runs on Java 21; consider bumping compatibilityLevel to "JAVA_21" to prevent Mixin linkage issues on newer JDKs.
Proposed change:
- "compatibilityLevel": "JAVA_16", + "compatibilityLevel": "JAVA_21",src/main/java/org/samo_lego/fabrictailor/mixin/accessors/AAvatar.java (1)
10-12: Check accessor staticness for static field.If Avatar.DATA_PLAYER_MODE_CUSTOMISATION is static (likely), prefer a static accessor and use it statically at call sites to avoid odd instance binding semantics.
If confirmed static, apply:
- @Accessor("DATA_PLAYER_MODE_CUSTOMISATION") - EntityDataAccessor<Byte> getPLAYER_MODEL_PARTS(); + @Accessor("DATA_PLAYER_MODE_CUSTOMISATION") + static EntityDataAccessor<Byte> getPLAYER_MODEL_PARTS() { + throw new AssertionError("mixin"); + }Then replace usages with AAvatar.getPLAYER_MODEL_PARTS() (see MServerPlayerEntity_TailoredPlayer).
src/main/java/org/samo_lego/fabrictailor/client/screen/SkinChangeScreen.java (2)
189-190: Remove unused variable.map is unused due to the commented block.
Apply this diff:
- PropertyMap map = ((AAbstractClientPlayer) this.minecraft.player).ft_getPlayerInfo().getProfile().properties(); + // no-op: clientside change path left as TODO
86-92: Minor: duplicate tooltip call.The builder.tooltip(...) is invoked twice for openExplorerButton; drop one.
src/main/java/org/samo_lego/fabrictailor/mixin/accessors/APlayer.java (1)
12-15: Confirm accessor annotations.Setter shape is right, but @Final/@mutable are usually applied to the shadowed field, not the accessor method. Verify this compiles under your Mixin version; otherwise use @Accessor("gameProfile") void setGameProfile(GameProfile) without @Final/@mutable here and mark the field @mutable in a dedicated mixin if needed.
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java (3)
208-211: Don’t catch Error.Catching Error swallows serious VM issues (e.g., OOM). Catch Exception instead.
Apply this diff:
- } catch (Error e) { + } catch (Exception e) { // Something went wrong when trying to set the skin Logging.error(e.getMessage()); }
223-225: Avoid iterator().next() with Optional.Use stream().findFirst() to remove reliance on exceptions for control flow.
Apply this diff:
- try { - Property property = this.getGameProfile().properties().get(TailoredPlayer.PROPERTY_TEXTURES).iterator().next(); - this.skinValue = property.value(); - } catch (Exception ignored) { - // Player has no skin data, no worries - } + this.getGameProfile().properties() + .get(TailoredPlayer.PROPERTY_TEXTURES) + .stream() + .findFirst() + .ifPresent(p -> this.skinValue = p.value());And similarly for signature:
- try { - Property property = this.getGameProfile().properties().get(TailoredPlayer.PROPERTY_TEXTURES).iterator().next(); - this.skinSignature = property.signature(); - } catch (Exception ignored) { - // Player has no skin data, no worries - } + this.getGameProfile().properties() + .get(TailoredPlayer.PROPERTY_TEXTURES) + .stream() + .findFirst() + .ifPresent(p -> this.skinSignature = p.signature());Also applies to: 237-239
318-319: Align with AAvatar accessor choice.If AAvatar.getPLAYER_MODEL_PARTS() is made static, update this call accordingly.
Proposed change:
- this.self.getEntityData().set(this.getPLAYER_MODEL_PARTS(), playerModel); + this.self.getEntityData().set(AAvatar.getPLAYER_MODEL_PARTS(), playerModel);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
gradle.properties(1 hunks)src/main/java/org/samo_lego/fabrictailor/client/ClientTailor.java(2 hunks)src/main/java/org/samo_lego/fabrictailor/client/screen/SkinChangeScreen.java(5 hunks)src/main/java/org/samo_lego/fabrictailor/client/screen/tabs/SkinTabType.java(1 hunks)src/main/java/org/samo_lego/fabrictailor/command/SkinCommand.java(1 hunks)src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java(11 hunks)src/main/java/org/samo_lego/fabrictailor/mixin/accessors/AAvatar.java(1 hunks)src/main/java/org/samo_lego/fabrictailor/mixin/accessors/APlayer.java(1 hunks)src/main/java/org/samo_lego/fabrictailor/network/NetworkHandler.java(1 hunks)src/main/resources/assets/fabrictailor/lang/en_us.json(1 hunks)src/main/resources/fabrictailor.mixins.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/resources/fabrictailor.mixins.json (1)
src/main/java/org/samo_lego/fabrictailor/mixin/accessors/ATrackedEntity.java (1)
ATrackedEntity(10-17)
src/main/java/org/samo_lego/fabrictailor/client/ClientTailor.java (2)
src/main/java/org/samo_lego/fabrictailor/FabricTailor.java (1)
FabricTailor(29-73)src/main/java/org/samo_lego/fabrictailor/network/SkinPackets.java (1)
SkinPackets(7-12)
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java (1)
src/main/java/org/samo_lego/fabrictailor/casts/TailoredPlayer.java (3)
TailoredPlayer(11-158)fabrictailor_reloadSkin(16-16)fabrictailor_setSkin(33-33)
src/main/java/org/samo_lego/fabrictailor/client/screen/tabs/SkinTabType.java (1)
src/main/java/org/samo_lego/fabrictailor/casts/TailoredPlayer.java (3)
TailoredPlayer(11-158)fabrictailor_setSkin(33-33)fabrictailor_setSkin(50-50)
src/main/java/org/samo_lego/fabrictailor/mixin/accessors/APlayer.java (1)
src/main/java/org/samo_lego/fabrictailor/casts/TailoredPlayer.java (1)
TailoredPlayer(11-158)
🔇 Additional comments (11)
gradle.properties (1)
4-6: Version upgrade properly coordinated across the Fabric ecosystem.The version bumps are well-aligned: Minecraft 1.21.10 is mirrored in yarn_mappings, and the Fabric API version jump (0.131.0 → 0.135.0) provides support for the breaking API changes referenced in the PR (MouseButtonEvent, KeyMapping.Category, GameProfile.properties() accessors). The mod_version minor bump (2.8.0 → 2.8.1) is appropriate for an API adaptation patch. Carpet core, loader, and permissions API are consistently updated.
Also applies to: 10-10, 13-13, 19-20
src/main/java/org/samo_lego/fabrictailor/command/SkinCommand.java (1)
131-131: LGTM! Correct API migration for Minecraft 1.21.10.The change from
player.getServer()toplayer.level().getServer()properly adapts to the updated Minecraft API. The existing null check on line 132 continues to safeguard against a null server instance, and in the command execution context (where the player is obtained viagetPlayerOrException()), the player's level should always be available.src/main/java/org/samo_lego/fabrictailor/client/ClientTailor.java (2)
13-13: LGTM! Necessary imports for API migration.The imports are correctly added to support the new
KeyMapping.CategoryAPI that requires aResourceLocation.Also applies to: 19-19
45-45: Correct API migration to ResourceLocation-based category.The
KeyMapping.Categoryconstruction properly usesResourceLocation.fromNamespaceAndPath(MOD_ID, "skin_category"), which aligns with the updated localization keykey.category.fabrictailor.skin_categoryin the locale file.src/main/resources/assets/fabrictailor/lang/en_us.json (1)
19-19: LGTM! Localization key updated for new category API.The key rename from
category.fabrictailor.skin_categorytokey.category.fabrictailor.skin_categorycorrectly follows Minecraft's naming convention and aligns with theResourceLocation-based category construction inClientTailor.java.src/main/java/org/samo_lego/fabrictailor/network/NetworkHandler.java (1)
43-44: Accessor migration looks correct.Switch to GameProfile.name() matches 1.21.10 record-style accessors.
src/main/java/org/samo_lego/fabrictailor/client/screen/tabs/SkinTabType.java (1)
48-48: LGTM on properties() migration.Using profile.properties().get(...).stream().findFirst() aligns with the new immutable map API.
src/main/resources/fabrictailor.mixins.json (1)
15-16: New accessor registration looks correct.Adding accessors.AAvatar to common mixins matches the Avatar refactor.
src/main/java/org/samo_lego/fabrictailor/client/screen/SkinChangeScreen.java (2)
368-382: MouseButtonEvent migration looks good.Signature, coordinate reads, and super call are correctly updated.
Also applies to: 20-20
155-158: Default skin payload lookup: OK.Optional handling avoids NoSuchElementException.
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java (1)
176-197: PropertyMap reconstruction approach: good.Copying properties to a mutable Multimap, editing, then rebuilding the GameProfile matches the new immutable properties contract.
Please sanity-check that com.mojang.authlib.properties.PropertyMap has the Multimap constructor in your mapped environment (1.21.10).
Also applies to: 189-197
|
Hi there, thanks for your time! It mostly looks good, I just don't see the reason for the changes in the TailoredPlayer. |
|
Oh, did you mean the changes at import static org.samo_lego.fabrictailor.FabricTailor.config;
- public abstract class MServerPlayerEntity_TailoredPlayer extends Player implements TailoredPlayer {
+ public abstract class MServerPlayerEntity_TailoredPlayer extends Player implements TailoredPlayer, AAvatar, APlayer {
@Unique
private static final String STEVE = "MHF_STEVE";I admit it I took a little bit of personal liberty on changing it from a static access to interface. Too OOP brained. |
|
Yeah I think this should be kept as it was, with static accessor. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java (2)
3-3: Consider using specific imports instead of wildcard.While the wildcard import works, consider importing only the specific types used (
ArrayListMultimap,Multimap) for better code clarity.Apply this diff to use specific imports:
-import com.google.common.collect.*; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.Multimap;
178-189: GameProfile reconstruction pattern is correct but duplicated.The approach of creating a mutable copy of properties, modifying it, and reconstructing the GameProfile is correct for handling the immutable
ImmutableMultimapin Minecraft 1.21.10. However, this pattern is duplicated across lines 178-186, 192-199, and 258-265.Consider extracting this into a helper method to reduce duplication:
@Unique private void updateGameProfileProperties(java.util.function.Consumer<Multimap<String, Property>> modifier) { try { Multimap<String, Property> properties = ArrayListMultimap.create(this.getGameProfile().properties()); modifier.accept(properties); ((APlayer) this).setGameProfile(new GameProfile( this.getGameProfile().id(), this.getGameProfile().name(), new PropertyMap(properties) )); } catch (Exception e) { Logging.debug("Failed to update GameProfile properties: " + e.getMessage()); } }Then simplify the call sites:
// Clearing skin updateGameProfileProperties(props -> props.removeAll(TailoredPlayer.PROPERTY_TEXTURES)); // Setting skin updateGameProfileProperties(props -> { props.removeAll(TailoredPlayer.PROPERTY_TEXTURES); props.put(TailoredPlayer.PROPERTY_TEXTURES, skinData); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java(10 hunks)src/main/java/org/samo_lego/fabrictailor/mixin/accessors/AAvatar.java(1 hunks)
🔇 Additional comments (3)
src/main/java/org/samo_lego/fabrictailor/mixin/accessors/AAvatar.java (1)
1-14: LGTM! Accessor correctly adapted for Minecraft 1.21.10 API changes.The accessor interface correctly provides access to
DATA_PLAYER_MODE_CUSTOMISATIONwhich was moved from thePlayerclass to theAvatarclass in Minecraft 1.21.10. The implementation follows standard Mixin accessor patterns.src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java (2)
54-54: LGTM! Correctly updated for API migration.The static import change from
APlayer.getPLAYER_MODEL_PARTStoAAvatar.getPLAYER_MODEL_PARTSis correct, asDATA_PLAYER_MODE_CUSTOMISATIONwas moved from thePlayerclass to theAvatarclass in Minecraft 1.21.10.
226-226: LGTM! Correctly updated to use record-style accessor.The changes from
.getProperties()to.properties()correctly adapt to the Minecraft 1.21.10 API where property accessors follow the record style.Also applies to: 240-240
src/main/java/org/samo_lego/fabrictailor/mixin/MServerPlayerEntity_TailoredPlayer.java
Show resolved
Hide resolved
|
Hi everybody! Can the merge be done? If not, how can I help? |
|
I posted some minor reviews that were not resolved, then forgot a bit about this 😅 |
|
I'm not sure if replacing the GameProfile is the cleanest sollution. On the other hand I haven't deepdived myself though. |
|
Hi, I apologize for the complete lack of follow up. I have already changed it to a static accessor and use casting instead of messing with the class. As for the GameProfile solution, I saw the solution on a PR for floodgate fabric so I figured it might work as well. I didn't do much digging on other solution. |
|
Sorry for all the wait. I am really gratefull for the update though! |
3 participants
Bump version from 1.21.8 to 1.21.10. Some changes are needed due to breaking changes in the Minecraft API notably
GameProfileno longer being mutable.Relevant API changes
.getProperties()=>.properties()AAvatarmixin due toPlayer.DATA_PLAYER_MODE_CUSTOMISATIONbeing moved toAvatarclassPlayer.GameProfilewith updated textures properties map due toGameProfile.propertiesusingImmutableMultimapSkinChangeScreen.mouseClickedoverride method signature to useMouseButtonEventKeyMapping.Categoryfor registering key bindingManual Testing
Did only small testing locally on Minecraft Java 1.21.10 on Linux with 2 players.
Using GUI skin picker
Summary by CodeRabbit