Refactor Connect and Disconnect functionality out to CClient #3372

ann0see · 2024-09-13T20:53:05Z

This is an extract from #2550

Short description of changes

Refactor Connect() and disconnect functionality.
Introduces a new Connect() method for quick connection and adds signals to Start() and Stop() of client.

CHANGELOG: Refactoring: Move Connect() functionality to CClient.

Context: Fixes an issue?

Fixes: #3367

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

No

Status of this Pull Request

Ready for review. Start for refactoring. Most likely overlaps and to a large extent relates somehow to #3364

What is missing until this pull request can be merged?

Review, clarification, probably documentation and refactoring.

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

src/client.cpp

src/clientdlg.cpp

src/client.cpp

pljones · 2024-09-14T08:53:17Z

src/client.cpp

+    if ( !IsRunning() )
+    {
+        // Set server address and connect if valid address was supplied
+        if ( SetServerAddr ( strServerAddress ) )


I don't like this style at all. If there's some validation to happen, it should have happened before we get this far. We shouldn't try to connect to an address that's not invalid.

Perhaps this whole routine is redundant and SetServerAddr should be called Connect and do the emits?

SetServerAddr

Should just do exactly what the name implies IMO - Otherwise we could introduce mixing of functionality.
Connect() would then emit the signals. That's cleaner and closer to what we do now.

I think that nevertheless Connect(serverAddress) should exist. It simplifies needing to set and then start the client.

src/client.cpp

src/clientdlg.cpp

pljones · 2024-09-14T09:29:04Z

src/clientdlg.cpp

@@ -1193,65 +1193,36 @@ void CClientDlg::OnCLPingTimeWithNumClientsReceived ( CHostAddress InetAddr, int
    ConnectDlg.SetPingTimeAndNumClientsResult ( InetAddr, iPingTime, iNumClients );
 }

-void CClientDlg::Connect ( const QString& strSelectedAddress, const QString& strMixerBoardLabel )
+void CClientDlg::OnConnect ( const QString& strMixerBoardLabel )


Ideally, this signal would get issued only on successful connection.

As we're going cross-thread here, we need Client to supply any listener -- ClientDlg or the Client JSON-RPC -- with full details of the newly established connection.

We should follow this rule for all state changes in the Client -- that should be the only way for ClientDlg and the Client JSON-RPC to acquire knowledge of Client state, no calls to functions across threads.

(The same is true on the server side, too.)

Similarly, to change the state of the Client, the Client should expose slots and ClientDlg or Client JSON-RPC should signal those slots to effect the change in state. Not call methods across threads.

This does demand a major restructure but I think this could be a good place to start. Set out what state changes when the Client successfully establishes a connection to a server, create an object instance containing that information, and issue a signal passing that object. This gives a clearly defined API to that change in state.

Either the message on the signal could be minimal, covering only the state that changes on any connect, or extensive, supplying the whole Client state. In the latter case, every signal from the Client would pass a new state instance with the full current Client state, making the listener's job much easier. (I don't like it as much as the minimal approach but it's likely much easier to explain and maintain. It smacks of "global variables" a bit too much, though...)

The global approach could also be wasteful while the small approach could be more difficult. If we restructure correctly, we should aim at the small approach.

src/clientdlg.cpp

src/client.cpp

src/clientdlg.cpp

src/client.h

ann0see · 2024-09-15T10:20:43Z

channel.cpp emits Disconnected() in GetData --> may need some refactoring

src/clientdlg.h

src/clientdlg.cpp

src/channel.cpp

src/client.cpp

src/clientdlg.cpp

src/client.cpp

ann0see · 2024-10-08T19:12:35Z

Please recheck if the important parts are in the two issues I opened and comment there.

pljones · 2024-10-08T19:21:44Z

src/client.cpp

+            {
+                throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) );
+            }
+        }


Should this throw some indication it ignored the request?

It emits the error message further down

8e48c03#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R941

ann0see · 2024-10-08T19:25:04Z

I do not understand why clang format complains about the list style here.

https://github.com/jamulussoftware/jamulus/actions/runs/11242233032/job/31255455777?pr=3372#step:4:18

src/clientdlg.cpp

softins · 2024-10-29T18:08:03Z

src/clientdlg.cpp

-        // first check if we are already connected, if this is the case we have to
-        // disconnect the old server first
-        if ( pClient->IsRunning() )
-        {
-            Disconnect();
-        }
-


I don't think this code should be removed. Or else equivalent code should be added somewhere according to the new structure (which I don't yet fully understand).

It is possible, while connected to a server, to go File->Connection Setup..., or Ctrl-C, to bring up the connect dialog, and then select a new server to connect to. In existing versions of Jamulus, this disconnects from the connected server, and then immediately connects to the new server. This was used to great effect during WorldJam to quickly move between rooms.

In the version built from this PR, this doesn't work. The existing connection continues instead of connecting to the new server.

We could probably call something like pClient->Disconnect()/Stop() and then pClient->Connect().
The main idea of this PR is to:

Emit signals if the client disconnects or connects

Introduce convenient methods for programmatic (JSON-RPC) disconnection and connection.

Ok. Fixed. Not sure if we need the if is running then stop.
I'd like to have this if in the client then.

src/clientdlg.cpp

This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>

ann0see · 2024-11-04T20:32:39Z

src/client.cpp

+/// @method
+/// @brief Stops the client if the client is running
+/// @emit Disconnected
+void CClient::Disconnect()


Introduced again. Mainly an alias. But now also with the check

pljones · 2024-11-05T18:48:52Z

src/client.cpp

@@ -122,7 +121,7 @@ CClient::CClient ( const quint16  iPortNumber,
    QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::OnConClientListMesReceived );
    QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::ConClientListMesReceived );

-    QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Disconnected );
+    QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Stop );


I'm not happy with Stop as a name. What does it do? It doesn't exit the application...

It's used (at least) here and in the SIGINT/SIGTERM handler. The latter makes me wonder if it's also used in the OnAboutToQuit handling (which should be invoked by the SIGINT/SIGTERM handler...).

Stop() means it stops the client.
But this concept is not well defined IMO.

I guess I want to know what "Stop" stops the client doing. About the only thing I can think is it should trigger emitting a signal that stops audio processing. If that is all it's doing, this would be better as a lambda.

I think I'd like SIGINT/SIGTERM to simply exit the application and have the AboutToQuit handling clean up, if that's a sane approach. Currently it feels like there's random paths through the code for no real reason.

pljones · 2024-11-12T18:32:38Z

OK... so what I'm expecting to see...

sort out "exit client" so that there's only "one way out"
- there should be one CClient method that tells Qt the client is exiting and does nothing else.
  - this would be called from GUI (File->Exit) and SIGINT/SIGTERM handlers to make the client close and those would do nothing else.
- any and all other cleaning up should happen when Qt sends the "OnAboutToQuit" signal.
work out what "disconnecting from a server" means
- have a CClient method that performs disconnection
  - does nothing if not connected
  - tells the server the client is disconnecting (?? do we do this ??)
  - calls another CClient method to handle disconnecting
- have CClient respond to the server disconnecting by calling another CClient method to handle disconnecting (lambda)
- have a CClient method that handles disconnecting by
  - cleaning up any CClient structures
  - emitting a (blocking) Disconnected signal
- amend other components to listen for Disconnected and take appropriate clean up actions
work out what "connecting to a server" means
- have a CClient method that
  - calls the CClient method to perform disconnection
  - initiates the connection (which could fail)
  - emits a signal indicating a connection has been requested but not yet granted
- have a CClient method that handles a successful connection
- have a CClient method that handles an unsuccessful connection

I think most of the Connect stuff is there. I'm less clear what's happening around Disconnect. And the exiting the client currently appears very messy - at least, it's confusing me...

ann0see · 2024-11-12T22:20:36Z

Yes, I agree.
This may imply that we may need to restructure the UI functionality a bit.
I'll have a look in a few weeks. Currently I have a higher load with another development project.

ann0see · 2024-11-16T19:44:11Z

I would like to define the methods as follows (not sure if this is everything)

CClient is connected iff all of those hold

CClient.Init() was called
CClient.Connect() was called
Channel.IsEnabled() is true
Sound.IsRunning() is true
CClient.IsRunning() is true (which is to be redefined)
There is no soundcard error (to be defined)

CClient will disconnect if at least one of these conditions holds

A soundcard error occurs
The server disconnects
Disconnect() was called

CClient is running iff

CClient.Init() was called
CClient.Start() was called
CClient.IsRunning() returns true
Potentially: SetServerAddress() was called and succeeded

This means, there may be a sound card error while CClient is running and the client may not be connected to a server

To transition from CClient being running to CClient being connected, we need to call Connect() and the connection to the server must stand (within the timeout).
To transition from CClient being connected to just being running, we have to:

Either call Disconnect()
Have any kind of network or audio issue
The Server requests a disconnect

A SIGINT would

Stop the client
Which will force the client to disconnect.

To transition from CClient being connected to cclient being disconnected, We'd just call disconnect. I do not think there's a way to have a failed disconnection:

This means:

Sound.Stop() gets called and Channel.SetEnable(false) gets called, ConnLessProtocol.CreateCLDisconnection() gets called. Only after ConnLessProtocol.CreateCLDisconnection() the client is disconnected (and should not accept any packets from the server anymore). It is only in stopped state after the GUI is finished (SignalLevelMeter.Reset() was called - I don't like to have this in CCLient though.)

Does this make sense?

pljones · 2024-11-17T11:19:06Z

Background

(Apologies for writing this out in a PR where you know it as well as I do but I'm trying to make sure my thoughts are in order...)

We need states:

disconnected
connection requested but not connected
connection established and current

"Disconnected" means no connection has been requested or established
"Connection requested but not connected" means a connection has been requested but audio data is not being exchanged yet
"Connection established and current" means a server connection is in progress

Now, these states should apply equally to the Client's view of it's single connection to a Server and Server's view of each of its connections to a Client. This is important (and a core concept to Jamulus).

The Client Channel owns that state. The Client has one Channel but the Server has many Channels. So any changes here must not break the Server.

A Channel is considered disconnected when no more audio data has been exchanged for a certain period (or something, I don't remember this bit) or an explicit disconnect is received (maybe, I'm not sure on that, either). Again, that goes equally for both ends of the Channel.

(And finally... The Channel carries Protocol messages that can either be audio or other messages. The audio messages go into the jitter buffer, triggering audio processing. The other protocol messages get processed, quite often by the Channel itself but sometimes needing either Client or Server (as they know more about the world) to get involved. The main difference between Client and Server, of course, is audio processing -- the Client exchanges CSound audio data with the jitter buffer but the Server pushes the audio through the mixer.)

CClient is connected iff all of those hold

OK, let's start with "CClient has a CChannel" and it's "CChannel" that holds state regarding whether an active connection has been established. Given this is the case (and it has to be for the Client and Server to work using the same code):

CClient is "connected" or "running" or "started" or whatever, if and only if
- its CChannel.ConnectionState() is Connected (or "CChannel.IsConnected()")
- its CSound.IsRunning() is true

Neither of those alone is useful for components outside CClient (other than CChannel and CSound maintaining their own state). Nothing else needs worrying about.

CClient.Connect() was called

Maybe we rename CClient.Connect(...) to CClient.RequestConnection(QString server) because that's where all the Client-specific logic lives and it doesn't end up by connecting. It would:

always call a CClient.EndConnection() method that
- calls CChannel.Disconnect()
  - (somewhere drains the out-going and in-coming audio packets in the jitter buffer - I guess it's here)
- calls CSound.Stop()
  - (by this point it should have had one packet of silence to output)
- does any other Client-specific clean up (e.g. clear connected server name)
- emit a ConnectionEnded signal
calls CSound.Start()
calls CChannel.RequestConnection(QString server)
emits a ConnectionRequested(QString server) signal

CChannel.Connect() is Client-only. It would

set connection state of ConnectionRequested (so CChannel.ConnectionState() returns that -- IsConnected() would be false)
set up the connection to the server
- fills the jitter buffers with silence and keeps them that way until the connection is established
- open the socket and set up the signal handlers
- kill the connection if a timer expires before the timer is stopped

(The Client is responsible for putting audio from CSound into the channel jitter buffer, which should trigger CChannel to take audio from the jitter buffer and send it to the Server, and for reading the jitter buffer and passing the audio to CSound. I don't clearly remember the structure here. There's a timer that expires if processing doesn't start. There's a timer that expires for each audio cycle, too. I think both are in CChannel as it's very similar in Client and Server.)

CChannel will handle audio being received by marking the CChannel connection state as ConnectionEstablished (IsConnected() is true). Again, same at Client and Server for this. It also emits a ConnectionEstablished signal.

CClient will disconnect if at least one of these conditions holds

The first two should cause CClient.Disconnect() to be called and nothing else - lambda slot handlers in their respective classes calling the client method for signals. It'll get called during CClient.OnAboutToQuit, I expect.

Actually, I'm proposing CClient.EndConnection() in place of CClient.Disconnect() to make it clear it's a non-trivial operation. It should also always, without condition:

call CChannel.Disconnect() (which may itself check whether it's in a ConnectionEstablished state)
- (somewhere drains the out-going and in-coming audio packets in the jitter buffer - I guess it's here)
call CSound.Stop() (which may itself check whether it's running)
- (by this point it should have had one packet of silence to output)
do any other Client-specific clean up (e.g. clear connected server name)
emit a ConnectionEnded signal

CClient is running iff

We need two things:

CChannel.ConnectionState() should be ConnectionEstablished
CSound.IsRunning() should be true

We could also check the wiring up of the signals from those two classes to the slots in the Client are in place so the sound goes to/from the channel... because CChannel and CSound do not know CClient is handling their signals. However, that would be wired up once during creation of CClient, so can be taken as a given.

Nothing else matters because, so long as we have sound going in and out and packets going in and out - that means we set up okay and we're running fine.

This means, there may be a sound card error while CClient is running and the client may not be connected to a server

Yes - but by CChannel changing state from ConnectionEstablished to Disconnected and emitting a ConnectionEnded signal or by CSound changing state from Running to Stopped and emitting an SoundStopped signal, where both are handled by CClient. In either case, asking CClient whether it's "connected" or "running" or "started" or whatever, it'll say false.

In fact, CChannel::ConnectionEnded and CSound::SoundStopped would get handled the same way -- calling CClient::EndConnection. (Or StopConnection or something... it needs a better name...)

To transition from CClient being connected to just being running, we have to:

OK, I don't understand your concept of "just being running". The Client just being there isn't the Client being in any particular state in and of itself. It's just there, handling signals. That doesn't need a state in and of itself.

If CChannel.ConnectionState() should - unexpectedly - transition from ConnectionEstablished to anything else, we terminate the connection and stop sound -- because it means we lost the connection (timer expired).
If CSound.IsRunning() should - unexpectedly - transition from true to anything else, we terminate the connection and stop sound -- because we got an audio error.

Neither of the above changes the state of the Client itself -- it would change the result of the method that combines the result of CChannel.ConnectionState() and CSound.IsRunning(), though. But that's not a state, it's a computation.

If CChannel gets a server disconnect protocol message, it'll need to pass that to CClient to deal with - not simply disconnect, as that would be seen as an error happening.

Any request to Disconnect would be handled the same way -- call CClient.EndConnection().

Exiting the client can happen multiple ways -- CLI through SIGINT/SIGTERM, GUI through menu emitting a signal, or a JSON-RPC command. These should all trigger the Qt OnAboutToQuit by asking Qt to exit the application. The Window Manager can also trigger Qt OnAboutToQuit by asking Qt to exit the application. In all cases, when the client terminates, part of handling OnAboutToQuit in CClient would be to call CClient.EndConnection().

Or maybe I'm missing something?

ann0see commented Sep 13, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

ann0see commented Sep 13, 2024

View reviewed changes

src/clientdlg.cpp Outdated Show resolved Hide resolved

ann0see commented Sep 13, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

ann0see mentioned this pull request Sep 13, 2024

Fix connection status issue (#2519) #3364

Draft

5 tasks

ann0see requested review from softins and pljones September 13, 2024 21:44

pljones reviewed Sep 14, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/client.cpp Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/clientdlg.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/clientdlg.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

pljones reviewed Sep 14, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

ann0see commented Sep 14, 2024

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

ann0see commented Sep 15, 2024

View reviewed changes

src/client.h Show resolved Hide resolved

ann0see commented Sep 15, 2024

View reviewed changes

src/clientdlg.h Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 8bacce4 to 4ec9c68 Compare September 15, 2024 10:56

ann0see commented Sep 15, 2024

View reviewed changes

src/clientdlg.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 4ec9c68 to a1d98aa Compare September 15, 2024 10:58

ann0see commented Sep 15, 2024

View reviewed changes

src/channel.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch 2 times, most recently from 69246f5 to 57825bf Compare September 15, 2024 20:44

ann0see commented Sep 15, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

pljones reviewed Sep 16, 2024

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

pljones reviewed Sep 16, 2024

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 57825bf to 8bc81d7 Compare September 16, 2024 19:30

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 65622ee to 1957532 Compare September 22, 2024 16:08

ann0see force-pushed the refactoring/moveConnectDisconnect branch 2 times, most recently from e27569f to ae31a65 Compare October 8, 2024 18:43

This was referenced Oct 8, 2024

Refactor CClientDlgOnConnectDisconnectBut() #3401

Open

Rewrite cclientdlg closeEvent, onAboutToQuit, ... #3402

Open

ann0see added this to the Release 3.12.0 milestone Oct 8, 2024

ann0see requested a review from pljones October 8, 2024 19:09

ann0see force-pushed the refactoring/moveConnectDisconnect branch 2 times, most recently from 8b0b44d to 8e48c03 Compare October 8, 2024 19:17

pljones reviewed Oct 8, 2024

View reviewed changes

ann0see marked this pull request as ready for review October 8, 2024 19:27

pljones reviewed Oct 8, 2024

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 8e48c03 to 9f93619 Compare October 8, 2024 19:30

softins requested changes Oct 29, 2024

View reviewed changes

ann0see force-pushed the refactoring/moveConnectDisconnect branch 2 times, most recently from 4642e40 to b996df2 Compare November 3, 2024 21:30

pljones requested changes Nov 4, 2024

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

src/clientdlg.cpp Outdated Show resolved Hide resolved

src/clientdlg.cpp Outdated Show resolved Hide resolved

ann0see force-pushed the refactoring/moveConnectDisconnect branch from b996df2 to 7b420ff Compare November 4, 2024 20:27

Refactor Connect out to CClient

a77dcfe

This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>

ann0see force-pushed the refactoring/moveConnectDisconnect branch from 7b420ff to a77dcfe Compare November 4, 2024 20:30

ann0see requested review from pljones and softins November 4, 2024 20:31

ann0see commented Nov 4, 2024

View reviewed changes

pljones reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Connect and Disconnect functionality out to CClient #3372

Refactor Connect and Disconnect functionality out to CClient #3372

ann0see commented Sep 13, 2024 •

edited

Loading

pljones Sep 14, 2024

ann0see Sep 14, 2024

ann0see Sep 16, 2024

pljones Sep 14, 2024

ann0see Sep 14, 2024

ann0see commented Sep 15, 2024

ann0see commented Oct 8, 2024 •

edited

Loading

pljones Oct 8, 2024

ann0see Oct 8, 2024

ann0see Oct 8, 2024

ann0see commented Oct 8, 2024 •

edited

Loading

softins Oct 29, 2024

ann0see Nov 3, 2024

ann0see Nov 3, 2024 •

edited

Loading

ann0see Nov 4, 2024

pljones Nov 5, 2024

ann0see Nov 7, 2024

pljones Nov 7, 2024

pljones commented Nov 12, 2024

ann0see commented Nov 12, 2024

ann0see commented Nov 16, 2024 •

edited

Loading

pljones commented Nov 17, 2024 •

edited

Loading

Refactor Connect and Disconnect functionality out to CClient #3372

Are you sure you want to change the base?

Refactor Connect and Disconnect functionality out to CClient #3372

Conversation

ann0see commented Sep 13, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Sep 15, 2024

ann0see commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones commented Nov 12, 2024

ann0see commented Nov 12, 2024

ann0see commented Nov 16, 2024 • edited Loading

pljones commented Nov 17, 2024 • edited Loading

Background

ann0see commented Sep 13, 2024 •

edited

Loading

ann0see commented Oct 8, 2024 •

edited

Loading

ann0see commented Oct 8, 2024 •

edited

Loading

ann0see Nov 3, 2024 •

edited

Loading

ann0see commented Nov 16, 2024 •

edited

Loading

pljones commented Nov 17, 2024 •

edited

Loading