-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-930 Msg placeholders & Async placeholder for future #1203
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
base: master
Are you sure you want to change the base?
Changes from all commits
b3764bd
af593f1
1f98392
cf7151e
1407a28
a8c6659
2633ef6
dc6f963
aa360dd
a128154
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,104 @@ | ||||||
| package com.eternalcode.core.feature.msg; | ||||||
|
|
||||||
| import com.eternalcode.core.feature.msg.toggle.MsgState; | ||||||
| import com.eternalcode.core.feature.msg.toggle.MsgToggleRepository; | ||||||
| import com.eternalcode.core.injector.annotations.Inject; | ||||||
| import com.eternalcode.core.injector.annotations.component.Controller; | ||||||
| import com.eternalcode.core.placeholder.PlaceholderRegistry; | ||||||
| import com.eternalcode.core.placeholder.PlaceholderReplacer; | ||||||
| import com.eternalcode.core.placeholder.cache.AsyncPlaceholderCacheRegistry; | ||||||
| import com.eternalcode.core.placeholder.cache.AsyncPlaceholderCached; | ||||||
| import com.eternalcode.core.publish.Subscribe; | ||||||
| import com.eternalcode.core.publish.event.EternalInitializeEvent; | ||||||
| import com.eternalcode.core.translation.Translation; | ||||||
| import com.eternalcode.core.translation.TranslationManager; | ||||||
| import java.time.Duration; | ||||||
| import java.util.UUID; | ||||||
| import java.util.function.Function; | ||||||
|
|
||||||
| @Controller | ||||||
| public class MsgPlaceholderSetup { | ||||||
|
|
||||||
| public static final String MSG_STATE_CACHE_KEY = "msg_state"; | ||||||
|
|
||||||
| private final MsgService msgService; | ||||||
| private final MsgToggleRepository msgToggleRepository; | ||||||
| private final TranslationManager translationManager; | ||||||
| private final AsyncPlaceholderCacheRegistry cacheRegistry; | ||||||
|
|
||||||
| @Inject | ||||||
| MsgPlaceholderSetup( | ||||||
| MsgService msgService, | ||||||
| MsgToggleRepository msgToggleRepository, | ||||||
| TranslationManager translationManager, | ||||||
| AsyncPlaceholderCacheRegistry cacheRegistry | ||||||
| ) { | ||||||
| this.msgService = msgService; | ||||||
| this.msgToggleRepository = msgToggleRepository; | ||||||
| this.translationManager = translationManager; | ||||||
| this.cacheRegistry = cacheRegistry; | ||||||
| } | ||||||
|
|
||||||
| @Subscribe(EternalInitializeEvent.class) | ||||||
| void setUpPlaceholders(PlaceholderRegistry placeholderRegistry) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proponowalbym poprawic nazewnictwo metody obecnie jest to ogolnikowe a nie jasne ze chodzi o rejestracje placeholderow, czyli jezeli jest kompozycja na registry dajemy |
||||||
| Translation translation = this.translationManager.getMessages(); | ||||||
|
|
||||||
| AsyncPlaceholderCached<MsgState> stateCache = this.cacheRegistry.register( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| MSG_STATE_CACHE_KEY, | ||||||
| this.msgToggleRepository::getPrivateChatState, | ||||||
| Duration.ofMinutes(10) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move to private static variable, maybe? |
||||||
| ); | ||||||
|
|
||||||
| placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of( | ||||||
| "socialspy_status", | ||||||
| player -> String.valueOf(this.msgService.isSpy(player.getUniqueId())) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. microoptimizations: use Boolean#toString instead of String.valueOf
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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));
} |
||||||
| )); | ||||||
|
|
||||||
| placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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();
} |
||||||
| "socialspy_status_formatted", | ||||||
| player -> { | ||||||
| UUID uuid = player.getUniqueId(); | ||||||
| return this.msgService.isSpy(uuid) | ||||||
| ? translation.msg().placeholders().socialSpyEnabled() | ||||||
| : translation.msg().placeholders().socialSpyDisabled(); | ||||||
| } | ||||||
| )); | ||||||
|
|
||||||
| placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of( | ||||||
| "msg_status", | ||||||
| player -> this.formatMsgState( | ||||||
| player.getUniqueId(), | ||||||
| stateCache, | ||||||
| translation, | ||||||
| state -> state.name().toLowerCase() | ||||||
| ) | ||||||
| )); | ||||||
|
|
||||||
| placeholderRegistry.registerPlaceholder(PlaceholderReplacer.of( | ||||||
| "msg_status_formatted", | ||||||
| player -> this.formatMsgState( | ||||||
| player.getUniqueId(), | ||||||
| stateCache, | ||||||
| translation, | ||||||
| state -> state == MsgState.ENABLED | ||||||
| ? translation.msg().placeholders().msgEnabled() | ||||||
| : translation.msg().placeholders().msgDisabled() | ||||||
| ) | ||||||
| )); | ||||||
| } | ||||||
|
|
||||||
| private String formatMsgState( | ||||||
| UUID uuid, | ||||||
| AsyncPlaceholderCached<MsgState> stateCache, | ||||||
| Translation translation, | ||||||
| Function<MsgState, String> formatter | ||||||
| ) { | ||||||
| MsgState state = stateCache.getCached(uuid); | ||||||
|
|
||||||
| if (state == null) { | ||||||
| return translation.msg().placeholders().loading(); | ||||||
| } | ||||||
|
|
||||||
| return formatter.apply(state); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,4 +36,22 @@ public class ENMsgMessages extends OkaeriConfig implements MsgMessages { | |
| 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") | ||
P1otrulla marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gemini is right 👍 |
||
| public ENPlaceholders placeholders = new ENPlaceholders(); | ||
|
|
||
| @Getter | ||
| @Accessors(fluent = true) | ||
| public static class ENPlaceholders extends OkaeriConfig implements MsgMessages.Placeholders { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| private String loading = "<yellow>Loading..."; | ||
|
|
||
| private String msgEnabled = "<green>Enabled"; | ||
| private String msgDisabled = "<red>Disabled"; | ||
| private String socialSpyEnabled = "<green>Enabled"; | ||
| private String socialSpyDisabled = "<red>Disabled"; | ||
| } | ||
|
|
||
| public ENPlaceholders placeholders() { | ||
| return this.placeholders; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -37,4 +37,21 @@ public class PLMsgMessages extends OkaeriConfig implements MsgMessages { | |||||
| public Notice otherMessagesDisabled = Notice.chat("<green>► <white>Wiadomości prywatne zostały <red>wyłączone <white>dla gracza <green>{PLAYER}<white>!"); | ||||||
| public Notice otherMessagesEnabled = Notice.chat("<green>► <white>Wiadomości prywatne zostały <green>włączone <white>dla gracza <green>{PLAYER}<white>!"); | ||||||
|
|
||||||
| @Comment("# Formatowanie placeholderów") | ||||||
| public PLPlaceholders placeholders = new PLPlaceholders(); | ||||||
|
|
||||||
| @Getter | ||||||
| @Accessors(fluent = true) | ||||||
| public static class PLPlaceholders extends OkaeriConfig implements MsgMessages.Placeholders { | ||||||
| private String loading = "<yellow>Ładowanie..."; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Wczytywanie (w kontekście danych) tutaj brzmi lepiej, ładowanie bardziej się kojarzy z jakimiś intensywnymi zadaniami |
||||||
|
|
||||||
| private String msgEnabled = "<green>Włączone"; | ||||||
| private String msgDisabled = "<red>Wyłączone"; | ||||||
| private String socialSpyEnabled = "<green>Włączony"; | ||||||
| private String socialSpyDisabled = "<red>Wyłączony"; | ||||||
| } | ||||||
|
|
||||||
| public PLPlaceholders placeholders() { | ||||||
| return this.placeholders; | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,42 @@ | ||
| package com.eternalcode.core.feature.msg.toggle; | ||
|
|
||
| import com.eternalcode.core.feature.msg.MsgPlaceholderSetup; | ||
| import com.eternalcode.core.injector.annotations.Inject; | ||
| import com.eternalcode.core.injector.annotations.component.Service; | ||
| import com.eternalcode.core.placeholder.cache.AsyncPlaceholderCacheRegistry; | ||
| import com.eternalcode.core.placeholder.cache.AsyncPlaceholderCached; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Consumer; | ||
|
|
||
| @Service | ||
| class MsgToggleServiceImpl implements MsgToggleService { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rozbilbym to na kilka innych komponentow z ich odpowiedzialnoscia z kosztem korzysci,
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); |
||
|
|
||
| private final MsgToggleRepository msgToggleRepository; | ||
| private final ConcurrentHashMap<UUID, MsgState> cachedToggleStates; | ||
| private final AsyncPlaceholderCacheRegistry cacheRegistry; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| @Inject | ||
| MsgToggleServiceImpl(MsgToggleRepository msgToggleRepository) { | ||
| this.cachedToggleStates = new ConcurrentHashMap<>(); | ||
| MsgToggleServiceImpl(MsgToggleRepository msgToggleRepository, AsyncPlaceholderCacheRegistry cacheRegistry) { | ||
| this.msgToggleRepository = msgToggleRepository; | ||
|
|
||
| this.cacheRegistry = cacheRegistry; | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public CompletableFuture<MsgState> getState(UUID playerUniqueId) { | ||
| if (this.cachedToggleStates.containsKey(playerUniqueId)) { | ||
| return CompletableFuture.completedFuture(this.cachedToggleStates.get(playerUniqueId)); | ||
| } | ||
|
|
||
| return this.msgToggleRepository.getPrivateChatState(playerUniqueId); | ||
| return this.msgToggleRepository.getPrivateChatState(playerUniqueId) | ||
| .thenApply(state -> { | ||
| this.withCache(cache -> cache.update(playerUniqueId, state)); | ||
| return state; | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<Void> setState(UUID playerUniqueId, MsgState state) { | ||
| this.cachedToggleStates.put(playerUniqueId, state); | ||
| this.withCache(cache -> cache.update(playerUniqueId, state)); | ||
|
|
||
| return this.msgToggleRepository.setPrivateChatState(playerUniqueId, state) | ||
| .exceptionally(throwable -> { | ||
| this.cachedToggleStates.remove(playerUniqueId); | ||
| this.withCache(cache -> cache.invalidate(playerUniqueId)); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
@@ -48,4 +49,9 @@ public CompletableFuture<MsgState> toggleState(UUID playerUniqueId) { | |
| .thenApply(aVoid -> newState); | ||
| }); | ||
| } | ||
|
|
||
| private void withCache(Consumer<AsyncPlaceholderCached<MsgState>> action) { | ||
| this.cacheRegistry.<MsgState>get(MsgPlaceholderSetup.MSG_STATE_CACHE_KEY) | ||
| .ifPresent(action); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.eternalcode.core.placeholder.cache; | ||
|
|
||
| import com.eternalcode.core.injector.annotations.Inject; | ||
| import com.eternalcode.core.injector.annotations.component.Controller; | ||
| import com.eternalcode.core.publish.Subscribe; | ||
| import com.eternalcode.core.publish.event.EternalReloadEvent; | ||
| import com.eternalcode.core.publish.event.EternalShutdownEvent; | ||
| import org.bukkit.event.EventHandler; | ||
| import org.bukkit.event.EventPriority; | ||
| import org.bukkit.event.Listener; | ||
| import org.bukkit.event.player.PlayerQuitEvent; | ||
|
|
||
| @Controller | ||
| class AsyncPlaceholderCacheController implements Listener { | ||
|
|
||
| private final AsyncPlaceholderCacheRegistry cacheRegistry; | ||
|
|
||
| @Inject | ||
| AsyncPlaceholderCacheController(AsyncPlaceholderCacheRegistry cacheRegistry) { | ||
| this.cacheRegistry = cacheRegistry; | ||
| } | ||
|
|
||
| @EventHandler(priority = EventPriority.MONITOR) | ||
| void onPlayerQuit(PlayerQuitEvent event) { | ||
| this.cacheRegistry.invalidatePlayer(event.getPlayer().getUniqueId()); | ||
| } | ||
|
|
||
| @Subscribe | ||
| void onDisable(EternalShutdownEvent event) { | ||
| this.cacheRegistry.invalidateAll(); | ||
| } | ||
|
|
||
| @Subscribe | ||
| void onReload(EternalReloadEvent event) { | ||
| this.cacheRegistry.invalidateAll(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package com.eternalcode.core.placeholder.cache; | ||
|
|
||
| import com.eternalcode.core.injector.annotations.Inject; | ||
| import com.eternalcode.core.injector.annotations.component.Service; | ||
| import java.time.Duration; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Function; | ||
|
|
||
| @Service | ||
| public class AsyncPlaceholderCacheRegistry { | ||
|
|
||
| private static final Duration DEFAULT_EXPIRE_DURATION = Duration.ofMinutes(30); | ||
|
|
||
| private final Map<String, AsyncPlaceholderCached<?>> caches = new ConcurrentHashMap<>(); | ||
|
|
||
| @Inject | ||
| public AsyncPlaceholderCacheRegistry() { | ||
| } | ||
|
|
||
| public <T> AsyncPlaceholderCached<T> register(String key, Function<UUID, CompletableFuture<T>> loader) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moze byc taka sytuacja ze key juz istnieje w mapie ale zostanie nadpisany bez ostrzezenia to mamy niestety utrate cache, Wszystko tutaj mozna rozbudowac i rozszerzyc na jakas konfiguracje jezeli potrzeba w obecnym stanie jezeli nie - zostawiamy i tyle |
||
| return this.register(key, loader, DEFAULT_EXPIRE_DURATION); | ||
| } | ||
|
|
||
| public <T> AsyncPlaceholderCached<T> register(String key, Function<UUID, CompletableFuture<T>> loader, Duration expireAfterWrite) { | ||
| AsyncPlaceholderCached<T> cache = new AsyncPlaceholderCached<>(loader, expireAfterWrite); | ||
| this.caches.put(key, cache); | ||
| return cache; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> Optional<AsyncPlaceholderCached<T>> get(String key) { | ||
| return Optional.ofNullable((AsyncPlaceholderCached<T>) this.caches.get(key)); | ||
| } | ||
|
|
||
| public void invalidatePlayer(UUID uuid) { | ||
| this.caches.values().forEach(cache -> cache.invalidate(uuid)); | ||
| } | ||
|
|
||
| public void invalidateAll() { | ||
| this.caches.values().forEach(AsyncPlaceholderCached::clear); | ||
| } | ||
|
|
||
| public void unregister(String key) { | ||
| AsyncPlaceholderCached<?> cache = this.caches.remove(key); | ||
| if (cache != null) { | ||
| cache.clear(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package com.eternalcode.core.placeholder.cache; | ||
|
|
||
| import com.google.common.cache.Cache; | ||
| import com.google.common.cache.CacheBuilder; | ||
| import java.time.Duration; | ||
| import java.util.Map; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Function; | ||
|
|
||
| public class AsyncPlaceholderCached<T> { | ||
|
|
||
| private final Cache<UUID, T> cache; | ||
| private final Function<UUID, CompletableFuture<T>> loader; | ||
| private final Map<UUID, CompletableFuture<T>> loading = new ConcurrentHashMap<>(); | ||
|
|
||
| public AsyncPlaceholderCached(Function<UUID, CompletableFuture<T>> loader, Duration expireAfterWrite) { | ||
| this.loader = loader; | ||
| this.cache = CacheBuilder.newBuilder() | ||
| .expireAfterWrite(expireAfterWrite) | ||
| .build(); | ||
| } | ||
|
|
||
| public T getCached(UUID uuid) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return null - jak value nie jest jeszcze dostepna to wymaga obslugi null majac ze soba ryzuko rzucenia NPE.
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);
});
} |
||
| T cached = this.cache.getIfPresent(uuid); | ||
| if (cached != null) { | ||
| return cached; | ||
| } | ||
|
|
||
| this.loading.computeIfAbsent(uuid, key -> | ||
| this.loader.apply(key).whenComplete((value, throwable) -> { | ||
| if (value != null) { | ||
| this.cache.put(key, value); | ||
| } | ||
| this.loading.remove(key); | ||
| }) | ||
| ); | ||
|
|
||
| return null; | ||
| } | ||
| public void update(UUID uuid, T value) { | ||
| this.cache.put(uuid, value); | ||
| } | ||
|
|
||
| public void invalidate(UUID uuid) { | ||
| this.cache.invalidate(uuid); | ||
| } | ||
|
|
||
| public void clear() { | ||
| this.cache.invalidateAll(); | ||
| } | ||
|
|
||
| public boolean contains(UUID uuid) { | ||
| return this.cache.getIfPresent(uuid) != 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.
Tutaj mozna byloby rozwazyc wrzucenie dodatkowych stalych globalnych na sama mozliwosc kontrolowania centralnia czas cache zamiast overidowac w metodzie rejeestracji