Skip to content

Chat private messages (protocol v2) - #15291

Open
Fanfblrik wants to merge 3 commits into
yairm210:masterfrom
Fanfblrik:feature/chat-private-messages
Open

Chat private messages (protocol v2)#15291
Fanfblrik wants to merge 3 commits into
yairm210:masterfrom
Fanfblrik:feature/chat-private-messages

Conversation

@Fanfblrik

@Fanfblrik Fanfblrik commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Chat protocol v2: optional outbound userId on chat messages (null = public broadcast). Public chat unchanged when omitted.
  • Server maps websocket sessions to authenticated user ids and delivers private messages only to sender + recipient sessions subscribed to the game. Relayed payload uses private: true (no recipient id/name on the wire). chatVersion is 2 when chat is enabled.
  • ChatPopup shows a To: selector (Everyone + other human major civs) when chatVersion >= 2. Private lines render as Sender (private): text. Spectators / Unknown keep Everyone-only.
  • ignoreUnknownKeys enabled on client/server chat JSON for forward compatibility.
  • Verified against local UncivServer jar and UncivServer.xyz (chatVersion: 2, private delivery + sender echo).

Conflict note: preferred rebase order with sibling chat PRs is #15289#15290#15291. When rebasing onto #15290, fold isPrivate into existing Chat.Line (history branch already uses that field name) and keep #15289 coloring in ChatPopup. Happy to rebase when conflicts appear.

Test plan

  • .\gradlew :core:compileKotlin :server:compileKotlin
  • Against a v2 server jar: open chat → recipient list appears; public messages still visible to all
  • A → B private: only A and B see it; C does not
  • Against chatVersion 1 / 0: no recipient selector; public chat works as before
  • Unknown/spectator: no private targets (Everyone only)
  • Sender (private) display is clear enough without recipient name in the log
  • UncivServer.xyz: public + private delivery; sender echo present

Route optional toPlayerId on the server to sender+recipient sessions only, bump chatVersion to 2, and add a recipient selector in ChatPopup when the server supports it.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Fanfblrik

Fanfblrik commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Conflict note (merge order is not under our control — happy to rebase when asked):

Preferred order if more than one of these lands, to minimize rebase pain:

  1. Chat: nation-colored names, white body #15289 — ChatPopup styling only (no ChatStore shape change)
  2. Persist multiplayer chat history on this device (opt-in) #15290Chat.Line + opt-in local history (isPrivate reserved for v2)
  3. Chat private messages (protocol v2) #15291 — private messages (protocol v2: outbound userId, inbound private: true)

After reworks: #15290 and #15291 both reshape message storage (Chat.Line / isPrivate vs ChatMessageEntry / isPrivate). #15291 no longer uses toCivName. Out-of-order merges need a careful rebase; ping me and I will fix conflicts.

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Happy to rebase / fix merge conflicts (and any review feedback) — just say the word.

@touhidurrr

touhidurrr commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

is toCivName really necessary?
For server if userId is provided then the server should send the message to designated userId only.
When a private message is sent, the server can just add private: true to say that this message is only for you. And when personal == true, you can display (private) after civName
A full userId is not necessary.

Comment thread core/src/com/unciv/logic/multiplayer/chat/ChatStore.kt Outdated
Comment thread core/src/com/unciv/logic/multiplayer/chat/ChatWebSocket.kt Outdated
Comment thread core/src/com/unciv/logic/multiplayer/chat/ChatWebSocket.kt Outdated
@touhidurrr

Copy link
Copy Markdown
Contributor

So, mainly too many unnecessary properties.

@touhidurrr

Copy link
Copy Markdown
Contributor

Ok @Fanfblrik , I have added your protocol v2 support in UncivServer.xyz. The gist of it is below:

  1. To send a private message include userId field to payload
  2. If a message is a private message, the server sent message has private: true on, otherwise private maybe omitted or be false.

Kindly test with the server and see if it works.

@touhidurrr

Copy link
Copy Markdown
Contributor

Also, I think you need to add a 4th pull request or modify this to add UI support for sending private messages.

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

I was just doing some testing.

image

Address review: drop toCivName/toPlayerId echo; render (private); left-align To: selector.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Addressed review feedback in 3cd1e0a82:

  • Outbound: userId for private routing (null = public)
  • Inbound: private: true only — no toCivName / recipient id echo
  • UI: Sender (private): …; To: selector left-aligned with its label
  • Local two-client test: public + private delivery looks good; (private) without recipient name is fine in the log

@touhidurrr

Copy link
Copy Markdown
Contributor

Addressed review feedback in 3cd1e0a82:

  • Outbound: userId for private routing (null = public)
  • Inbound: private: true only — no toCivName / recipient id echo
  • UI: Sender (private): …; To: selector left-aligned with its label
  • Local two-client test: public + private delivery looks good; (private) without recipient name is fine in the log

Does UncivServer.xyz work?

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

I tested against a locally built Unciv server jar from this PR branch, not against UncivServer.xyz.

Local server matches the protocol we settled on: outbound userId for private routing, inbound private: true, and /isalive reports chatVersion: 2 so the To: selector appears.

Looking at the current UncivServer.xyz side, a few gaps would block the same client path:

  1. /isalive still reports chatVersion: 1 — the client only shows the To: selector when chatVersion >= 2
  2. In publishChat, delete chat.userId runs before the Preview lookup / user: publish, so the target id is already gone
  3. Private publish goes only to user:{recipient} — no echo to the sender; our client displays a line only after the server relays it back

Once those are aligned, I can retest on UncivServer.xyz.

@touhidurrr

Copy link
Copy Markdown
Contributor

Do we really need to increment chatVersion? Since the changes here is not breaking.

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

If I leave it at 1, how will the client distinguish a server with real private routing from an old Unciv jar?

@touhidurrr

Copy link
Copy Markdown
Contributor

If I leave it at 1, how will the client distinguish a server with real private routing from an old Unciv jar?

Ok I will increment chatVersion for you later this night inshallah.

@touhidurrr

touhidurrr commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

chatVersion is now 2 with touhidurrr/UncivServer.xyz@8ddf054

Can you check if it works now?

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Verified against a local UncivServer.xyz checkout (current main, /isalive reports chatVersion: 2).

Setup: two registered users, packed Preview+game uploaded, both WS clients joined the game successfully.

Results:

  • Public chat: delivered to both clients
  • Private chat (userId set to the other player): does not work

Sender only got a server error:
User not found in game <gameId>

Recipient got nothing; no private: true payload.

Cause looks to be in publishChat: delete chat.userId runs before the Preview exists check / user: publish, so the target id is already gone when those run.

@touhidurrr

touhidurrr commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Sender only got a server error:
User not found in game

Cause looks to be in publishChat: delete chat.userId runs before the Preview exists check / user: publish, so the target id is already gone when those run.

image

Seems like I was deleting userId before using it. -_-

Fixed in touhidurrr/UncivServer.xyz@832e7b2, Should be working now.

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Retested against a local UncivServer.xyz checkout at v4.66.4 / 832e7b2 (chatVersion: 2).

Same setup as before: two registered users, packed Preview+game uploaded, both WS clients joined successfully.

Results:

  • Public chat: delivered to both clients
  • Private chat (A → B): delivered to B with "private": true
  • No more User not found in game … on the sender
  • Sender does not get an echo (expected for UncivServer.xyz; Unciv jar still echoes sender+recipient)

Looks good on the xyz side — thanks for the fix.

@touhidurrr

touhidurrr commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Sender does not get an echo.

I guess that is a problem. How do we fix this?

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Already handled on the Unciv jar side in this PR: publishPrivate delivers to both sender and recipient sessions (same payload with "private": true).

So for UncivServer.xyz the fix is the same idea — when routing a private message, publish to the target and the sender, not only the target. Then the sender’s chat log gets the echo over the wire like everyone else; no client-side special case needed.

@touhidurrr

Copy link
Copy Markdown
Contributor

Sender does not get an echo (expected for UncivServer.xyz; Unciv jar still echoes sender+recipient)

Ok. Fixed. It should work find now. Although it feels like something is missing.

Anyways, can you debug another issue when you are at it? It seems like when the Server sends an error message it is printed to chat but then somehow the client gets disconnected. Can you debug it also?

@Fanfblrik

Copy link
Copy Markdown
Contributor Author

Checked this against the Unciv jar chat server from this PR (chatVersion: 2).

Does not reproduce for in-session chat errors — the socket stays up:

  1. chat without jointype: "error" "You are not subscribed to this channel!" → then join still succeeds on the same connection
  2. Private chat with a bad userIdtype: "error" "Invalid userId!" → then a public chat still relays on the same connection

On the client, Response.Error only goes to the chat UI; it does not close or restart the WebSocket by itself. A disconnect after an error only happens if the server closes the socket afterward.

The one jar path that does send error and then close is auth failure (Authentication failed! / missing auth) — that close is intentional. If you are seeing disconnect after a different error payload on UncivServer.xyz, that would be on the xyz side (or please share the exact error.message + steps and I can dig further).

@touhidurrr touhidurrr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My requirements for backend is done. The UI can be better but we can gradually improve it. Ok from me.

Fanfblrik added a commit to Fanfblrik/Unciv that referenced this pull request Aug 4, 2026
Replace reserved toCivName with isPrivate so local history matches yairm210#15291 inbound private: true when that PR lands.

Co-authored-by: Cursor <cursoragent@cursor.com>
Fanfblrik added a commit to Fanfblrik/Unciv that referenced this pull request Aug 4, 2026
Replace reserved toCivName with isPrivate so local history matches yairm210#15291 inbound private: true when that PR lands.

Co-authored-by: Cursor <cursoragent@cursor.com>
Fanfblrik added a commit to Fanfblrik/Unciv that referenced this pull request Aug 5, 2026
Replace reserved toCivName with isPrivate so local history matches yairm210#15291 inbound private: true when that PR lands.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Resolve ChatPopup conflict: keep master's ColorMarkup nation colors
and retain private-message recipient UI / isPrivate display.
Keep pre-merge workflow files so the push does not require workflow scope.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot removed the Conflicts label Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Conflicts have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants