Refactor Connect/Disconnect out to CClient with an explicit connection state by mcfnord · Pull Request #3805 · jamulussoftware/jamulus

mcfnord · 2026-07-19T01:37:09Z

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:

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.
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

This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

Introduce EConnectionState (disconnected / connecting / connected) owned by CClient as the single source of truth. A connection is 'requested' when the audio stream starts (CS_CONNECTING) and 'established' once the server assigns our channel ID (CS_CONNECTED). Every transition emits ConnectionStateChanged. Rename the Connected(name) signal emitted from Start() to Connecting(name), since at that point the connection is only requested, not established. CClient::Connect() now terminates any current connection first, so connecting while connected behaves as a reconnect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ann0see · 2026-07-19T05:55:41Z

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

pljones · 2026-07-19T08:10:53Z

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 · 2026-07-19T13:54:51Z

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 · 2026-07-19T18:28:25Z

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 · 2026-07-19T18:32:09Z

+///       Use to set CClientDlg to show not being connected
 void CClient::Stop()
 {
    // stop audio interface


The IsRunning() guard is not needed? (don't remember the initial argument of my PR.)

Please check this - especially for the sigterm case: https://github.com/jamulussoftware/jamulus/pull/3805/changes#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R912-R913

Addressed together with the guard above in efee722 — OnHandledSignal now calls Disconnect(), and the state-based guard means SIGTERM while CS_CONNECTING still sends CLM_DISCONNECTION (verified by capture).

Good catch — a guard is needed, but IsRunning() was the wrong predicate.

IsRunning() is Sound.IsRunning() (the audio device). Start() sets CS_CONNECTING and calls Sound.Start() as separate steps, so the two diverge: during CS_CONNECTING, and in headless mode where audio may lag or be unavailable, the state is connecting/connected while IsRunning() is still false. Stop() is the only thing that sends CLDisconnection, and on the client there is no aboutToQuit handler and ~CClient() only does Sound.Stop() — so OnHandledSignal was calling raw Stop() precisely to dodge the guard and still notify the server on shutdown.

Fixed in efee722 by keying the guard off the explicit state this PR introduces, and routing SIGTERM through Disconnect():

void CClient::Disconnect() { if ( GetConnectionState() != CS_DISCONNECTED ) { Stop(); } } // OnHandledSignal now: Disconnect(); (was: raw Stop();)

So Disconnect() stays idempotent (the repeated UI calls remain no-ops) and SIGTERM notifies the server whenever a connection is pending or established — including mid-connect and in headless mode. Dropped the stale // needed for headless mode / // this should trigger OnAboutToQuit comments (there is no client OnAboutToQuit).

Verified with a UDP capture: SIGTERM while connected → server receives CLM_DISCONNECTION; SIGTERM while idle → no spurious disconnect (the old raw Stop() sent one to a stale address); clean exit 0 in both cases.

ann0see · 2026-07-19T18:34:15Z

+    }
+}
+
+void CClient::SetConnectionState ( const EConnectionState eNewConnectionState )


Add documentation like for connect/disconnect.

Done in efee722 — added a /// @method doc-comment.

ann0see · 2026-07-19T18:35:29Z

    void FreeClientChannel ( const int iServerChannelID );
    int  FindClientChannel ( const int iServerChannelID, const bool bCreateIfNew ); // returns a client channel ID or INVALID_INDEX
    bool ReorderLevelList ( CVector<uint16_t>& vecLevelList );                      // modifies vecLevelList, passed by reference
+    // information for the connected server


Which kind of information? Comment is unclear.

Done in efee722 — reworded to "human-readable name of the current server, shown in the UI".

ann0see · 2026-07-19T18:35:57Z

+    // single source of truth for the connection state, only to be modified
+    // via SetConnectionState() so that every change emits ConnectionStateChanged()


Does not really add much and smells to much AI ish.

Done in efee722 — trimmed to "current connection state; set only via SetConnectionState()".

ann0see · 2026-07-19T18:37:37Z

+    if ( pSettings->bEnableFeedbackDetection )
+    {
+        TimerDetectFeedback.start ( DETECT_FEEDBACK_TIME_MS ); // single shot timer
+        bDetectFeedback = true;


Not part of this PR, but yes, this should also not be in the UI.

Agreed, out of scope for this PR — left as-is for a separate change.

ann0see · 2026-07-19T18:38:38Z

    RS_RECORDING       = 3
 };

+// Client connection state -----------------------------------------------------


Do not put this into util.h I think it's a client specific thing -> client.h

Done in efee722 — moved EConnectionState to client.h.

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.

- SIGTERM/SIGINT: route through Disconnect() instead of a raw Stop(), and guard Disconnect() on the connection state rather than IsRunning() (which tracks the audio device). IsRunning() is false while connecting and in headless mode, so the old guard could skip notifying the server on shutdown; the raw Stop() worked around that but also fired a spurious disconnect when idle. Now the server is notified iff a connection is pending or established, via the single Disconnect() path. - Move EConnectionState from util.h to client.h (it is client specific). - Add a doc-comment to SetConnectionState; reword two member comments. Addresses review feedback on jamulussoftware#3805. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

pgScorpio and others added 2 commits July 19, 2026 01:01

Refactor Connect out to CClient

43b5eb5

This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

mcfnord mentioned this pull request Jul 19, 2026

Add JSON-RPC methods to connect, disconnect and query connection state #3806

Draft

5 tasks

mcfnord marked this pull request as ready for review July 19, 2026 02:25

ann0see requested review from pljones and softins July 19, 2026 05:56

ann0see mentioned this pull request Jul 19, 2026

Refactor Connect and Disconnect functionality out to CClient #3372

Closed

5 tasks

ann0see reviewed Jul 19, 2026

View reviewed changes

ann0see requested changes Jul 19, 2026

View reviewed changes

ann0see reviewed Jul 19, 2026

View reviewed changes

pljones added this to Tracking Jul 20, 2026

pljones added this to the Release 4.1.0 milestone Jul 20, 2026

github-project-automation Bot moved this to Triage in Tracking Jul 20, 2026

github-project-automation Bot moved this from Triage to Waiting externally in Tracking Jul 20, 2026

pljones moved this from Waiting externally to Backlog in Tracking Jul 20, 2026

mcfnord mentioned this pull request Jul 21, 2026

Client: add --connectdirectory for directory-assisted hole punch on --connect #3815

Draft

5 tasks

		// single source of truth for the connection state, only to be modified
		// via SetConnectionState() so that every change emits ConnectionStateChanged()

Uh oh!

Conversation

mcfnord commented Jul 19, 2026

Checklist

Uh oh!

ann0see commented Jul 19, 2026

Uh oh!

pljones commented Jul 19, 2026

Uh oh!

ann0see commented Jul 19, 2026

Uh oh!

ann0see commented Jul 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ann0see Jul 19, 2026 •

edited

Loading

ann0see left a comment •

edited

Loading