-
Notifications
You must be signed in to change notification settings - Fork 247
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
43b5eb5
1b49a78
efee722
a7c5c7b
5964120
d952fbb
c47352e
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 | ||
|
|
@@ -921,11 +925,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(); | ||
|
|
@@ -1019,6 +1021,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 ); | ||
| } | ||
|
|
||
|
|
@@ -1061,8 +1066,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() | ||
|
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. We might even be able to make Stop() private so that everything is forced to go through Disconnect() |
||
| { | ||
| // stop audio interface | ||
|
ann0see marked this conversation as resolved.
|
||
|
|
@@ -1073,7 +1089,15 @@ void CClient::Stop() | |
|
|
||
| // Fall back to opus in case raw was used | ||
| bRawAudioIsSupported = false; | ||
| Init(); | ||
| try | ||
| { | ||
| Init(); | ||
| } | ||
| catch ( const CGenErr& ) | ||
| { | ||
| // a dead audio backend (e.g. JACK was shut down) must not prevent the | ||
|
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. Hmm. Maybe the warning should be logged. |
||
| // disconnect message below from reaching the server | ||
| } | ||
|
|
||
| // wait for approx. 100 ms to make sure no audio packet is still in the | ||
| // network queue causing the channel to be reconnected right after having | ||
|
|
@@ -1104,6 +1128,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 ( const QString& strServerAddress, const QString& strServerName ) | ||
| { | ||
| 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.
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. Probably also worth to make this private |
||
| { | ||
| if ( eConnectionState != eNewConnectionState ) | ||
| { | ||
| eConnectionState = eNewConnectionState; | ||
| emit ConnectionStateChanged ( eConnectionState ); | ||
| } | ||
| } | ||
|
|
||
| void CClient::Init() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.