GH-1221 Reformat placeholder definitions #1221

Rollczi · 2025-10-21T18:24:08Z

No description provided.

gemini-code-assist

Code Review

This pull request focuses on refactoring the placeholder system to use a new Placeholder interface, replacing the old PlaceholderReplacer. This involves changes in how placeholders are registered and used throughout the codebase, including the AFK and Home features. The changes also include the introduction of PlaceholdersDocs for documenting placeholders.

Jakubk15 · 2025-10-24T18:36:55Z

...ore-core/src/main/java/com/eternalcode/core/feature/afk/placeholder/AfkPlaceholderSetup.java

-import com.eternalcode.annotations.scan.placeholder.PlaceholderDocs;
+import com.eternalcode.annotations.scan.placeholder.PlaceholdersDocs;
+import com.eternalcode.annotations.scan.placeholder.PlaceholdersDocs.Entry;
+import static com.eternalcode.annotations.scan.placeholder.PlaceholdersDocs.Entry.*;


Static imports should typically go to the bottom

Jakubk15 · 2025-10-24T18:39:05Z

...lcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholderBukkitRegistryImpl.java


    private final Server server;
-    private final Set<PlaceholderReplacer> replacerPlayers = new HashSet<>();
+    private final Set<Placeholder> replacerPlayers = new HashSet<>();


Suggested change

private final Set<Placeholder> replacerPlayers = new HashSet<>();

private final Set<Placeholder> playerPlaceholders = new HashSet<>();

eternalcore-docs-api/src/main/java/com/eternalcode/annotations/scan/reflect/AnnotationUtil.java

Jakubk15 · 2025-10-24T18:42:16Z

...ore-docs-api/src/main/java/com/eternalcode/annotations/scan/GroupAnnotationScanResolver.java

+import java.lang.annotation.Annotation;
+import java.util.List;
+
+public abstract class GroupAnnotationScanResolver<A extends Annotation, G extends Annotation, RESULT> implements EternalScanResolver<RESULT> {


I think GROUP will do better as a generic name here

Jakubk15 · 2025-10-24T18:43:14Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the placeholder system by introducing a new Placeholder interface and a grouped @PlaceholdersDocs annotation for better organization and documentation. The changes are generally positive, simplifying the code and improving maintainability.

However, I've identified several issues that should be addressed:

A critical bug in the new annotation processing logic (GroupAnnotationScanResolver) that will prevent it from functioning correctly.
Logic bugs in the afk_formatted and homes_left placeholder implementations, which could lead to incorrect behavior for users.
A performance degradation in how configuration-based placeholders are registered.
Inconsistencies in the new placeholder documentation regarding return types.

I have provided detailed comments with suggestions for fixes. Addressing these points will ensure the refactoring is robust and correct.

...ore-docs-api/src/main/java/com/eternalcode/annotations/scan/GroupAnnotationScanResolver.java

gemini-code-assist · 2025-10-24T18:45:58Z

...ore-core/src/main/java/com/eternalcode/core/feature/afk/placeholder/AfkPlaceholderSetup.java

+    void setUpPlaceholders(PlaceholderRegistry placeholders, AfkService afkService) {
+        placeholders.register(Placeholder.ofBoolean("afk", player -> afkService.isAfk(player.getUniqueId())));
+        placeholders.register(Placeholder.of("afk_formatted", player -> {
+            Translation messages = this.translationManager.getMessages();


The change from this.translationManager.getMessages(player.getUniqueId()) to this.translationManager.getMessages() appears to be a bug. The placeholder description for afk_formatted states it should be "based on player's language settings". Using getMessages() without a player context will likely result in using a default language for all players, instead of their specific language. This should be reverted to use the player-specific method call.

Suggested change

Translation messages = this.translationManager.getMessages();

Translation messages = this.translationManager.getMessages(player.getUniqueId());

gemini-code-assist · 2025-10-24T18:45:58Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomePlaceholderSetup.java

+        placeholders.register(Placeholder.ofInt("homes_left", target -> {
+            int homesLimit = this.homeService.getHomeLimit(target);
+            int amountOfHomes = this.homeService.getAmountOfHomes(target.getUniqueId());

-    @PlaceholderDocs(
-        name = "homes_limit",
-        description = "Returns the maximum number of homes the player can have.",
-        example = "5",
-        returnType = "int",
-        category = "Home",
-        requiresPlayer = true
-    )
-    private PlaceholderReplacer createHomesLimitPlaceholder() {
-        return PlaceholderReplacer.of(
-            "homes_limit",
-            (text, targetPlayer) ->
-                String.valueOf(this.homeService.getHomeLimit(targetPlayer))
-        );
-    }
-
-    @PlaceholderDocs(
-        name = "homes_left",
-        description = "Returns how many more homes the player can create before reaching the limit.",
-        example = "2",
-        returnType = "int",
-        category = "Home",
-        requiresPlayer = true
-    )
-    private PlaceholderReplacer createHomesLeftPlaceholder() {
-        return PlaceholderReplacer.of(
-            "homes_left",
-            (text, targetPlayer) -> {
-                int homesLimit = this.homeService.getHomeLimit(targetPlayer);
-                int amountOfHomes = this.homeService.getAmountOfHomes(targetPlayer.getUniqueId());
-
-                if (homesLimit < -1 || amountOfHomes > homesLimit) {
-                    return "0";
-                }
-
-                return String.valueOf(homesLimit - amountOfHomes);
+            if (homesLimit < -1 || amountOfHomes > homesLimit) {
+                return 0;
            }
-        );
+
+            return homesLimit - amountOfHomes;
+        }));


The logic for the homes_left placeholder seems incorrect when a player has an unlimited number of homes (usually represented by homesLimit == -1). The current implementation will return 0, which is misleading. For an unlimited limit, it should indicate that, for example by returning -1 or another special value since this is an integer placeholder.

The proposed suggestion handles the unlimited case and also simplifies the logic for calculating remaining homes.

placeholders.register(Placeholder.ofInt("homes_left", target -> { int homesLimit = this.homeService.getHomeLimit(target); if (homesLimit == -1) { return -1; // Use -1 to indicate unlimited } int amountOfHomes = this.homeService.getAmountOfHomes(target.getUniqueId()); return Math.max(0, homesLimit - amountOfHomes); }));

...ore-core/src/main/java/com/eternalcode/core/feature/afk/placeholder/AfkPlaceholderSetup.java

gemini-code-assist · 2025-10-24T18:45:58Z

eternalcore-core/src/main/java/com/eternalcode/core/placeholder/Placeholder.java

+    static Placeholder of(String target, Placeholder placeholder) {
+        return new PlaceholderRaw(target, player -> placeholder.apply(target, player));
+    }


The implementation of of(String target, Placeholder placeholder) seems incorrect or at least very confusing. It calls placeholder.apply(target, player), passing the new target string as the text to be processed. This means it will try to find and replace placeholders within the target string itself, which is likely not the intended behavior for aliasing a placeholder. This method should be reviewed to ensure it behaves as expected.

eternalcore-core/src/main/java/com/eternalcode/core/placeholder/PlaceholdersSetup.java

Rollczi requested a review from a team as a code owner October 21, 2025 18:24

Rollczi requested a review from vLuckyyy October 21, 2025 18:24

gemini-code-assist bot reviewed Oct 21, 2025

View reviewed changes

Rollczi changed the title ~~Reformat placeholder definitions~~ GH-1221 Reformat placeholder definitions Oct 21, 2025

vLuckyyy approved these changes Oct 21, 2025

View reviewed changes

noyzys approved these changes Oct 22, 2025

View reviewed changes

Jakubk15 reviewed Oct 24, 2025

View reviewed changes

gemini-code-assist bot reviewed Oct 24, 2025

View reviewed changes

Reformat placeholder definitions

b3f4d99

Rollczi force-pushed the Reformat-placeholder-definitions branch from c66db8a to b3f4d99 Compare October 26, 2025 20:38

CR

5a01379

Rollczi requested review from Jakubk15, noyzys and vLuckyyy October 26, 2025 21:34

vLuckyyy approved these changes Oct 26, 2025

View reviewed changes

vLuckyyy merged commit fc53c69 into master Oct 26, 2025
2 checks passed

vLuckyyy deleted the Reformat-placeholder-definitions branch October 26, 2025 21:38

	private final Set<Placeholder> replacerPlayers = new HashSet<>();
	private final Set<Placeholder> playerPlaceholders = new HashSet<>();

	Translation messages = this.translationManager.getMessages();
	Translation messages = this.translationManager.getMessages(player.getUniqueId());

Uh oh!

Uh oh!

GH-1221 Reformat placeholder definitions #1221

GH-1221 Reformat placeholder definitions #1221

Uh oh!

Conversation

Rollczi commented Oct 21, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Jakubk15 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Jakubk15 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jakubk15 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Jakubk15 commented Oct 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants