Refactor Connect/Disconnect out to CClient with an explicit connection state

[mcfnord]

Short description of changes

This picks up and finishes #3372 (@ann0see's extract of @pgScorpio's work in #2550), which had stalled on review discussion. It contains two commits:

1. The original Refactor Connect and Disconnect functionality out to CClient #3372 refactor, ported onto current main (authorship preserved): Connect() / Disconnect() move out of CClientDlg into CClient, which emits signals the dialog consumes. CClientDlg no longer holds connection logic — it only reacts (OnConnecting, OnConnectingFailed, OnDisconnect). Connect-on-startup moves out of the dialog constructor into main.cpp via Client.Connect(...), so it now also works identically in --nogui mode.

2. An explicit connection state machine, addressing the state-model review comments in Refactor Connect and Disconnect functionality out to CClient #3372: a new EConnectionState (CS_DISCONNECTED / CS_CONNECTING / CS_CONNECTED) owned by CClient as the single source of truth, with every transition emitting ConnectionStateChanged. Following that discussion's definitions: a connection is requested (CS_CONNECTING) when the audio stream starts toward the configured server, and established (CS_CONNECTED) once the server assigns our channel ID. Accordingly, the signal formerly emitted from Start() as Connected(name) is renamed Connecting(name), since at that point the connection is only requested. CClient::Connect() now terminates any current connection first, so connecting while connected behaves as a clean reconnect.

This is a step toward #3801 (CClient as the orchestration layer behind UI and JSON-RPC): a follow-up PR stacked on this one exposes connect/disconnect/state over JSON-RPC as symmetric consumers of the same signals.

Context: Fixes an issue?

Fixes #3367. Supersedes #3372 (both commits build on it; the first preserves its authorship).

Does this change need documentation? What needs to be documented and how?

No user-facing behavior change. The follow-up JSON-RPC PR updates docs/JSON-RPC.md.

Status of this Pull Request

Ready for review.

What is missing until this pull request can be merged?

Review.

Tested: full GUI build and CONFIG+=headless build on Linux/Qt 5.15; a --nogui client was driven through connect → reconnect-while-connected → disconnect cycles against a local server, observing the Connecting/ConnectionStateChanged/Disconnected signal sequence via the follow-up PR's JSON-RPC notifications (16/16 scripted checks pass); clean SIGTERM shutdown verified.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

AUTOBUILD: Please build all targets

🤖 Generated with Claude Code

[ann0see]

Thanks. This needs thorough testing as this refactor is definitely non trivial.

[pljones]

I'm just wondering if we should cut a 4.0.0 off where we are on main plus the TCP changes when they land and have these refactorings land onto 4.1.0 (4.0.0dev)?

We're currently holding up a feature (raw audio) that benefits all users for work that has infrastructure value.

It's definitely good to have them, though.

[ann0see]

We're currently holding up a feature (raw audio) that benefits all users for work that has infrastructure value.

It's stalled for a good reason though. Now we know it works (but probably would benefit from PLC) but at the start we didn't.

[ann0see]

GUI tested and I can confirm it works as expected. From the code review it also looks fine. I'll still need to do an in depth analysis though.

[ann0see]

Check if this breaks feedback detection somehow. (Manually).

Free model from open code (Big Pickle) had the following comments (also seem my comments on this)

This one I was already suspicious about:

Bug Assessment
Likely Bug: Stop() emits Disconnected unconditionally
CClient::Stop() now always emits Disconnected() even if the client wasn't connected:
void CClient::Stop()
{
    // ... cleanup ...
    SetConnectionState ( CS_DISCONNECTED ); // no-op if already disconnected
    emit Disconnected();                    // <-- ALWAYS emitted
}
This means every Stop() call triggers the full UI teardown chain in OnDisconnect() (reset button text, hide mixer board, clear faders, reset deco). If Stop() is ever called when the client wasn't running, the UI will react to a spurious disconnect. The old code guarded this with if (IsRunning()).

Potential Issue: bDetectFeedback not reset on disconnect
When OnConnecting() starts the feedback detection timer and sets bDetectFeedback = true, neither OnDisconnect() nor Stop() resets it. The single-shot timer will expire, but the flag remains stale. Not a crash bug, but could cause unexpected behavior if bDetectFeedback is checked elsewhere before the next connection.

Fair point. Maybe be problematic - but not sure...

Minor: CS_CONNECTING state during Connect() failure path
In Connect(), the catch block calls Stop(), which transitions CS_CONNECTING → CS_DISCONNECTED and emits Disconnected. Then the catch block also emits ConnectingFailed. So the UI gets both signals in sequence — this works, but ConnectingFailed arrives after the UI has already been torn down by Disconnected, which could be confusing for error display ordering.

Probably really not too bad.

[ann0see]

I'll look at this later. I'd like to have this in 4.0 not 4.1 as it's crucial for clean connect/disconnect RPC functionality.

[softins]

It looks good. There are several places where a QString value parameter could better be a const ref const QString&.

I wondered for a moment whether the signal/slot boundary needed object parameters such as QString to be passed by value. But in fact Qt can handle const refs just fine, copying only when necessary to cross a thread boundary, and it reduces the number of copies, as described here

[softins]

BTW, I've tested my suggested changes.

[ann0see]

@mcfnord Please have a look at this Pr again.

[mcfnord]

🤖 AI

All four suggestions are in, as one commit. The receiving slots already declared const QString&, so signal and slot signatures now match exactly instead of relying on conversion.

Checked while making the change: Connect() calls Disconnect() before it reads strServerName, and nothing on that path writes strConnectedServerName, so the reference stays valid. All three call sites pass function-local strings.

GUI and headless both build clean. A headless client still connects to a local server end to end: jamulusclient/getClientInfo returns {"connected": true}.

---

🤖 This message was written by AI and reviewed by @mcfnord.

[softins]

Looks good to me now. Thanks!

[ann0see]

Please don't merge. Updating the branch = rebase.

[ann0see]

Hmm. Maybe the warning should be logged.

[ann0see]

See above comment.

[ann0see]

Otherwise it seems good.

[ann0see]

We might even be able to make Stop() private so that everything is forced to go through Disconnect()

[ann0see]

Probably also worth to make this private