Fix connection status issue (#2519) by ann0see · Pull Request #3364 · jamulussoftware/jamulus

ann0see · 2024-09-08T09:51:25Z

This is a manual port of #2550 by @pgScorpio

Short description of changes

First step to get to know the codebase of #2550

CHANGELOG: DO NOT MERGE

Context: Fixes an issue?

Fixes: #2519
Supersedes: #2550

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

No

Status of this Pull Request

Ready for first review and decision on how to move forward.

What is missing until this pull request can be merged?

Review.
It compiles but not every part of the code is fully understood and commented.

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

@pgScorpio

This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

ann0see · 2024-09-08T10:29:29Z

Coding style check also fails on main. Seems to be an unrelated issue?

ann0see · 2024-09-08T10:36:01Z

Pasting from previous PR here:

Moved Connect/Disconnect code from CClientdlg to CClient.
Now using the proper connected checks in several places.
Added bDisconnectAndDisable to CChannel. (For a Client now
Channel.Disconnect() will block audio data and auto disable the channel on disconnected)

ann0see · 2024-09-08T16:18:44Z

Ok.

a5af8cc is for having a smaller diff in this PR. The renaming of methods can happen in another PR.

Client.Start() and Client.Stop() have been re-introduced in 535f781, but can be - of course - merged again if wanted.

I think, this PR should now get an in depth review.

ann0see · 2024-09-08T16:20:17Z


    void Start();
    void Stop();
+    bool Connect ( QString strServerAddress, QString strServerName );


It does make sense to introduce an easy to call Connect() and Disconnect() method IMO, so this is good.

Having these in client.cpp rather than clientdlg.cpp is important. It's important enough to be done in a single PR on its own. I think we should start with that.

Agree. That's issue #3367

Factored out into #3372

ann0see · 2024-09-08T16:20:56Z

+
    bool IsRunning() { return Sound.IsRunning(); }
-    bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); }
+    bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } // For OnTimerCheckAudioDeviceOk only


Reverted naming. May need some in depth review and discussion.

ann0see · 2024-09-08T16:21:50Z


    void CLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList );

+    void Connecting ( QString strServerName );


Probably a missing slot.

ann0see · 2024-09-08T16:22:26Z

+    void Connecting ( QString strServerName );
    void Disconnected();
-    void SoundDeviceChanged ( QString strError );
+    void SoundDeviceChanged();


I don't feel strongly about removing the error message handling. Could be reverted.

As above:

split this out to a separate PR

have two slots, one for GUI, one for headless, and pass the error to each

ann0see · 2024-09-08T16:23:18Z

-
-        // initiate connection
-        Connect ( strSelectedAddress, strMixerBoardLabel );
+        // TODO: investigate and add error handling on failed Connect() call.


TODO as commented in code.

pljones · 2024-09-13T16:27:01Z

 {
    // make sure we are running and the server address is correct
-    if ( IsRunning() && ( InetAddr == Channel.GetAddress() ) )
+    if ( Channel.IsEnabled() && ( InetAddr == Channel.GetAddress() ) )


This isn't about the channel being enabled. It's about whether the client is running. It's a different intent.

I think the initial PR confused active channel and active client: #2550 (comment)

This difference needs documentation. IMO: Add (like for JSON-RPC) respective technical documentation before the respective methods. As end goal: Write them for every function, for now: Write docs while changing.

pljones · 2024-09-13T16:37:07Z

-    Sound.Stop();
+    // start disconnection
+    // Channel.Disconnect() should automatically disable Channel as soon as disconnected.
+    // Note that this only works if sound is active!


So why make the changes here?

I believe it's mainly moving around the Channel.Disconnect() and Sound.Stop() code around.

This may be the actual bug fix?

pljones · 2024-09-13T16:43:34Z

 }

-void CClientDlg::Disconnect()
+void CClientDlg::OnDisconnect()


Tidying up the mixer board should go in here, I guess.

ann0see · 2024-09-13T16:44:53Z

Thanks for the review! I'll go through what I can answer later.

pljones · 2024-09-13T17:28:53Z

                // show dialog
                ClientDlg.show();

+                // Connect on startup


These shouldn't be in main.cpp.

I think that's up to discussion. I do understand that one could put this functionality here. How it's handled currently also seems somewhat messy.

Yes, it is messy. But that's a bigger issue with how start up works.

Factored out into jamulussoftware#3369

ann0see · 2024-09-13T20:07:21Z

-        // disconnect the old server first
-        if ( pClient->IsRunning() )
+        // initiate connection
+        if ( pClient->Connect ( strSelectedAddress, strMixerBoardLabel ) )


This should be refactored imo. Feels very clumsy.

UI should simply pass any captured details to Client and await feedback. There shouldn't be any testing of the response. (Any input validation should be factored out of Client if it needs sharing between Client and ClientDlg.)

ann0see · 2024-12-23T20:18:59Z

Tagged for Jamulus 4. This PR should NOT be merged. However, the comments are still valuable.

pljones · 2026-07-19T12:37:16Z

@mcfnord did you have a reworking of this? (I've lost a few PRs somewhere.)

mcfnord · 2026-07-22T06:21:17Z

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

@pljones — yes. The reworking is #3805 ("Refactor Connect/Disconnect out to CClient with an explicit connection state"), open and awaiting review. It's the same line of work as this PR: both descend from @pgScorpio's #2550. This PR ports #2550 directly onto main; #3805 goes through @ann0see's #3372 extract, finishes it, and adds a state machine on top. It's the one to carry forward.

Structurally #3805 does what #3367 asks for: Connect() / Disconnect() move out of CClientDlg into CClient, and the dialog only reacts to signals (OnConnecting / OnConnectingFailed / OnDisconnect). It adds an explicit EConnectionState (CS_DISCONNECTED / CS_CONNECTING / CS_CONNECTED) owned by CClient as the single source of truth, with every transition emitting ConnectionStateChanged. Connect-on-startup also moves into main.cpp, so it behaves the same under --nogui.

That structural change is what actually resolves #2519, and it answers the doubt @ann0see raised in the review here — "check if the changed conditions in the if statements are really correct or if the bug lays elsewhere." It does lie elsewhere. The GUI's "connected" is CClient::IsRunning(), which is Sound.IsRunning() — audio-device state, not network state (src/client.h:162). The real network predicate IsConnected() = Channel.IsConnected() exists (src/client.h:174) but the connect/disconnect UI never reads it: OnConnectDisconBut() (src/clientdlg.cpp:786), OnConnectDlgAccepted(), and closeEvent() all key off IsRunning(). So a device error while connected flips the button to "Connect" over a live connection, and audio starting before the server answers shows "Disconnect" while nothing is connected — both reported directions. (Full trace in #2519.)

This PR refines the conditions inside that dialog-centric model; #3805 replaces the model with the single EConnectionState source of truth, which is the durable version. Once #3805 lands, I'd suggest this one can be closed in its favor — your call.

pljones · 2026-07-22T16:43:44Z

@ann0see are you okay we can close this in favour of #3372 and #3805 ? I'd like to pick both up together in a single release (i.e. 4.1.0 now).

ann0see · 2026-07-22T17:19:33Z

Yes.

ann0see mentioned this pull request Sep 8, 2024

Connection status issue #2519 #2550

Closed

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 2 times, most recently from b12ce7e to 2b9d985 Compare September 8, 2024 09:58

Fix connection status issue (jamulussoftware#2519)

38fb414

This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 2 times, most recently from d6e5a6c to 38fb414 Compare September 8, 2024 10:28

ann0see marked this pull request as draft September 8, 2024 10:34

ann0see commented Sep 8, 2024

View reviewed changes

Comment thread src/client.cpp Outdated

ann0see commented Sep 8, 2024

View reviewed changes

Comment thread src/client.cpp Outdated

ann0see mentioned this pull request Sep 8, 2024

Add JSON-RPC access to directories and server lists. #3249

Closed

5 tasks

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 3 times, most recently from db402b9 to 10a4d61 Compare September 8, 2024 15:23

Revert some changes for now

a5af8cc

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch from 10a4d61 to a5af8cc Compare September 8, 2024 15:25

ann0see commented Sep 8, 2024

View reviewed changes

Comment thread src/main.cpp

ann0see commented Sep 8, 2024

View reviewed changes

Comment thread src/client.cpp

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch 3 times, most recently from f7f0515 to 1614b81 Compare September 8, 2024 16:08

Refactor Connect() and Disconnect() into two methods

535f781

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch from 1614b81 to 535f781 Compare September 8, 2024 16:13

ann0see commented Sep 8, 2024

View reviewed changes

pljones reviewed Sep 13, 2024

View reviewed changes

Comment thread src/client.cpp Outdated

pljones reviewed Sep 13, 2024

View reviewed changes

Comment thread src/client.cpp Outdated

pljones reviewed Sep 13, 2024

View reviewed changes

Comment thread src/client.cpp Outdated

pljones reviewed Sep 13, 2024

View reviewed changes

Comment thread src/clientsettingsdlg.cpp Outdated

pljones reviewed Sep 13, 2024

View reviewed changes

ann0see added 2 commits September 13, 2024 21:06

Revert renaming of strMixerBoardLabel

d7b4b8b

Revert strError handling

66edab5

ann0see force-pushed the refactoring/RefactorClientConnectionUi branch from 2633d3c to 66edab5 Compare September 13, 2024 19:33

Revert refactoring of UpdateDisplay()

49402c2

Factored out into jamulussoftware#3369

ann0see commented Sep 13, 2024

View reviewed changes

ann0see mentioned this pull request Sep 13, 2024

Refactor Connect and Disconnect functionality out to CClient #3372

Closed

5 tasks

ann0see added this to the Release 4.0.0 milestone Dec 23, 2024

ann0see added the refactoring Non-behavioural changes, Code cleanup label Dec 23, 2024

pljones assigned ann0see Sep 3, 2025

pljones moved this from Waiting on Team to Triage in Tracking Apr 26, 2026

pljones modified the milestones: Release 4.0.0, Release 4.1.0 Jul 20, 2026

ann0see closed this Jul 22, 2026

github-project-automation Bot moved this from Triage to Done in Tracking Jul 22, 2026

pljones removed this from the Release 4.1.0 milestone Jul 23, 2026

pljones removed this from Tracking Jul 23, 2026


		void CLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList );

		void Connecting ( QString strServerName );

Uh oh!

Conversation

ann0see commented Sep 8, 2024 • edited by softins Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ann0see commented Sep 8, 2024

Uh oh!

ann0see commented Sep 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ann0see commented Sep 8, 2024

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see Sep 13, 2024 • 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!

ann0see commented Sep 13, 2024

Uh oh!

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 commented Dec 23, 2024

Uh oh!

pljones commented Jul 19, 2026

Uh oh!

mcfnord commented Jul 22, 2026

Uh oh!

pljones commented Jul 22, 2026

Uh oh!

ann0see commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ann0see commented Sep 8, 2024 •

edited by softins

Loading

ann0see Sep 13, 2024 •

edited

Loading