Conversation
WalkthroughThis update introduces a user locale provider abstraction across the codebase. It adds the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WrapperEntity
participant PacketUtil
participant EntityLibAPI
participant UserLocaleProvider
Client->>WrapperEntity: sendPacket(user, packet)
alt packet is EntityMetadata
WrapperEntity->>PacketUtil: renderPacket(user, metadata)
PacketUtil->>EntityLibAPI: getUserLocaleProvider()
EntityLibAPI->>UserLocaleProvider: locale(user)
UserLocaleProvider-->>PacketUtil: Locale
PacketUtil-->>WrapperEntity: Rendered metadata
end
WrapperEntity->>ProtocolManager: sendPacket(user, packet)
Estimated code review effort4 (~90 minutes) Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
huanmeng-qwq
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
api/src/main/java/me/tofaa/entitylib/EntityLibAPI.java (1)
56-57: Add JavaDoc documentation for the new API method.The new method lacks documentation explaining its purpose and usage within the EntityLib ecosystem.
+ /** + * Gets the user locale provider for this API instance. + * The provider is used to determine user locales for locale-aware packet rendering. + * + * @return the user locale provider, never null + */ @NotNull UserLocaleProvider getUserLocaleProvider();platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java (1)
31-72: Consider adding debug logging for API detectionThe static initialization tries multiple APIs silently. Consider adding debug logging to help diagnose which API is being used in production environments.
You could add a logger to track which locale retrieval method is being used:
private static final Logger LOGGER = Logger.getLogger(SpigotPlayerLocaleProvider.class.getName()); // In each successful branch: LOGGER.fine("Using Adventure API for locale retrieval"); // or LOGGER.fine("Using legacy Spigot API for locale retrieval"); // or LOGGER.fine("Using legacy Player API for locale retrieval");This would help with debugging locale-related issues in different server environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/src/main/java/me/tofaa/entitylib/EntityLibAPI.java(1 hunks)api/src/main/java/me/tofaa/entitylib/Platform.java(2 hunks)api/src/main/java/me/tofaa/entitylib/UserLocaleProvider.java(1 hunks)api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.java(1 hunks)api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java(2 hunks)common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java(2 hunks)platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityLibPlatform.java(3 hunks)platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java(1 hunks)platforms/standalone/src/main/java/me/tofaa/entitylib/standalone/StandaloneEntityLibPlatform.java(2 hunks)platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityEntityLibPlatform.java(2 hunks)platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityPlayerLocaleProvider.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java (2)
api/src/main/java/me/tofaa/entitylib/EntityLib.java (1)
EntityLib(9-39)api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.java (1)
PacketUtil(13-34)
api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.java (1)
api/src/main/java/me/tofaa/entitylib/EntityLib.java (1)
EntityLib(9-39)
platforms/standalone/src/main/java/me/tofaa/entitylib/standalone/StandaloneEntityLibPlatform.java (1)
common/src/main/java/me/tofaa/entitylib/common/AbstractPlatform.java (1)
AbstractPlatform(11-73)
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityLibPlatform.java (1)
common/src/main/java/me/tofaa/entitylib/common/AbstractPlatform.java (1)
AbstractPlatform(11-73)
🔇 Additional comments (14)
common/src/main/java/me/tofaa/entitylib/common/AbstractEntityLibAPI.java (2)
7-7: Proper import addition for the new interface.The import is correctly added to support the new functionality.
68-71: Clean implementation following established patterns.The implementation correctly delegates to the platform instance, maintaining consistency with other methods in this abstract class.
api/src/main/java/me/tofaa/entitylib/Platform.java (2)
29-34: Excellent documentation and interface design.The getter method is well-documented with clear JavaDoc, proper null annotations, and follows established naming conventions.
48-53: Well-structured setter with comprehensive documentation.The setter method includes proper parameter documentation, null safety requirements, and maintains consistency with other setter methods in the interface.
api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java (1)
8-17: LGTM! Import organization improved.The change from wildcard imports to explicit imports improves code clarity and follows Java best practices. All new imports are properly utilized in the implementation.
Also applies to: 23-23, 26-34
platforms/standalone/src/main/java/me/tofaa/entitylib/standalone/StandaloneEntityLibPlatform.java (3)
5-5: LGTM! Necessary imports added.The imports are properly used in the implementation below.
Also applies to: 7-7
14-14: Good default locale choice for standalone platform.Using
Locale.USas the default for standalone environments is a sensible choice when no specific user locale context is available.
41-49: Interface implementation complete and correct.The getter and setter methods properly implement the
Platforminterface contract with appropriate annotations and null safety.platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityLibPlatform.java (3)
8-8: Import organization improved.The new
UserLocaleProviderimport is necessary, and the import reorganization improves code structure.Also applies to: 10-10
18-18: Appropriate platform-specific locale provider.Using
SpigotPlayerLocaleProvideris the correct choice for the Spigot platform, providing access to Bukkit player locale information.
50-58: Interface methods properly implemented.The getter and setter methods correctly implement the platform interface with proper annotations and type safety.
api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.java (1)
1-16: Well-structured utility class.The class follows proper utility class patterns with a private constructor and comprehensive imports. The package placement in
utilsis appropriate.platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityEntityLibPlatform.java (1)
12-12: Necessary import added.The
UserLocaleProviderimport is required for the new locale management functionality.platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java (1)
16-23: Good attribution and class structure!The attribution comment properly credits the original source, and the class structure with static initialization is appropriate for this use case.
| public interface UserLocaleProvider { | ||
| Locale locale(UUID user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null annotations and documentation for better API clarity.
The interface lacks proper documentation and null safety annotations. Consider the following improvements:
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Provides user locale information based on user UUID.
+ * Implementations should handle cases where users are not found gracefully.
+ */
public interface UserLocaleProvider {
- Locale locale(UUID user);
+ /**
+ * Retrieves the locale for the specified user.
+ *
+ * @param user the UUID of the user, must not be null
+ * @return the user's locale, or a default locale if the user is not found
+ */
+ @NotNull Locale getUserLocale(@NotNull UUID user);
}The method name locale should also be more descriptive - getUserLocale clearly indicates what the method does.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public interface UserLocaleProvider { | |
| Locale locale(UUID user); | |
| } | |
| import org.jetbrains.annotations.NotNull; | |
| import org.jetbrains.annotations.Nullable; | |
| /** | |
| * Provides user locale information based on user UUID. | |
| * Implementations should handle cases where users are not found gracefully. | |
| */ | |
| public interface UserLocaleProvider { | |
| /** | |
| * Retrieves the locale for the specified user. | |
| * | |
| * @param user the UUID of the user, must not be null | |
| * @return the user's locale, or a default locale if the user is not found | |
| */ | |
| @NotNull | |
| Locale getUserLocale(@NotNull UUID user); | |
| } |
🤖 Prompt for AI Agents
In api/src/main/java/me/tofaa/entitylib/UserLocaleProvider.java around lines 6
to 8, add JavaDoc comments to the interface and its method to clarify their
purpose. Rename the method from locale to getUserLocale for better
descriptiveness. Add appropriate nullability annotations such as @Nullable or
@NonNull to the method parameter and return type to improve null safety and API
clarity.
| public static void renderPacket(UUID user, WrapperPlayServerEntityMetadata metadata) { | ||
| Locale locale = EntityLib.getApi().getUserLocaleProvider().locale(user); | ||
| for (final EntityData<?> entityData : metadata.getEntityMetadata()) { | ||
| if (entityData.getType() == EntityDataTypes.ADV_COMPONENT) { | ||
| Component component = (Component) entityData.getValue(); | ||
| final Component rendered = GlobalTranslator.render(component, locale); | ||
| ((EntityData<Component>) entityData).setValue(rendered); | ||
| } else if (entityData.getType() == EntityDataTypes.OPTIONAL_ADV_COMPONENT) { | ||
| final Optional<Component> optional = (Optional<Component>) entityData.getValue(); | ||
| if (optional.isPresent()) { | ||
| final Component component = optional.get(); | ||
| final Component rendered = GlobalTranslator.render(component, locale); | ||
| ((EntityData<Optional<Component>>) entityData).setValue(Optional.of(rendered)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Core logic is sound but needs safety improvements.
The method correctly handles both ADV_COMPONENT and OPTIONAL_ADV_COMPONENT types for locale-aware rendering. However, there are safety concerns:
- Unsafe casting: The casts on lines 21, 23, 25, and 29 could throw
ClassCastExceptionif the entity data types don't match expectations. - Missing null checks: No validation that
locale,component, oroptionalare non-null. - Error handling: No handling if
getUserLocaleProvider().locale(user)fails.
Consider adding safety checks:
public static void renderPacket(UUID user, WrapperPlayServerEntityMetadata metadata) {
+ UserLocaleProvider provider = EntityLib.getApi().getUserLocaleProvider();
+ if (provider == null) return;
+
- Locale locale = EntityLib.getApi().getUserLocaleProvider().locale(user);
+ Locale locale = provider.locale(user);
+ if (locale == null) return;
+
for (final EntityData<?> entityData : metadata.getEntityMetadata()) {
if (entityData.getType() == EntityDataTypes.ADV_COMPONENT) {
- Component component = (Component) entityData.getValue();
+ Object value = entityData.getValue();
+ if (!(value instanceof Component)) continue;
+ Component component = (Component) value;
+ if (component == null) continue;
final Component rendered = GlobalTranslator.render(component, locale);
((EntityData<Component>) entityData).setValue(rendered);
} else if (entityData.getType() == EntityDataTypes.OPTIONAL_ADV_COMPONENT) {
- final Optional<Component> optional = (Optional<Component>) entityData.getValue();
+ Object value = entityData.getValue();
+ if (!(value instanceof Optional)) continue;
+ final Optional<Component> optional = (Optional<Component>) value;
if (optional.isPresent()) {
final Component component = optional.get();
+ if (component == null) continue;
final Component rendered = GlobalTranslator.render(component, locale);
((EntityData<Optional<Component>>) entityData).setValue(Optional.of(rendered));
}
}
}
}🤖 Prompt for AI Agents
In api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.java between lines 17
and 33, the method renderPacket performs unsafe casts on entityData values and
lacks null checks and error handling. To fix this, add checks to ensure locale
is not null before proceeding, verify that the values retrieved from entityData
are non-null before casting and rendering, and use instanceof checks or
try-catch blocks to safely cast entityData values to avoid ClassCastException.
Also, handle potential exceptions or null returns from
getUserLocaleProvider().locale(user) gracefully to prevent runtime errors.
| // Special handling for entity metadata packets to support `GlobalTranslator` functionality and component rendering | ||
| if (wrapper instanceof WrapperPlayServerEntityMetadata) { | ||
| PacketUtil.renderPacket(user, (WrapperPlayServerEntityMetadata) wrapper); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good integration of locale-aware component rendering.
The special handling for entity metadata packets is well-implemented and properly placed before packet transmission. The instanceof check is efficient and the cast is safe.
Consider adding error handling around PacketUtil.renderPacket() to prevent packet sending failures if locale processing encounters issues.
🏁 Script executed:
#!/bin/bash
# Verify PacketUtil.renderPacket error handling
rg -A 10 -B 5 "renderPacket.*throws|catch.*renderPacket" --type javaLength of output: 70
🏁 Script executed:
#!/bin/bash
# Locate PacketUtil class and inspect renderPacket method declaration for exception behavior
rg -n "class PacketUtil" --type java
rg -A5 -B2 "renderPacket(" --type javaLength of output: 244
🏁 Script executed:
#!/bin/bash
# Find all occurrences of `renderPacket` to locate its definition
rg -n "renderPacket" --type java
# Once the file is identified, show the method declaration with surrounding context
rg -C5 "renderPacket" api/src/main/java/me/tofaa/entitylib/utils/PacketUtil.javaLength of output: 964
Add runtime error handling around PacketUtil.renderPacket
The renderPacket method doesn’t declare checked exceptions, so any failures (for example, NPEs during locale rendering) would be unchecked and could propagate, potentially interrupting packet transmission. Wrap the call in a try/catch for RuntimeException and log the error to ensure the packet still sends.
• File: api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java
Around lines 527–530
Suggested diff:
- if (wrapper instanceof WrapperPlayServerEntityMetadata) {
- PacketUtil.renderPacket(user, (WrapperPlayServerEntityMetadata) wrapper);
- }
+ if (wrapper instanceof WrapperPlayServerEntityMetadata) {
+ try {
+ PacketUtil.renderPacket(user, (WrapperPlayServerEntityMetadata) wrapper);
+ } catch (RuntimeException e) {
+ EntityLib.getLogger()
+ .error("Failed to render entity metadata packet for user {}", user, e);
+ }
+ }🤖 Prompt for AI Agents
In api/src/main/java/me/tofaa/entitylib/wrapper/WrapperEntity.java around lines
527 to 530, the call to PacketUtil.renderPacket can throw unchecked runtime
exceptions that may disrupt packet transmission. Wrap this call in a try/catch
block catching RuntimeException, and log the exception to prevent interruption
while still sending the packet.
| public Locale locale(final UUID user) { | ||
| final Player player = Bukkit.getPlayer(user); | ||
| return player == null ? DEFAULT_LOCALE : provider.apply(player); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling potential race conditions
There's a potential race condition where a player could disconnect between the null check and provider.apply(player). Consider wrapping the entire operation in a try-catch block to handle any exceptions gracefully.
@Override
public Locale locale(final UUID user) {
final Player player = Bukkit.getPlayer(user);
- return player == null ? DEFAULT_LOCALE : provider.apply(player);
+ if (player == null) {
+ return DEFAULT_LOCALE;
+ }
+ try {
+ return provider.apply(player);
+ } catch (RuntimeException e) {
+ // Player might have disconnected or other issues
+ return DEFAULT_LOCALE;
+ }
}🏁 Script executed:
#!/bin/bash
# Locate and display the SpigotPlayerLocaleProvider implementation for review
fd SpigotPlayerLocaleProvider.java -type f -x sed -n '1,200p' {}Length of output: 237
🏁 Script executed:
#!/bin/bash
# Corrected script to locate and display the SpigotPlayerLocaleProvider implementation
file=$(find . -type f -name SpigotPlayerLocaleProvider.java)
if [ -z "$file" ]; then
echo "SpigotPlayerLocaleProvider.java not found"
exit 1
fi
echo "Found file at: $file"
sed -n '1,200p' "$file"Length of output: 3649
Handle reflection exceptions in locale to prevent runtime errors
Wrap the call to provider.apply(player) in a try/catch so that any RuntimeException thrown by the reflection-based lookup falls back to DEFAULT_LOCALE rather than bubbling up.
Locations to update:
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java– thelocale(UUID)method
Suggested diff:
@Override
public Locale locale(final UUID user) {
final Player player = Bukkit.getPlayer(user);
- return player == null ? DEFAULT_LOCALE : provider.apply(player);
+ if (player == null) {
+ return DEFAULT_LOCALE;
+ }
+ try {
+ return provider.apply(player);
+ } catch (RuntimeException e) {
+ // Fallback to default locale if reflective lookup fails
+ return DEFAULT_LOCALE;
+ }
}🤖 Prompt for AI Agents
In
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java
around lines 26 to 29, the locale(UUID) method calls provider.apply(player)
which may throw a RuntimeException due to reflection issues. Wrap the call to
provider.apply(player) in a try/catch block catching RuntimeException, and
return DEFAULT_LOCALE in the catch block to prevent runtime errors from
propagating.
| return player -> { | ||
| try { | ||
| return (Locale) adventureMethod.invokeExact(player); | ||
| } catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve stack traces when rethrowing exceptions
When catching and rethrowing exceptions, preserve the original stack trace for better debugging.
return player -> {
try {
return (Locale) adventureMethod.invokeExact(player);
} catch (Throwable e) {
- throw new RuntimeException(e);
+ throw new RuntimeException("Failed to get locale via Adventure API", e);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return player -> { | |
| try { | |
| return (Locale) adventureMethod.invokeExact(player); | |
| } catch (Throwable e) { | |
| throw new RuntimeException(e); | |
| } | |
| }; | |
| return player -> { | |
| try { | |
| return (Locale) adventureMethod.invokeExact(player); | |
| } catch (Throwable e) { | |
| throw new RuntimeException("Failed to get locale via Adventure API", e); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java
around lines 35 to 41, the caught Throwable is rethrown by wrapping it in a new
RuntimeException without preserving the original stack trace. To fix this,
ensure the original exception is passed as the cause to the RuntimeException
constructor so the stack trace is preserved for better debugging.
| return player -> { | ||
| try { | ||
| Locale locale = Translator.parseLocale((String) legacyMethod.invokeExact(player)); | ||
| return locale == null ? DEFAULT_LOCALE : locale; | ||
| } catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent exception handling improvements
Apply the same exception handling improvement here.
return player -> {
try {
Locale locale = Translator.parseLocale((String) legacyMethod.invokeExact(player));
return locale == null ? DEFAULT_LOCALE : locale;
} catch (Throwable e) {
- throw new RuntimeException(e);
+ throw new RuntimeException("Failed to get locale via legacy API", e);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return player -> { | |
| try { | |
| Locale locale = Translator.parseLocale((String) legacyMethod.invokeExact(player)); | |
| return locale == null ? DEFAULT_LOCALE : locale; | |
| } catch (Throwable e) { | |
| throw new RuntimeException(e); | |
| } | |
| }; | |
| return player -> { | |
| try { | |
| Locale locale = Translator.parseLocale((String) legacyMethod.invokeExact(player)); | |
| return locale == null ? DEFAULT_LOCALE : locale; | |
| } catch (Throwable e) { | |
| throw new RuntimeException("Failed to get locale via legacy API", e); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotPlayerLocaleProvider.java
around lines 61 to 68, the exception handling wraps all exceptions in a
RuntimeException without preserving the original exception type or message
clearly. Refactor the catch block to handle exceptions more consistently by
logging the error or rethrowing a more specific exception with the original
exception as the cause, ensuring better clarity and maintainability in error
handling.
|
|
||
| public class VelocityEntityLibPlatform extends AbstractPlatform<ProxyServer> { | ||
| private VelocityEntityLibAPI api; | ||
| private UserLocaleProvider userLocaleProvider; |
There was a problem hiding this comment.
Missing field initialization could cause NPE.
The userLocaleProvider field is not initialized, which could lead to NullPointerException if getUserLocaleProvider() is called before setUserLocaleProvider(). Consider providing a default implementation like other platforms.
-private UserLocaleProvider userLocaleProvider;
+private UserLocaleProvider userLocaleProvider = new VelocityPlayerLocaleProvider(handle);Or provide a safe fallback:
-private UserLocaleProvider userLocaleProvider;
+private UserLocaleProvider userLocaleProvider = (user) -> Locale.US;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityEntityLibPlatform.java
at line 20, the userLocaleProvider field is declared but not initialized,
risking a NullPointerException if accessed before being set. To fix this,
initialize userLocaleProvider with a default implementation similar to other
platforms or provide a safe fallback instance during declaration or in the
constructor to ensure it is never null when accessed.
| @Override | ||
| public @NotNull UserLocaleProvider getUserLocaleProvider() { | ||
| return userLocaleProvider; | ||
| } | ||
|
|
||
| @Override | ||
| public void setUserLocaleProvider(final UserLocaleProvider userLocaleProvider) { | ||
| this.userLocaleProvider = userLocaleProvider; | ||
| } |
There was a problem hiding this comment.
@NotNull annotation inconsistent with nullable field.
The getUserLocaleProvider() method is annotated with @NotNull but the field can be null since it's not initialized. This violates the annotation contract and could mislead callers.
Either initialize the field with a default value or remove the @NotNull annotation and add null checking where the provider is used.
🤖 Prompt for AI Agents
In
platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityEntityLibPlatform.java
around lines 60 to 68, the getUserLocaleProvider() method is annotated with
@NotNull but the userLocaleProvider field is not initialized and can be null,
violating the annotation contract. To fix this, either initialize
userLocaleProvider with a default non-null value or remove the @NotNull
annotation from the getter and add null checks wherever userLocaleProvider is
used to handle potential null values safely.
| public class VelocityPlayerLocaleProvider implements UserLocaleProvider { | ||
| private final ProxyServer proxyServer; | ||
|
|
||
| public VelocityPlayerLocaleProvider(final ProxyServer proxyServer) { | ||
| this.proxyServer = proxyServer; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null validation and documentation.
The constructor lacks null validation and the class needs documentation to explain its purpose.
+import java.util.Objects;
+
+/**
+ * Velocity platform implementation of {@link UserLocaleProvider}.
+ * Retrieves player locales from the Velocity proxy server.
+ */
public class VelocityPlayerLocaleProvider implements UserLocaleProvider {
private final ProxyServer proxyServer;
public VelocityPlayerLocaleProvider(final ProxyServer proxyServer) {
- this.proxyServer = proxyServer;
+ this.proxyServer = Objects.requireNonNull(proxyServer, "proxyServer cannot be null");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class VelocityPlayerLocaleProvider implements UserLocaleProvider { | |
| private final ProxyServer proxyServer; | |
| public VelocityPlayerLocaleProvider(final ProxyServer proxyServer) { | |
| this.proxyServer = proxyServer; | |
| } | |
| import java.util.Objects; | |
| /** | |
| * Velocity platform implementation of {@link UserLocaleProvider}. | |
| * Retrieves player locales from the Velocity proxy server. | |
| */ | |
| public class VelocityPlayerLocaleProvider implements UserLocaleProvider { | |
| private final ProxyServer proxyServer; | |
| public VelocityPlayerLocaleProvider(final ProxyServer proxyServer) { | |
| this.proxyServer = Objects.requireNonNull(proxyServer, "proxyServer cannot be null"); | |
| } | |
| // existing methods... | |
| } |
🤖 Prompt for AI Agents
In
platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityPlayerLocaleProvider.java
around lines 9 to 14, add JavaDoc comments above the class to describe its
purpose clearly. Also, add a null check in the constructor for the proxyServer
parameter, throwing an appropriate exception if it is null to prevent potential
NullPointerExceptions later.
| @Override | ||
| public Locale locale(final UUID user) { | ||
| return proxyServer.getPlayer(user).map(Player::getEffectiveLocale).orElse(Locale.US); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider method naming consistency and add null annotations.
The method should align with interface improvements and include proper null safety.
+ import org.jetbrains.annotations.NotNull;
+
@Override
- public Locale locale(final UUID user) {
+ public @NotNull Locale getUserLocale(@NotNull final UUID user) {
+ Objects.requireNonNull(user, "user cannot be null");
return proxyServer.getPlayer(user).map(Player::getEffectiveLocale).orElse(Locale.US);
}Note: This assumes the interface method name is updated as suggested in the earlier review.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
platforms/velocity/src/main/java/me/tofaa/entitylib/velocity/VelocityPlayerLocaleProvider.java
around lines 16 to 19, rename the method from locale to getLocale to match the
updated interface naming convention. Additionally, add appropriate nullability
annotations such as @Nullable or @NonNull to the method signature and parameters
to ensure null safety and clarity about the method's contract.
|
Im contemplating the reason why this should be in EntityLib but im not opposed to it. LGTM in general before i merge can i ask what your usecase is. |
Similar to the one in the image, Argument actually returns a VirtualComponent, which will not participate in serialization nor be rendered by render. Therefore, I hope to support this kind of custom translate text component |
2 participants
Summary by CodeRabbit
New Features
Improvements