GH-930 Msg placeholders & Async placeholder for future #1203

P1otrulla · 2025-10-12T11:27:01Z

Tested with love for OpenSource 💕

https://medal.tv/pl/games/minecraft/clips/lhPGLpi6DGCYMFpSd?invite=cr-MSxmazgsNDI2MDIzOTA4&

* Register new placeholders for `msg_toggle` and `socialspy_status` in `MsgPlaceholderSetup`. * Extend `MsgMessages` with a `Placeholders` interface and implement language-specific placeholder formats in `ENMsgMessages` and `PLMsgMessages`.

* Add caching logic for `msg_toggle` state in `MsgPlaceholderSetup`. * Introduce new formatted placeholders for `msg_toggle` and `socialspy_status`. * Extend `MsgMessages.Placeholders` to support a loading state with translations.

…erAPI` * Adjust cache duration in `MsgPlaceholderSetup` from 200ms to 5s. * Rename `msg_toggle` placeholders to `msg_status` for consistency. * Extend `ENPlaceholders` and `PLPlaceholders` with `OkaeriConfig`. * Add `PlaceholderAPI` dependency in `runServer` task configuration.

* Replace existing cache in `MsgPlaceholderSetup` with `AsyncPlaceholderCached`. * Introduce `AsyncPlaceholderCacheRegistry` for managing placeholder caches. * Add `AsyncPlaceholderCacheController` to handle cache invalidation on player quit. * Simplify placeholder logic and remove redundant caching methods.

…derCacheRegistry` * Replace individual `stateCache` with dynamically registered cache in the `AsyncPlaceholderCacheRegistry`. * Simplify placeholder setup logic and adjust `MsgToggleService` to utilize the registry for cache management.

…lidation * Add `Subscribe` annotations to invalidate cache on `EternalReloadEvent` and `EternalShutdownEvent`.

* Rename `onEternalDisable` to `onDisable` and `onReload`.

gemini-code-assist

Code Review

This pull request introduces a new asynchronous caching mechanism for placeholders and integrates it with the messaging feature. The changes include a new AsyncPlaceholderCacheRegistry and AsyncPlaceholderCached for managing async data loading, new placeholders for message and social spy status, and refactoring MsgToggleServiceImpl to use the new caching system. The overall implementation is good, but I've found a few areas for improvement. There's a race condition in AsyncPlaceholderCached that could lead to multiple concurrent loads for the same data, some code duplication in MsgPlaceholderSetup when defining similar placeholders, and a minor inconsistency in a comment in one of the message files. I've left specific comments with suggestions on how to address these points.

...alcore-core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCached.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

noyzys · 2025-10-12T13:29:35Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+    }
+
+    @Subscribe(EternalInitializeEvent.class)
+    void setUpPlaceholders(PlaceholderRegistry placeholderRegistry) {


Proponowalbym poprawic nazewnictwo metody obecnie jest to ogolnikowe a nie jasne ze chodzi o rejestracje placeholderow, czyli jezeli jest kompozycja na registry dajemy setUpPlaceholders -> registerMessagePlaceholders, lub samo registryPlaceholders ale jezeli wciagamy "msg" to Message

noyzys · 2025-10-12T13:32:03Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+            player -> String.valueOf(this.msgService.isSpy(player.getUniqueId()))
+        ));
+
+        placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of(


Takie wyrazenia w scope mozna byloby wyizolowac do prywatnej metody przykladowo

private String socialSpyStatusFormatted(UUID uuid, Translation translation) { return msgService.isSpy(uuid) ? translation.msg().placeholders().socialSpyEnabled() : translation.msg().placeholders().socialSpyDisabled(); }

noyzys · 2025-10-12T13:34:20Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+    void setUpPlaceholders(PlaceholderRegistry placeholderRegistry) {
+        Translation translation = this.translationManager.getMessages();
+
+        AsyncPlaceholderCached<MsgState> stateCache = this.cacheRegistry.register(


Narazie wystarczy ale w przyszlosci jezeli bedziesz planowac w roadmapie dalej zasilac w pelni asynchroniczne placeholdery to wypadaloby zwracac Cfa i opakowywac go w calosci

noyzys · 2025-10-12T13:37:12Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+
+        placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of(
+            "socialspy_status",
+            player -> String.valueOf(this.msgService.isSpy(player.getUniqueId()))


Obecny auto boxing zamienic na Boolean#toString intuicyjniej bardziej i wyizolowac do prywatnej metody cos w stylu:

private String getSocialSpyStatus(UUID uuid) { return Boolean.toString(msgService.isSpy(uuid)); }

noyzys · 2025-10-12T13:39:50Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+@Controller
+public class MsgPlaceholderSetup {
+
+    public static final String MSG_STATE_CACHE_KEY = "msg_state";


Tutaj mozna byloby rozwazyc wrzucenie dodatkowych stalych globalnych na sama mozliwosc kontrolowania centralnia czas cache zamiast overidowac w metodzie rejeestracji

noyzys · 2025-10-12T14:08:32Z

...alcore-core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCached.java

+            .build();
+    }
+
+    public T getCached(UUID uuid) {


return null - jak value nie jest jeszcze dostepna to wymaga obslugi null majac ze soba ryzuko rzucenia NPE.
Obecnie nie zwraca pelnej informacji o tym ze loading odbywa sie cyklu async.

Proponowalbym zwracac CFA dlaczego lepiej? - bo zwracamy CFA zamiast null od razu wiemy ze score jest cyklem async,

-Mozna dorzucic null check na UUID uzyskujemy safe API

Izolacja logiki ladowania do loadAsync - czystsza seperacja wlasciwej odpowiedzialnosci.

Obslugujemy async ladowanie danych z pola loader i aktualizujemy cache,

Zapobiegamy wielokrotnemu ruszaniu loadera dla tego samego UUID

Wazne zeby logika bla non-blocking przy Paper API nie blokujac tickow serwera

Ogolnie to najlepiej opakowac takie logiki w jakis error handling dla spokoju i wytrwalosci

Finalnie by to wjechalo tak:

public CompletableFuture<T> getCached(UUID uuid) { // ewentualny null check dla UUID T cached = cache.getIfPresent(uuid); if (cached != null) { return CompletableFuture.completedFuture(cached); } return loading.computeIfAbsent(uuid, this::loadAsync); } private CompletableFuture<T> loadAsync(UUID uuid) { return loader.apply(uuid).whenComplete((value, throwable) -> { if (value != null) { cache.put(uuid, value); } loading.remove(uuid); }); }

noyzys · 2025-10-12T14:15:26Z

...core/src/main/java/com/eternalcode/core/placeholder/cache/AsyncPlaceholderCacheRegistry.java

+    public AsyncPlaceholderCacheRegistry() {
+    }
+
+    public <T> AsyncPlaceholderCached<T> register(String key, Function<UUID, CompletableFuture<T>> loader) {


Moze byc taka sytuacja ze key juz istnieje w mapie ale zostanie nadpisany bez ostrzezenia to mamy niestety utrate cache,
Minusem troche moze byc to ze nie ma konfiguracji loadera z timeoutem,
Null-check dla key i loader i ewentualne rzucenie wyjatku kiedy klucz juz istnieje na runtime, przekazywanie timeoutu loadera do PlaceholderCached to samo w metodach nizej.
Wlasciwa walidacja do logiki tutaj bylaby bardzo potrzebna jedna metoda zalatwiasz cala walidacje przykladowo:

private <T> void validateRegistrationParameters(String key, Function<UUID, CompletableFuture<T>> loader, Duration expireAfterWrite) { if (key == null || key.trim().isEmpty()) { throw new IllegalArgumentException("Cache key cannot be null or empty"); } if (loader == null) { throw new IllegalArgumentException("Loader function cannot be null"); } if (expireAfterWrite == null) { throw new IllegalArgumentException("Expire duration cannot be null"); } if (expireAfterWrite.isNegative() || expireAfterWrite.isZero()) { throw new IllegalArgumentException("Expire duration must be positive"); } }

Wszystko tutaj mozna rozbudowac i rozszerzyc na jakas konfiguracje jezeli potrzeba w obecnym stanie jezeli nie - zostawiamy i tyle

noyzys · 2025-10-12T14:33:24Z

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java

+import java.util.function.Consumer;

 @Service
 class MsgToggleServiceImpl implements MsgToggleService {


Rozbilbym to na kilka innych komponentow z ich odpowiedzialnoscia z kosztem korzysci,

MessageStateCache -> operacja na cache, izolacja logiki cache,

MessageStateToggleCalculator -> logika biznesowa od samego toggle w zamian za kalkulacje i invertowanie,

MessageStateUpdateCoordinator -> koordynacja samym update cos ala transaction-like,

MessageToggleServiceImpl -> Orchestracja uzyskujemy cienka warstwe ktora koordynuje tylko komponenty

Wszystko wchodzi do glownego MsgToggleService. Wyzej zaznaczylem Message ale to wiadomo o co raczej chodzi

@Override public CompletableFuture<Void> setState(UUID playerId, MsgState state) { return updateCoordinator.updateState(playerId, state); } // i tak dalej: MsgState newState = toggleCalculator.calculateToggledState(currentState);

noyzys

Jezeli cos niezrozumialego smialo pytac lub dawac wlasne propozycje zmian ;).
Wszystko mozna podkrecic i jezeli jest jakis problem nie sugerowac sie moimi zmianami bo zawsze jest jakies ala w trakcie review

Jakubk15 · 2025-10-12T11:55:58Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+
+        placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of(
+            "socialspy_status",
+            player -> String.valueOf(this.msgService.isSpy(player.getUniqueId()))


microoptimizations: use Boolean#toString instead of String.valueOf

Suggested change

player -> String.valueOf(this.msgService.isSpy(player.getUniqueId()))

player -> Boolean.toString(this.msgService.isSpy(player.getUniqueId()))

Jakubk15 · 2025-10-12T11:57:34Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/PLMsgMessages.java

+    @Getter
+    @Accessors(fluent = true)
+    public static class PLPlaceholders extends OkaeriConfig implements MsgMessages.Placeholders {
+        private String loading = "<yellow>Ładowanie...";


Suggested change

private String loading = "<yellow>Ładowanie...";

private String loading = "<yellow>Wczytywanie...";

Wczytywanie (w kontekście danych) tutaj brzmi lepiej, ładowanie bardziej się kojarzy z jakimiś intensywnymi zadaniami

vLuckyyy · 2025-10-13T20:37:38Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

+
+    @Getter
+    @Accessors(fluent = true)
+    public static class ENPlaceholders extends OkaeriConfig implements MsgMessages.Placeholders {


This section is too general; you should move it to the feature-related section. Creating such a section makes it similar to PlayerSection/ItemSection in the past. Currently, it only has a few messages, but over time, it will become very messy. Alternatively, I suggest you move it to the placeholders.yml file, but remember to also divide it into feature-related sections.

imDMK · 2025-10-13T04:29:35Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/MsgPlaceholderSetup.java

+        AsyncPlaceholderCached<MsgState> stateCache = this.cacheRegistry.register(
+            MSG_STATE_CACHE_KEY,
+            this.msgToggleRepository::getPrivateChatState,
+            Duration.ofMinutes(10)


move to private static variable, maybe?

CitralFlo · 2025-10-14T07:53:41Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/msg/messages/ENMsgMessages.java

    public Notice socialSpyEnable = Notice.chat("<green>► <white>SocialSpy has been {STATE}<white>!");
    public Notice socialSpyDisable = Notice.chat("<red>► <white>SocialSpy has been {STATE}<white>!");

+    @Comment("# Formatowanie placeholderów")


Gemini is right 👍

Rollczi

1 komentarz na razie bo może być dużo do przerobienia

Rollczi · 2025-10-21T18:59:47Z

...nalcore-core/src/main/java/com/eternalcode/core/feature/msg/toggle/MsgToggleServiceImpl.java


    private final MsgToggleRepository msgToggleRepository;
-    private final ConcurrentHashMap<UUID, MsgState> cachedToggleStates;
+    private final AsyncPlaceholderCacheRegistry cacheRegistry;


słabe rozwiązanie, najlepiej jakby tego cache nie było w repo + nie było tutaj nic placeholderach, bo to tworzy dziwne zależności. Raczej placeholdery widzą API serisów i tyle. Serwisy nie powinny widzieć placeholders

P1otrulla added 8 commits October 12, 2025 12:20

Enhance AsyncPlaceholderCacheController with event-based cache inva…

a8c6659

…lidation * Add `Subscribe` annotations to invalidate cache on `EternalReloadEvent` and `EternalShutdownEvent`.

Refactor AsyncPlaceholderCacheController event methods

2633ef6

* Rename `onEternalDisable` to `onDisable` and `onReload`.

the bomb was defused xd

dc6f963

P1otrulla requested a review from a team as a code owner October 12, 2025 11:27

P1otrulla linked an issue Oct 12, 2025 that may be closed by this pull request

Add a placeholder for the /msgtoggle command. i.e: %eternalcore_msgtoggle% #930

Open

P1otrulla requested review from CitralFlo, Jakubk15, igoyek, imDMK and vLuckyyy October 12, 2025 11:27

gemini-code-assist bot reviewed Oct 12, 2025

View reviewed changes

P1otrulla added 2 commits October 12, 2025 13:30

Follow gemini comment

aa360dd

Follow gemini comment

a128154

P1otrulla changed the title ~~Msg placeholders & Async placeholder for future~~ GH-930 Msg placeholders & Async placeholder for future Oct 12, 2025

P1otrulla requested a review from noyzys October 12, 2025 12:12

noyzys reviewed Oct 12, 2025

View reviewed changes

Jakubk15 reviewed Oct 12, 2025

View reviewed changes

Jakubk15 approved these changes Oct 12, 2025

View reviewed changes

P1otrulla requested a review from Qbiterv October 12, 2025 22:37

vLuckyyy reviewed Oct 13, 2025

View reviewed changes

imDMK approved these changes Oct 14, 2025

View reviewed changes

CitralFlo reviewed Oct 14, 2025

View reviewed changes

vLuckyyy requested review from Rollczi and removed request for Qbiterv October 19, 2025 12:38

Rollczi requested changes Oct 21, 2025

View reviewed changes

igoyek approved these changes Oct 22, 2025

View reviewed changes

	player -> String.valueOf(this.msgService.isSpy(player.getUniqueId()))
	player -> Boolean.toString(this.msgService.isSpy(player.getUniqueId()))

	private String loading = "<yellow>Ładowanie...";
	private String loading = "<yellow>Wczytywanie...";

Uh oh!

Uh oh!

GH-930 Msg placeholders & Async placeholder for future #1203

Are you sure you want to change the base?

GH-930 Msg placeholders & Async placeholder for future #1203

Conversation

P1otrulla commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noyzys Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noyzys Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noyzys Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noyzys left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rollczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

P1otrulla commented Oct 12, 2025 •

edited

Loading

noyzys Oct 12, 2025 •

edited

Loading

noyzys Oct 12, 2025 •

edited

Loading

noyzys Oct 12, 2025 •

edited

Loading

noyzys left a comment •

edited

Loading