-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support color to filled map item #2504
Conversation
@@ -19,15 +20,16 @@ public static boolean describes(ObjectTag item) { | |||
return false; | |||
} | |||
Material mat = ((ItemTag) item).getBukkitMaterial(); | |||
// Leather armor and potions | |||
// Leather armor, potions and filled map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing comma: potions,
@@ -66,7 +68,7 @@ public ObjectTag getObjectAttribute(Attribute attribute) { | |||
// @mechanism ItemTag.color | |||
// @group properties | |||
// @description | |||
// Returns the color of the leather armor item or potion item. | |||
// Returns the color of the leather armor item, potion item or filled map item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing comma
plugin/src/main/java/com/denizenscript/denizen/objects/properties/item/ItemColor.java
Outdated
Show resolved
Hide resolved
This was not properly tested on real map items - Testing thoroughly is mandatory prior to PRs. |
if (mat == Material.FILLED_MAP) { | ||
MapMeta mapMeta = (MapMeta) item.getItemMeta(); | ||
if (!mapMeta.hasColor()) { | ||
return BukkitColorExtensions.fromColor(Color.WHITE).getObjectAttribute(attribute.fulfill(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is returning WHITE
here correct? Seems like it should probably be null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh... i dont know which default color of filled_map 😅, that why i put white color. i'll change it asap, thanks for remind me to using null
if (!mapMeta.hasColor()) { | ||
return null; | ||
} | ||
return BukkitColorExtensions.fromColor((mapMeta.getColor())).getObjectAttribute(attribute.fulfill(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor format - too many parens here
if (!mapMeta.hasColor()) { | ||
return null; | ||
} | ||
return BukkitColorExtensions.fromColor(mapMeta.getColor()).getObjectAttribute(attribute.fulfill((1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... you fixed one and unfixed the other???
@@ -79,6 +81,13 @@ public ObjectTag getObjectAttribute(Attribute attribute) { | |||
} | |||
return BukkitColorExtensions.fromColor(pm.getColor()).getObjectAttribute(attribute.fulfill((1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought it was correct because i follow on this line.... but, i realize and didn't get how getObjectAttribute
work. should i change this line too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole file is weird and needs cleaning up, but not really required for your PR to fix pre-existing issues (it'd be really handy if you did tho! - check #2355 if you're interested in helping cleanup the codebase)
No description provided.