Add Chat-Bubbles Feature#29
Conversation
paulikauro
left a comment
paulikauro
left a comment
There was a problem hiding this comment.
Cool! One general comment: "Chat-Bubble" -> "chat bubble". Some simplifications and a few things to think about in code comments
paulikauro
left a comment
paulikauro
left a comment
There was a problem hiding this comment.
Nice work! There are some ACF things that would be good to address at this point:
- commands should take
target: OnlinePlayer Bubbleshould get its own ACF context (like mentioned previously)- additionally, the
Bubblecontext should have a flag for checking bubble ownership. Then you can simply do@Flag(FLAG_BUBBLE_OWNED) bubble: Bubblewhenever a command needs the sender to be an owner of a bubble (delete, kick, setPrivate). (ThereFLAG_BUBBLE_OWNEDis just a constant string; compare to egUserCache.COMPLETION_USERNAMES, except this one doesn't have to be public)
Once implemented, these will make the commands shorter and nicer to read, plus there'll be less duplication :)
|
Review requested! |
paulikauro
left a comment
paulikauro
left a comment
There was a problem hiding this comment.
Nice, some small details still and a few bigger suggestions
`sender` is never Player, so the else branch was always taken. Now every error will also get the info prefix added to it. In the future, a sendError may be useful to add.
|
I'd like to formally request a review from the great PaukkuPalikka again |
| ?: throw ChattoreException("Target is not in a Chat-Bubble!") | ||
|
|
||
| if (!bubble.isInvited(sender.uniqueId) && bubble.isPrivate) | ||
| if (sender.uniqueId !in bubble.invitedPlayers && bubble.isPrivate) |
There was a problem hiding this comment.
This really doesn't actually matter, it's more of a style thing at this point, but flipping the operands of && here will be marginally more efficient because of short-circuiting
| luckPerms: LuckPerms, | ||
| chatBroadcastFormat: String, | ||
| formatConfig: FormatConfig, | ||
| getBubbleManager: () -> BubbleManager, |
There was a problem hiding this comment.
One possibly decent-ish way to break this dependency is to have a public val excludedFromGlobalChat: MutableSet<UUID> = ConcurrentHashMap.newKeySet() in Messenger, then add/remove players from that as they join/leave bubbles (if they have the setting enabled). Probably want helper methods in BubbleCommand for check setting + (add/remove) combos (and actually, remove doesn't even need to check the setting). The goal would be to replace each call to messenger.setCacheDirty() with a call to one of those new methods instead. (When popping a bubble, you can just excludedFromGlobalChat.removeAll(bubble.players) 🤔). The handler for /showglobalchat will have some extra logic unfortunately, I think. But after this, the new setting query (getAllGlobalChatEnabled) is no longer needed and the ShowGlobalChatInBubble setting can be made private.
... so at least this is a possibility, it's probably not the best solution though 🤔 Maybe worth trying. I think the final solution will be somewhat similar to this though.
2 participants
This PR adds what the title says 🧋