[Mosch0512]

What this does

The server groups of the login screen (Valhalla, Helheim, Midgard and their channel types) now come from Data/Local/ServerList.json instead of the encrypted ServerList.bmd, and MU Editor gets a Server List window to change them.

ServerList.json

{
  "formatVersion": 1,
  "groups": [
    {
      "id": 0,
      "name": "Valhalla",
      "channels": {"1": "gold", "2": "gold", "3": "gold", "4": "gold", "5": "goldPvp", "6": "nonPvp", "7": "nonPvp", "8": "nonPvp"}
    }
  ]
}

• id is the server ID / 20 (servers 0-19 are group 0), the list order is the button order, channels gives normal (default), nonPvp, goldPvp or gold per channel number. description and testServer are optional; of the test server groups, the login screen shows only the first, at its place in the list.
• A name may have 32 and a description 254 UTF-16 units (an emoji counts as two, as the client's wide strings hold it on Windows); longer text is cut with a warning.
• Errors stop the start with a message, as broken item data does; all problems go to the log.
• The shipped file was converted from ServerList.bmd; a one-time test (added with the conversion, removed with the bmd) compared every group, its order and all 20 channel types with what the old loader read.

Server List window (Windows → Server List)

• Groups in their order on the left (Add, Remove, Move up, Move down); the selected group's ID (with the server IDs it covers), name, description, test server mark and a type for each of the 20 channels on the right, with the name the login screen gives each channel.
• The login screen shows each change right away, and keeps the group that is open there when the list is reordered.
• Groups the connect server sent that the list does not have are listed with an Add button; the channel table shows which channels the connect server sent and their load.
• Save writes the file and refuses errors (no name, an ID twice). After the file was read with errors, Save stays off until it is fixed and Revert reads it again, so it cannot remove the groups the read left out.
• Undo and Redo in the editor bar now act on the tool used last: after a click into the Server List window they undo server list changes, after a click into an item tool item edits (MuEditor::UndoTargets). In the window, Ctrl+Z and Ctrl+Y (or Ctrl+Shift+Z) work while the keyboard is the editor's.
• Strings in English and German.

Fixes on the way

• The old loader read each group's description but never kept it, so the login screen's description panel was always empty. The JSON description is shown. (The shipped groups have none, so nothing changes on screen.)
• Channel types covered only the first 15 of the 20 channels a group can have.
• A server of a group the list does not have leaked the group object it built.

Review fixes

All findings of the review on this PR are fixed, one commit each:

Finding	Commit
Required: undo keys fired while the game had the keyboard	959096b
Required: Save after a read with errors wrote a shorter list	be743f2, c4286d7 (docs)
Hidden server select window kept a pointer to a deleted group	13f2a43
Reordering changed the group open on the login screen	f86a535
Name limit in characters could overflow wide buffers (emoji)	ff0d39b, d60a5cf (format)
"Test server is shown first" in docs, comment and note	0a3a37f
## in a group name read as an ImGui ID	e2be005
Login screen suffixes copied in the editor	7f9d274 (tests first), 5dbfd65
CopyText repeated wcsncpy_s	e8638af
Channel maps built every frame	50d4b48
No test for CServerListManager	7f9d274
Functions over the ~40-line aim	aaaeb87
Two ServerGroup.h files	d8f4da0

The shared Undo/Redo in the editor bar (f8fe9cd) was asked for while testing.

Not in this PR

• No download from OpenMU: OpenMU only knows a game server's ID, description and PvP flag, so the list stays client-side.

Testing

• server_list_data_tests (36 test cases): reader/writer (errors, warnings, cuts in UTF-16 units, round trip, save refusing errors without touching the file), the shipped file reads cleanly and is written exactly as the editor writes it, the editor's edits, undo and the Save guard, and CServerListManager (list order, all 20 channel types, channel names, unknown groups, description, test server, rebuild). The channel name tests went in before the suffix refactor and pass after it.
• The one-time comparison of ServerList.json with ServerList.bmd passed in the conversion commit (removed with the bmd in the next one).
• Windows editor build; test_item_database, test_item_database_files and light_mood_data_tests still pass (the item tools' undo now goes through the shared bar).
• clang-format 21.1.8 and cppcheck with the CI flags on the changed files.
• In game (Windows, editor build, local OpenMU), before the review fixes: the login screen shows the groups from the JSON; renaming a group, changing a channel type, adding a description and reordering in the Server List window showed on the login screen right away. The review fixes are not checked in game yet.
• Not built on Linux or macOS with the editor on: the fork CI skips those jobs.

Commits

1. fix: a server of a group the list does not have no longer leaks the group
2. feat: ServerList.json (+ converted file, one-time comparison test, LF in .gitattributes)
3. chore: remove ServerList.bmd and the comparison
4. feat(editor): the Server List window
5. docs: docs/server-list.md
6. 16 commits for the review findings and the shared Undo/Redo (table above)

[Mosch0512]

Verdict: changes requested for two items; the rest is non-blocking. The login screen gets its groups from ServerList.json as the PR says: the list order is the button order, all 20 channel types reach the channels, the description is kept, and a server of a group the list does not have no longer leaks its group. Both required items are in the Server List window: its undo keys can fire while the game has the keyboard, and Save after a failed read writes a shorter list over the file.

Required

• Ctrl+Z and Ctrl+Y in the Server List window don't check that the keyboard is the editor's, as the item tools and the map editor do (inline, MuServerListUI.cpp:227).
• Save after reading a file with errors writes only the groups that were read, so it deletes the broken groups, or the whole list, from the file (inline, MuServerListUI.cpp:191).

Non-blocking

• After an edit, the hidden server select window keeps a pointer to a deleted group; nothing uses it today (inline, MuServerListUI.cpp:520).
• The name limit counts characters, not bytes, so a name with emoji can overflow a 64-character buffer in the screenshot message on Windows (inline, ServerGroup.h:29).
• The docs, a comment and the editor's note say a test server group is shown first; the login screen shows it at its place in the list (inline, server-list.md:23).
• Reordering groups in the editor changes which group is open on the login screen (inline, MuServerListUI.cpp:519).
• A ## in a group name is read as an ImGui ID marker in the group list (inline, MuServerListUI.cpp:295).
• The login screen suffixes are a copy of the ones in InsertServer (inline, MuServerListUI.cpp:55).
• CopyText repeats wcsncpy_s with _TRUNCATE (inline, ServerListManager.cpp:24).
• The connect server's channels are collected into nested maps on every frame (inline, MuServerListUI.cpp:137).
• No test covers the game side in CServerListManager (inline, ServerListManager.cpp:79).
• RenderChannels, Render and RenderToolbar are over the ~40-line aim (inline, MuServerListUI.cpp:452).
• The new header has the same name as Network/Server/ServerGroup.h (inline, ServerGroup.h:1).

I read every hunk with its enclosing function, the callers of the changed CServerListManager functions, every user of m_pSelectServerGroup and of the buffers the group name is copied into, and how the item tools and the map editor guard their shortcuts. I didn't build or run it. CI: Quality Gates and the macOS build passed; the Linux and Windows builds were still pending when this was posted.

[Mosch0512]

Required. These Ctrl+Z and Ctrl+Y shortcuts don't check g_MuEditorCore.IsKeyboardWithEditor(). HandleUndoKeys in ItemToolWindow.cpp:67 and CMapEditorUI::Shortcut (MapEditorUI.cpp:125) do, because ImGui can keep a window focused after the keyboard has gone back to the game; the comment in ItemToolWindow.cpp names Alt+Tab with the editor inside the game window. The key then reaches the game and ImGui both. The map editor even skips its shortcuts while an RmlUi text field is active, which is the case here.

This window is used while the login screen is open. With the Server List window still focused for ImGui, Ctrl+Z typed in the login screen's ID or password field also undoes the last server list change and builds the login screen's groups again. Returning early while !g_MuEditorCore.IsKeyboardWithEditor(), as HandleUndoKeys does, fixes it. Ctrl+Shift+Z, which the item tools also take as Redo, could come along.

[Mosch0512]

Required. LoadFile keeps whatever the reader returned, and the reader leaves out every group that has an error. A file it rejects as a whole (a JSON syntax error, formatVersion 2) gives no groups at all. Save then writes that list, and since a shorter or empty list reads back without errors, nothing stops it.

For example, while the game runs, a channel type of Helheim is mistyped in the file by hand. Opening the window (or Revert) shows the error, and the window and the login screen have only Valhalla and Midgard. The next Save removes Helheim from ServerList.json. With a syntax error, the next Save writes an empty list. The game can't start with such a file, so this only happens when the file changes while the editor runs, but then the data is lost without a warning. Refusing Save while the last read had errors (until Revert reads a clean file), or a status line saying that Save writes only the groups shown, would prevent it.

[Mosch0512]

Non-blocking. RebuildServerGroups deletes every CServerGroup, but the server select window is refreshed only while it is visible. After a server was picked, ConnectToServer hides the window and keeps m_pSelectServerGroup. An edit made while the login window is up then leaves that pointer on a deleted group, and the new groups keep m_iBtnPos -1 until the next UpdateDisplay.

Nothing uses the pointer today: the window only comes back through ServerListReceived, which calls UpdateDisplay. MsgWin.cpp:279 (RECEIVE_JOIN_SERVER_WAITING) shows the window without UpdateDisplay; nothing raises that code now, but if something did, a click on a channel would read freed memory. Calling g_ServerSelWin.UpdateDisplay() after every rebuild, visible or not, removes the trap; it does no harm while the window is hidden.

[Mosch0512]

Non-blocking. The bmd kept 32 bytes; this limit is 32 characters (CutToCharacters). For most text that is at most 32 wchar_t, as before, but a character outside the Basic Multilingual Plane (most emoji) takes two wchar_t on Windows, so a name can now be 64 wchar_t long.

SceneManager.cpp:249 (outside the diff) formats " [%ls / %ls]" with the group name and the character name into wchar_t lpszTemp[64], and on MSVC mu_swprintf calls the two-argument swprintf, which has no size. A group name of 24 emoji and a 10-letter character name need 65 characters there, so taking a screenshot writes past the buffer. The description has the same gap: 254 characters can be more than the 254 wchar_t that CopyText keeps, and the cut can split a surrogate pair. Counting both limits in UTF-16 units, or mu_swprintf_s at that call site, closes it.

[Mosch0512]

Non-blocking. This line, line 71 ("true puts the group at the top"), the comment at ServerGroup.h:39 ("shown first") and the editor's Test server note ("Shown first") all say a test server group goes to the top. CServerSelWin::UpdateDisplay adds the groups in list order, and server_select.rml lists them in that order (its comment: "one merged, order-preserving list"). testServer gives the group button position 0 and hides later test server groups, but doesn't move it: a test server group at the end of the list is shown last. Either the four texts say that it stays at its place in the list, or UpdateDisplay puts it first.

[Mosch0512]

Non-blocking. CopyText is a wcsncpy_s that cuts to fit: wcsncpy_s(pServerGroup->m_szName, name.c_str(), _TRUNCATE) does the same on every platform (the CRT on Windows, Core/Platform/SecureCrt.h elsewhere, which stdafx.h includes; TextLineWrap.cpp:192 uses it this way).

[Mosch0512]

Non-blocking. FindSentChannels builds a std::map<int, std::map<int, int>> from every received server on each frame the window is drawn, while that list only changes when the connect server sends a new one. Rule 12 of docs/CODING_RULES.md asks for "caches that only rebuild on state change" on per-frame paths. A counter in CServerListManager, raised when the received list changes, would let the window build m_sent only then.

[Mosch0512]

Non-blocking. The PR's three fixes and the new mapping are in this file (SetServerGroups, MakeServerGroup, InsertServerGroup, RebuildServerGroups), but the tests cover only the JSON reader and writer and ServerListEdits. Nothing checks that the list order becomes m_iSequence, that testServer becomes SBP_CENTER, that all 20 channel types reach m_abyNonPvpServer and m_bPvPServer, that a server of a group the list doesn't have is not shown, or that a rebuild keeps the received servers. With the bmd comparison test gone, a regression here would only show in game. The constructor is protected, so a small test subclass (or the shared instance) would do.

[Mosch0512]

Non-blocking. RenderChannels is 57 lines and does three things for each row (the type combo, the login screen name, the connect server's load); Render (lines 116-169) and RenderToolbar (lines 202-253) are over 50. Rule 1 of docs/CODING_RULES.md: "Aim for functions that fit on one screen (~40 lines)." Moving one channel row into its own function would bring RenderChannels under it.

[Mosch0512]

Non-blocking. This file has the same name as Network/Server/ServerGroup.h, which holds CServerGroup, and ServerListManager.h includes both (one as "ServerGroup.h" from its own folder, this one by its full path). Two different ServerGroup types in two ServerGroup.h files are easy to mix up when reading or including. A name like ServerListGroup.h keeps them apart.

[Mosch0512]

Verdict: ready to merge. All 13 findings of the first review are fixed (959096b to d8f4da0), and I checked each fix in the code. The five notes below are about the shared Undo and Redo and the fix commits; all are non-blocking.

Fixed (from the first review)

• Required: the undo keys only act while the keyboard is the editor's, and Ctrl+Shift+Z is Redo too (959096b).
• Required: Save stays off after a read with errors, with a tooltip and a status line, until Revert reads a clean file; there is a test for it (be743f2).
• The server select window is refreshed after every rebuild, also while it is hidden (13f2a43).
• The login screen keeps its open group by its ID when groups move or are removed (f86a535).
• The name and description limits count UTF-16 units; emoji tests added (ff0d39b).
• The test server texts say the group stays at its place in the list (0a3a37f).
• A ## in a group name shows in the list (e2be005).
• CServerListManager has tests for the order, channels, types, names, PvP notice, description, test server and the rebuild (7f9d274).
• One GetChannelSuffix for the login screen and the editor (5dbfd65); wcsncpy_s instead of CopyText (e8638af); the sent channels are mapped only when they change (50d4b48); the long functions are split (aaaeb87); the header is ServerListGroup.h (d8f4da0).

Non-blocking

• After the Server List window is closed, the bar's Undo and Redo still act on it (inline, UndoTarget.cpp:40).
• docs/dev-editor.md:47 still says the bar's Undo and Redo are the item tools' (inline, server-list.md:97).
• The bar's tooltip for the Server List doesn't name the step it would undo (inline, MuServerListUI.cpp:323).
• SyncRmlModel's comment now sits above the new editor functions (inline, ServerSelWin.cpp:340).
• One line of the docs isn't wrapped (inline, server-list.md:108).

I read the 16 new commits with their enclosing functions, the new UndoTargets and every place that activates or renders it, and how UpdateDisplay behaves while the server select window has no document. I didn't build or run it. CI: Quality Gates passed; the Windows, Linux and macOS builds (editor OFF) were still pending when this was posted. The editor-ON jobs are skipped, so CI doesn't compile the Server List window, the undo code or the editor-only tests.

[Mosch0512]

Non-blocking. g_activeTarget only changes when another tool window has the focus, so after the Server List window is closed the bar's Undo and Redo still act on it. For example: edit a group, close the Server List window, then click the bar's Undo to take back the last item edit without clicking into an item tool first. It undoes the server list change instead and builds the login screen's groups again, from a window that is no longer shown. The tooltip also offers Ctrl+Z, which does nothing for a closed window.

A target that is not open could drop out, for example with an UndoTargets::Deactivate(*this) when the window closes, or an IsOpen() on UndoTarget; the bar then falls back to the item tools.

[Mosch0512]

Non-blocking. This describes the shared Undo and Redo, but docs/dev-editor.md:47 (outside the diff) still says the editor window's bar has "Undo and Redo of the item tools", and item-data.md:823 points there for the bar's Undo. Since f8fe9cd they act on the tool used last, the item tools or the Server List window. One sentence there, like the last one of this item, would match. Rule 11 of docs/CODING_RULES.md: "New, changed, or removed systems get a docs update".

[Mosch0512]

Non-blocking. GetUndoLabel and GetRedoLabel return just "Server List", so the bar's tooltip is "Server List (Ctrl+Z)", while for item edits it names the step. After a few changes, the tooltip doesn't tell whether Undo takes back a rename, a removed group or a channel type. ServerListEdits could keep a short label for each step, as ItemEditHistory does ("Remove Helheim", "Midgard: name").

[Mosch0512]

Non-blocking. The editor-only GetOpenGroupId and ShowRebuiltGroups went in between SyncRmlModel's comment and SyncRmlModel, so this comment now sits above GetOpenGroupId. Its claim that "this window's state only ever changes from its own click handlers" is also no longer true, since ShowRebuiltGroups changes it from the editor. Moving the #ifdef _EDITOR block above the comment, and naming the editor in it, keeps both right.

[Mosch0512]

Non-blocking. This line is 142 characters long; the rest of the file wraps at about 78. Wrapping the paragraph again keeps the source and its diffs easy to read.