-
Notifications
You must be signed in to change notification settings - Fork 246
Refactor Connect/Disconnect out to CClient with an explicit connection state #3805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -59,6 +59,7 @@ CClient::CClient ( const quint16 iPortNumber, | |||||
| strClientName ( strNClientName ), | ||||||
| pSignalHandler ( CSignalHandler::getSingletonP() ), | ||||||
| pSettings ( nullptr ), | ||||||
| eConnectionState ( CS_DISCONNECTED ), | ||||||
| Channel ( false ), /* we need a client channel -> "false" */ | ||||||
| CurOpusEncoder ( nullptr ), | ||||||
| CurOpusDecoder ( nullptr ), | ||||||
|
|
@@ -145,7 +146,7 @@ CClient::CClient ( const quint16 iPortNumber, | |||||
| // The first ConClientListMesReceived handler performs the necessary cleanup and has to run first: | ||||||
| QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::OnConClientListMesReceived ); | ||||||
|
|
||||||
| QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Disconnected ); | ||||||
| QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Stop ); | ||||||
|
|
||||||
| QObject::connect ( &Channel, &CChannel::NewConnection, this, &CClient::OnNewConnection ); | ||||||
|
|
||||||
|
|
@@ -618,6 +619,9 @@ bool CClient::SetServerAddr ( QString strNAddr ) | |||||
| // apply address to the channel | ||||||
| Channel.SetAddress ( HostAddress ); | ||||||
|
|
||||||
| // By default, set server name to HostAddress. If using the Connect() method, this may be overwritten | ||||||
| SetConnectedServerName ( HostAddress.toString() ); | ||||||
|
|
||||||
| return true; | ||||||
| } | ||||||
| else | ||||||
|
|
@@ -905,11 +909,9 @@ void CClient::OnHandledSignal ( int sigNum ) | |||||
| { | ||||||
| case SIGINT: | ||||||
| case SIGTERM: | ||||||
| // if connected, terminate connection (needed for headless mode) | ||||||
| if ( IsRunning() ) | ||||||
| { | ||||||
| Stop(); | ||||||
| } | ||||||
| // tear down any pending or established connection first, so the server | ||||||
| // is notified we are leaving (Disconnect() is a no-op if not connected) | ||||||
| Disconnect(); | ||||||
|
|
||||||
| // this should trigger OnAboutToQuit | ||||||
| QCoreApplication::instance()->exit(); | ||||||
|
|
@@ -1003,6 +1005,9 @@ void CClient::OnClientIDReceived ( int iServerChanID ) | |||||
| SetRemoteChanGain ( iChanID, 0, false ); | ||||||
| } | ||||||
|
|
||||||
| // the server has assigned us a channel ID, so the connection is established | ||||||
| SetConnectionState ( CS_CONNECTED ); | ||||||
|
|
||||||
| emit ClientIDReceived ( iChanID ); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -1045,8 +1050,19 @@ void CClient::Start() | |||||
| // Disable hibernation or display dimming if the app is running on Windows | ||||||
| SetThreadExecutionState ( ES_CONTINUOUS | ES_SYSTEM_REQUIRED | ES_DISPLAY_REQUIRED ); | ||||||
| #endif | ||||||
|
|
||||||
| // the connection is requested now but not yet established: the transition | ||||||
| // to CS_CONNECTED happens when the server assigns our channel ID | ||||||
| // (see OnClientIDReceived) | ||||||
| SetConnectionState ( CS_CONNECTING ); | ||||||
|
|
||||||
| emit Connecting ( GetConnectedServerName() ); | ||||||
| } | ||||||
|
|
||||||
| /// @method | ||||||
| /// @brief Stops client and disconnects from server | ||||||
| /// @emit Disconnected | ||||||
| /// Use to set CClientDlg to show not being connected | ||||||
| void CClient::Stop() | ||||||
| { | ||||||
| // stop audio interface | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The IsRunning() guard is not needed? (don't remember the initial argument of my PR.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check this - especially for the sigterm case: https://github.com/jamulussoftware/jamulus/pull/3805/changes#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R912-R913
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed together with the guard above in efee722 —
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — a guard is needed, but
Fixed in efee722 by keying the guard off the explicit state this PR introduces, and routing SIGTERM through void CClient::Disconnect()
{
if ( GetConnectionState() != CS_DISCONNECTED ) { Stop(); }
}
// OnHandledSignal now: Disconnect(); (was: raw Stop();)So Verified with a UDP capture: SIGTERM while connected → server receives |
||||||
|
|
@@ -1088,6 +1104,74 @@ void CClient::Stop() | |||||
| // Allow hibernation or display dimming if the app is running again (Windows) | ||||||
| SetThreadExecutionState ( ES_CONTINUOUS ); | ||||||
| #endif | ||||||
|
|
||||||
| SetConnectionState ( CS_DISCONNECTED ); | ||||||
|
|
||||||
| // emit Disconnected() to inform UI of disconnection | ||||||
| emit Disconnected(); | ||||||
| } | ||||||
|
|
||||||
| /// @method | ||||||
| /// @brief Disconnects from the server if a connection is requested or established. | ||||||
| /// Idempotent: a no-op when already disconnected. | ||||||
| /// @emit Disconnected | ||||||
| void CClient::Disconnect() | ||||||
| { | ||||||
| // Key off the connection state, not IsRunning() (which tracks the audio | ||||||
| // device): the two diverge while connecting and in headless mode, and on | ||||||
| // SIGTERM we must still send the disconnect message to the server. | ||||||
| if ( GetConnectionState() != CS_DISCONNECTED ) | ||||||
| { | ||||||
| Stop(); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// @method | ||||||
| /// @brief Connects to strServerAddress. If a connection is currently requested | ||||||
| /// or established, that connection is terminated first. | ||||||
| /// @emit Connecting (strServerName) if SetServerAddr was valid. emit happens through Start(). | ||||||
| /// Use to set CClientDlg to show being connected | ||||||
| /// @emit ConnectingFailed (error) if an error occurred | ||||||
| /// Use to display error message in CClientDlg | ||||||
| /// @param strServerAddress - the server address to connect to | ||||||
| /// @param strServerName - the human readable server name passed to Connecting() | ||||||
| void CClient::Connect ( QString strServerAddress, QString strServerName ) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be const refs
Suggested change
|
||||||
| { | ||||||
| try | ||||||
| { | ||||||
| // disconnect from any current server first so that connecting to a | ||||||
| // different server while connected behaves as a reconnect | ||||||
| Disconnect(); | ||||||
|
|
||||||
| // Set server address and connect if valid address was supplied | ||||||
| if ( SetServerAddr ( strServerAddress ) ) | ||||||
| { | ||||||
| SetConnectedServerName ( strServerName ); | ||||||
| Start(); | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) ); | ||||||
| } | ||||||
| } | ||||||
| catch ( const CGenErr& generr ) | ||||||
| { | ||||||
| Stop(); | ||||||
| emit ConnectingFailed ( generr.GetErrorText() ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// @method | ||||||
| /// @brief Updates the connection state and, if it changed, notifies observers. | ||||||
| /// @emit ConnectionStateChanged (state) when the state actually changes | ||||||
| /// @param eNewConnectionState - the state to transition to | ||||||
| void CClient::SetConnectionState ( const EConnectionState eNewConnectionState ) | ||||||
|
ann0see marked this conversation as resolved.
|
||||||
| { | ||||||
| if ( eConnectionState != eNewConnectionState ) | ||||||
| { | ||||||
| eConnectionState = eNewConnectionState; | ||||||
| emit ConnectionStateChanged ( eConnectionState ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| void CClient::Init() | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,6 +88,17 @@ | |||||||||
| #endif | ||||||||||
|
|
||||||||||
| /* Definitions ****************************************************************/ | ||||||||||
| // Client connection state ----------------------------------------------------- | ||||||||||
| enum EConnectionState | ||||||||||
| { | ||||||||||
| // a connection is "requested" as soon as the client starts the audio | ||||||||||
| // stream towards the configured server and "established" once the server | ||||||||||
| // has assigned a channel ID to the client | ||||||||||
| CS_DISCONNECTED = 0, // no connection requested or established | ||||||||||
| CS_CONNECTING = 1, // connection requested but not yet established | ||||||||||
| CS_CONNECTED = 2 // connection established and current | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| // audio in fader range | ||||||||||
| #define AUD_FADER_IN_MIN 0 | ||||||||||
| #define AUD_FADER_IN_MAX 100 | ||||||||||
|
|
@@ -159,6 +170,15 @@ class CClient : public QObject | |||||||||
|
|
||||||||||
| void Start(); | ||||||||||
| void Stop(); | ||||||||||
| void Disconnect(); | ||||||||||
| void Connect ( QString strServerAddress, QString strServerName ); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
|
||||||||||
| // The ConnectedServerName is emitted by Connecting() to update the UI with a human readable server name | ||||||||||
| void SetConnectedServerName ( const QString strServerName ) { strConnectedServerName = strServerName; }; | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do one copy instead of two:
Suggested change
|
||||||||||
| QString GetConnectedServerName() const { return strConnectedServerName; }; | ||||||||||
|
|
||||||||||
| EConnectionState GetConnectionState() const { return eConnectionState; } | ||||||||||
|
|
||||||||||
| bool IsRunning() { return Sound.IsRunning(); } | ||||||||||
| bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } | ||||||||||
| bool SetServerAddr ( QString strNAddr ); | ||||||||||
|
|
@@ -353,6 +373,13 @@ class CClient : public QObject | |||||||||
| 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 | ||||||||||
| // human-readable name of the current server, shown in the UI | ||||||||||
| QString strConnectedServerName; | ||||||||||
|
|
||||||||||
| // current connection state; set only via SetConnectionState() | ||||||||||
| EConnectionState eConnectionState; | ||||||||||
|
|
||||||||||
| void SetConnectionState ( const EConnectionState eNewConnectionState ); | ||||||||||
|
|
||||||||||
| // only one channel is needed for client application | ||||||||||
| CChannel Channel; | ||||||||||
|
|
@@ -464,7 +491,8 @@ protected slots: | |||||||||
| { | ||||||||||
| if ( InetAddr == Channel.GetAddress() ) | ||||||||||
| { | ||||||||||
| emit Disconnected(); | ||||||||||
| // Stop client in case it received a Disconnection request | ||||||||||
| Stop(); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| void OnCLPingReceived ( CHostAddress InetAddr, int iMs ); | ||||||||||
|
|
@@ -507,7 +535,11 @@ protected slots: | |||||||||
|
|
||||||||||
| void CLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList ); | ||||||||||
|
|
||||||||||
| void ConnectionStateChanged ( EConnectionState eConnectionState ); | ||||||||||
| void Connecting ( QString strServerName ); | ||||||||||
| void ConnectingFailed ( QString errorMessage ); | ||||||||||
|
Comment on lines
+539
to
+540
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| void Disconnected(); | ||||||||||
|
|
||||||||||
| void SoundDeviceChanged ( QString strError ); | ||||||||||
| void ControllerInFaderLevel ( int iChannelIdx, int iValue ); | ||||||||||
| void ControllerInPanValue ( int iChannelIdx, int iValue ); | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.