Conversation
…improved menu options
…estions and error messages
…ment to use simple names
There was a problem hiding this comment.
Pull request overview
Adds a per-player “player visibility” setting (GUI + commands) backed by surf-settings, and restructures event-server commands/permissions while introducing a config toggle to enable/disable the feature.
Changes:
- Add player visibility states, persistence hook, join handling, and a GUI/command interface to change visibility.
- Replace
/changeeventserverstatewith/eventserver changestateand add/eventserver reload, including new permission nodes. - Extend event server config with
playerVisibilityEnabledand add soft dependency wiring forsurf-settings-paper.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/menu/gui-util.kt | Adds InventoryFramework GUI helpers (outline, close button, click sound, click-cancel). |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/menu/PlayerVisibilityMenu.kt | New chest GUI to select a visibility state and persist on close. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/listener/PlayerConnectionListener.kt | Applies visibility logic on player join. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/command/argument/PlayerVisibilityArgument.kt | Adds CommandAPI argument parsing + suggestions for visibility state. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/command/PlayerVisibilityCommand.kt | Adds /playervisibility command with GUI, info, and state subcommand. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/SettingsHook.kt | Integrates surf-settings for creating/saving/loading the player visibility setting. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/PlayerVisibilityState.kt | Introduces the visibility enum (ALL/VIP/FRIENDS/NONE). |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/settings/PlayerVisibilityService.kt | Implements hide/show logic and join-state handling, including VIP permission filtering. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/permission/PermissionRegistry.kt | Adds new command and visibility permissions; renames event-server permission nodes. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/config/EventServerConfig.kt | Adds playerVisibilityEnabled feature toggle. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/command/EventServerStateChangeCommand.kt | Removes legacy standalone state-change command. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/command/EventServerCommand.kt | Adds new /eventserver command with changestate + reload. |
| surf-event-base/surf-event-base-paper/src/main/kotlin/dev/slne/surf/event/base/paper/PaperMain.kt | Registers new commands/listener and initializes settings integration on enable. |
| surf-event-base/surf-event-base-paper/build.gradle.kts | Adds soft dependency for surf-settings-paper and new dependencies. |
| gradle.properties | Bumps project version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| fun getSelectedState(playerUuid: UUID) = | ||
| surfSettingsApi.getPlayerSetting(playerUuid, "player-visibility") | ||
| ?.let { PlayerVisibilityState.valueOf(it.settingValue) } |
There was a problem hiding this comment.
PlayerVisibilityState.valueOf(it.settingValue) will throw an IllegalArgumentException if the stored value is missing/invalid (e.g., manual edits, old enum name). This can crash callers. Prefer a safe parse (e.g., runCatching { valueOf(...) }.getOrDefault(ALL) or a firstOrNull over entries) and fall back to ALL.
| ?.let { PlayerVisibilityState.valueOf(it.settingValue) } | |
| ?.let { | |
| runCatching { PlayerVisibilityState.valueOf(it.settingValue) } | |
| .getOrElse { PlayerVisibilityState.ALL } | |
| } |
| if (hasSettingsHook()) { | ||
| runBlocking { | ||
| settingsHook.createSettings() | ||
| } | ||
| } |
There was a problem hiding this comment.
runBlocking in onEnable() can block the server startup thread while createSettings() runs (likely doing I/O). Since you already use coroutines (SuspendingJavaPlugin / plugin.launch), prefer launching a coroutine instead of blocking, or move this work to an async enable hook if available.
| } | ||
| } | ||
|
|
||
| fun ChestGui.withOutClicks() = apply { |
There was a problem hiding this comment.
Function name withOutClicks() is unclear/awkward (reads like “with out clicks”). Consider renaming to something like withoutClicks(), disableClicks(), or cancelAllClicks() for readability.
| fun ChestGui.withOutClicks() = apply { | |
| fun ChestGui.withoutClicks() = apply { |
| override fun update() { | ||
| pane.addItem(GuiItem(buildItem(Material.GREEN_DYE) { | ||
| displayName { | ||
| note("Alle Spieler anzeigen") | ||
|
|
There was a problem hiding this comment.
update() mutates pane by adding items every time it runs, but nothing clears/overwrites the previous contents. This can lead to duplicated items/handlers or stale state depending on InventoryFramework internals. Consider building the pane once (or calling a clear/remove API on the pane) before re-adding items, and only updating item meta when selectedState changes.
|
|
||
| addPane(pane) | ||
|
|
||
| super.update() | ||
| } |
There was a problem hiding this comment.
addPane(pane) is called inside update(), so each update() call will register the same pane again. This can cause the GUI to render the pane multiple times and may duplicate click handling. Add the pane once during init and let update() only modify pane contents, then call super.update().
| playerVisibilityService.refreshState( | ||
| player as? Player | ||
| ?: error("Only players can have a player visibility state.") | ||
| ) |
There was a problem hiding this comment.
The menu API accepts HumanEntity, but on close it hard-fails for non-Player via error(...). To avoid a runtime crash path, make the menu/show function take Player (or handle non-player entities gracefully) and remove the cast + error call.
| // selectedState = PlayerVisibilityState.FRIENDS | ||
| // it.whoClicked.playClickSound() | ||
| // it.whoClicked.sendText { | ||
| // appendSuccessPrefix() | ||
| // success("Du siehst jetzt nur Freunde.") | ||
| // } | ||
| // update() | ||
|
|
There was a problem hiding this comment.
There is a block of commented-out logic in the FRIENDS click handler. Leaving dead code in production makes it harder to maintain and review behavior changes later. Please remove the commented code (or gate it behind a feature flag/config if it’s intended to return soon).
| // selectedState = PlayerVisibilityState.FRIENDS | |
| // it.whoClicked.playClickSound() | |
| // it.whoClicked.sendText { | |
| // appendSuccessPrefix() | |
| // success("Du siehst jetzt nur Freunde.") | |
| // } | |
| // update() |
|
|
||
| PlayerVisibilityState.FRIENDS -> showPlayer(it, player) | ||
| } | ||
| } |
There was a problem hiding this comment.
handleJoin only applies other players’ visibility settings to the joining player (i.e., whether others can see the joiner). It does not apply the joiner’s own selected visibility state to what they see, so a player with NONE/VIP will still see everyone after reconnect until they change state again. Consider calling refreshState(player) during join handling (after the loop).
| } | |
| } | |
| refreshState(player) |
05613b9
No description provided.