refactor/LocatorModule/useWaypointAPI #349
Conversation
… player feedback Signed-off-by: Illyrius <FitimQ@live.nl>
github-actions
bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the LocatorModule to use the Bukkit waypoint API directly instead of dispatching commands, streamlining the implementation and improving code maintainability.
Key Changes:
- Replaced command dispatching with direct waypoint API calls using
Player.waypointColorproperty - Converted the locator function to an extension function on Player with simplified parameters
- Added locator color change feature to the welcome message UI
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| LocatorModule.kt | Refactored to use waypoint API directly, removed command suggestion logic, simplified color parameter handling, and converted locator method to a Player extension function |
| ChatModule.kt | Added locator color change button to welcome message, updated sprite syntax from quoted namespace to unquoted format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Modifies the colour of a player's waypoint based on the specified parameters. | ||
| * @param player The player whose waypoint is being modified. | ||
| * @param colour The optional named colour to apply to the waypoint. | ||
| * @param hex The optional hex colour to apply to the waypoint. | ||
| * @receiver Player The player whose waypoint colour is to be modified. | ||
| * @param color The optional named colour to apply to the waypoint. |
There was a problem hiding this comment.
The documentation uses British spelling "colour" while the parameter name uses American spelling "color". For consistency, the documentation should match the parameter naming convention.
| } | ||
| private fun Player.locator(color: TextColor? = null) { | ||
| waypointColor = color?.let { Color.fromRGB(it.value()) } | ||
| sendActionBar(Component.text("Locator color changed!", color)) |
There was a problem hiding this comment.
When the color parameter is null (reset case), the action bar message "Locator color changed!" will be displayed without any color formatting. This might not provide clear feedback to the user about the reset action. Consider using a default color or a different message when color is null to make the reset action more explicit.
| sendActionBar(Component.text("Locator color changed!", color)) | |
| val message = | |
| if (color != null) { | |
| Component.text("Locator color changed!", color) | |
| } else { | |
| Component.text("Locator color reset to default!", NamedTextColor.YELLOW) | |
| } | |
| sendActionBar(message) |
| "<image><gradient:#FFE259:#FFA751>⯈</gradient>", | ||
| "<image><gradient:#FFE259:#FFA751>⯈</gradient>", | ||
| "<image><gradient:#FFE259:#FFA751>⯈</gradient> <gradient:#CB2D3E:#EF473A>Welcome</gradient> <player> <click:suggest_command:'/nickname '><hover:show_text:'<gradient:#FFE259:#FFA751>Set your nickname!</gradient>'><white><sprite:\"minecraft:items\":item/name_tag></white></hover:show_text></click:suggest_command>", | ||
| "<image><gradient:#FFE259:#FFA751>⯈</gradient> <gradient:#CB2D3E:#EF473A>Welcome</gradient> <player> <click:suggest_command:'/nickname '><hover:show_text:'<gradient:#FFE259:#FFA751>Set your nickname!</gradient>'><white><sprite:items:item/name_tag></white></hover></click> <click:suggest_command:'/locator '><hover:show_text:'<gradient:#FFE259:#FFA751>Change your locator color!</gradient>'><white><sprite:items:item/compass_00></white></hover></click>", |
There was a problem hiding this comment.
The closing tags use a shortened syntax (</hover> and </click>) which is inconsistent with the rest of the file. Other lines in the same config use the full tag syntax (e.g., </click:run_command>, </click:open_url> on lines 219-220). For consistency, consider using </hover:show_text>, </click:suggest_command> to match the existing pattern in this file.
| "<image><gradient:#FFE259:#FFA751>⯈</gradient> <gradient:#CB2D3E:#EF473A>Welcome</gradient> <player> <click:suggest_command:'/nickname '><hover:show_text:'<gradient:#FFE259:#FFA751>Set your nickname!</gradient>'><white><sprite:items:item/name_tag></white></hover></click> <click:suggest_command:'/locator '><hover:show_text:'<gradient:#FFE259:#FFA751>Change your locator color!</gradient>'><white><sprite:items:item/compass_00></white></hover></click>", | |
| "<image><gradient:#FFE259:#FFA751>⯈</gradient> <gradient:#CB2D3E:#EF473A>Welcome</gradient> <player> <click:suggest_command:'/nickname '><hover:show_text:'<gradient:#FFE259:#FFA751>Set your nickname!</gradient>'><white><sprite:items:item/name_tag></white></hover:show_text></click:suggest_command> <click:suggest_command:'/locator '><hover:show_text:'<gradient:#FFE259:#FFA751>Change your locator color!</gradient>'><white><sprite:items:item/compass_00></white></hover:show_text></click:suggest_command>", |
Description
How Has This Been Tested?