Allow URL argument in setSkullTexture() #213
Conversation
| String json = Base64.getDecoder().decode(texture); | ||
| if(!json.contains("\"url\":\"")){ | ||
| // Decoded json does not contain a skin URL, so check if the 'texture' argument is a URL. | ||
| if(texture.startsWith("http://")); // We are all set. |
There was a problem hiding this comment.
That is problematic as it ignores the possibility of a https:// url. So either also check if it starts with https:// or maybe just check if it starts with http (Tho that could be a bit to vague)
There was a problem hiding this comment.
Fair point, but Minecraft's skin server only uses http://. They could change in the future, but currently the game won't render skins that aren't hosted under Minecraft's website, so I didn't bother for https:// (since no existing valid skin will have it).
The old HD skins (super-high resolution heads, 640x640 etc) from the Education Edition website used https://, but when Minecraft removed that they did so on their server end, so it applies to ALL versions of Minecraft
To be flexible in case they start re-allowing that in the future, could do "http" as you suggest. It's a bit more vague, but yeah likely not an issue
There was a problem hiding this comment.
What I also find a bit problematic is, how this setup could allow any URL to work as a skin. While I doubt it will work, do I feel like it's not a good idea to leave it up to the server to do the cleanup here...
It just feels... odd.
There was a problem hiding this comment.
Fair enough, I'll update the check to be more thorough (by checking for the full Minecraft skin server URL)
(Note that the same criticism could be used against the existing Base64 codepath)
src/main/java/eu/decentsoftware/holograms/api/utils/items/SkullUtils.java
Outdated
Show resolved
Hide resolved
| catch(IllegalArgumentException ex){ | ||
| json = "invalid"; | ||
| } | ||
| if(!json.contains("\"url\":\"http://textures.minecraft.net/texture/")){ |
There was a problem hiding this comment.
Put this if inside the try block above. There is no need to put it outside, it just makes it more complicated. If you put it in the try block, you can avoid re-assigning the json variable to "invalid".
|
|
||
| PropertyMap properties = profile.getProperties(); | ||
| properties.put("textures", property); | ||
| String json; |
There was a problem hiding this comment.
See comment below; you can also move this in the try block.
| if(texture.startsWith("http://textures.minecraft.net/texture/")); // We are all set. | ||
| // Check if the texture is a hexidecimal (like the skinfiles on Minecraft's servers) | ||
| else if(texture.matches("^[0-9a-fA-F]+$")) texture = "http://textures.minecraft.net/texture/"+texture; | ||
| else; // TODO: Texture invalid, print a warning or something? |
There was a problem hiding this comment.
I wouldn't log anything. The method is called frequently and we don't want it to spam the console.
| else; // TODO: Texture invalid, print a warning or something? | ||
| json = "{\"textures\":{\"SKIN\":{\"url\":\""+texture+"\"}}}"; // (Stable for all MC versions with custom skins.) | ||
| texture = Base64.getEncoder().encodeToString(json.getBytes(StandardCharsets.ISO_8859_1)); | ||
| } |
There was a problem hiding this comment.
After you move the if block into the try block, the entirety of this "texture-modification" logic should be moved into a private method. By moving it out, we simplify this method and make everything easier to read. The private method could be, for example:
private String normalizeTextureURL(String texture) {
// ...
}| try{ | ||
| json = Base64.getDecoder().decode(texture); | ||
| } | ||
| catch(IllegalArgumentException ex){ |
There was a problem hiding this comment.
Please, adhere to some unified code style. The formatting of this code is completely different from the rest of the project. This applies to all new code.
| String json; | ||
|
|
||
| try{ | ||
| json = Base64.getDecoder().decode(texture); |
There was a problem hiding this comment.
I don't know if I'm doing something wrong, but doesn't decode return a byte[] here?
| } | ||
| if(!json.contains("\"url\":\"http://textures.minecraft.net/texture/")){ | ||
| // Decoded json does not contain a valid skin URL, so check if the 'texture' argument is a valid URL. | ||
| if(texture.startsWith("http://textures.minecraft.net/texture/")); // We are all set. |
There was a problem hiding this comment.
I think this check should be done before trying to decode texture as json. You actually don't really need to decode it at all. Just check if texture is a URL, and if it is, do whatever you do here. Otherwise just skip this and don't do anything.
| @@ -118,17 +120,32 @@ public static String getTexture(@NonNull ItemStack itemStack) { | |||
| * Set the Base64 texture of the given skull ItemStack. | |||
| * | |||
| * @param itemStack The ItemStack. | |||
| * @param texture The new skull texture (Base64). | |||
| * @param texture The new skull texture (Base64 or URL). | |||
There was a problem hiding this comment.
It's not really "or URL". It can either be a very specific URL: http://textures.minecraft.net/texture/ or a hexidecimal id? of a texture on the same site. Just mentioning URL can be misleading here.
|
Unfortunately I don't have the development branch in which I originally drafted this PR, and although I could recreate it I also just don't have as many free cycles in my day for open source community contributions like this at the moment :( It looks like it's pretty close to complete how the project owners want it to be, so maybe another author can step in and complete this. For context, I only sent this PR because someone from the DecentHolograms dev team was asking about it in a mutual Discord, I am not actually a regular contributor (or even a user) of this project, so I just wanted to give an example of how they could support it. |
For the setSkullTexture() method in SkullUtils.java:
public static void setSkullTexture(@nonnull ItemStack itemStack, @nonnull String texture);
...
Allow the
textureargument to be either a Base64 or URL, with some basic validation