Skip to content
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.

Sign up for GitHub

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

Refactor Connect and Disconnect functionality out to CClient #3372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+134 −115
Open

Refactor Connect and Disconnect functionality out to CClient #3372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 59 additions & 14 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
/* Implementation *************************************************************/
CClient::CClient ( const quint16 iPortNumber,
const quint16 iQosNumber,
const QString& strConnOnStartupAddress,
const QString& strMIDISetup,
const bool bNoAutoJackConnect,
const QString& strNClientName,
Expand Down Expand Up @@ -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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


QObject::connect ( &Channel, &CChannel::NewConnection, this, &CClient::OnNewConnection );

Expand Down Expand Up @@ -184,13 +183,6 @@ CClient::CClient ( const quint16 iPortNumber,
// start the socket (it is important to start the socket after all
// initializations and connections)
Socket.Start();

// do an immediate start if a server address is given
if ( !strConnOnStartupAddress.isEmpty() )
{
SetServerAddr ( strConnOnStartupAddress );
Start();
}
}

CClient::~CClient()
Expand Down Expand Up @@ -486,6 +478,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
Expand Down Expand Up @@ -773,11 +768,8 @@ void CClient::OnHandledSignal ( int sigNum )
{
case SIGINT:
case SIGTERM:
// if connected, terminate connection (needed for headless mode)
if ( IsRunning() )
{
Stop();
}
// if connected, Stop client (needed for headless mode)
Stop();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client::Stop is a slot which looks like it's meant for handling the CChannel::Disconnected signal.

We shouldn't conflate concepts like this. Having the (command line interface) send a "stop" signal isn't the same as the network saying it's lost the connection. QCoreApplication::instance()->exit() should be enough here, with appropriate OnAboutToQuit handlers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So

  • Sound.OnAboutToQuit would call Sound.Stop.
  • Channel.OnAboutToQuit would call Channel.SetEnable ( false )
  • Client.OnAboutToQuit would call Client.Stop -- which wouldn't call Sound.Stop or Channel.SetEnable ( false )

Copy link
Member Author

@ann0see ann0see Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly possible, but I'd like to hear @softins opinion here too.

To me it seems as if this could also complicate things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3402

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly possible, but I'd like to hear @softins opinion here too.

Yes, sorry. This PR is on my list of things to look at again, but just hadn't got to the top of the pile yet! I would certainly like to understand and comment.


// this should trigger OnAboutToQuit
QCoreApplication::instance()->exit();
Expand Down Expand Up @@ -872,8 +864,14 @@ void CClient::Start()

// start audio interface
Sound.Start();

emit Connected ( 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
Expand Down Expand Up @@ -906,6 +904,53 @@ void CClient::Stop()
// reset current signal level and LEDs
bJitterBufferOK = true;
SignalLevelMeter.Reset();

// emit Disconnected() to inform UI of disconnection
emit Disconnected();
}

/// @method
pljones marked this conversation as resolved.
Show resolved Hide resolved
/// @brief Stops the client if the client is running
/// @emit Disconnected
void CClient::Disconnect()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if ( IsRunning() )
{
Stop();
}
}

/// @method
/// @brief Connects to strServerAddress
/// @emit Connected (strServerName) if the client wasn't running and 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 String argument to be passed to Connecting()
void CClient::Connect ( QString strServerAddress, QString strServerName )
{
try
{
if ( !IsRunning() )
{
// 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." ) );
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw some indication it ignored the request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It emits the error message further down

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8e48c03#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R941

}
catch ( const CGenErr& generr )
{
Stop();
emit ConnectingFailed ( generr.GetErrorText() );
}
}

void CClient::Init()
Expand Down
19 changes: 17 additions & 2 deletions src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ class CClient : public QObject
public:
CClient ( const quint16 iPortNumber,
const quint16 iQosNumber,
const QString& strConnOnStartupAddress,
const QString& strMIDISetup,
const bool bNoAutoJackConnect,
const QString& strNClientName,
Expand All @@ -122,6 +121,13 @@ class CClient : public QObject

void Start();
void Stop();
void Disconnect();
void Connect ( QString strServerAddress, QString strServerName );

// The ConnectedServerName is emitted by Connected() to update the UI with a human readable server name
void SetConnectedServerName ( const QString strServerName ) { strConnectedServerName = strServerName; };
QString GetConnectedServerName() const { return strConnectedServerName; };

bool IsRunning() { return Sound.IsRunning(); }
bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); }
ann0see marked this conversation as resolved.
Show resolved Hide resolved
bool SetServerAddr ( QString strNAddr );
Expand Down Expand Up @@ -288,6 +294,10 @@ class CClient : public QObject
int EvaluatePingMessage ( const int iMs );
void CreateServerJitterBufferMessage();

// information for the connected server

QString strConnectedServerName;

// only one channel is needed for client application
CChannel Channel;
CProtocol ConnLessProtocol;
Expand Down Expand Up @@ -388,7 +398,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 );
Expand Down Expand Up @@ -428,7 +439,11 @@ protected slots:

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

void Connected ( QString strServerName );
void ConnectingFailed ( QString errorMessage );
void DisconnectClient();
void Disconnected();

void SoundDeviceChanged ( QString strError );
void ControllerInFaderLevel ( int iChannelIdx, int iValue );
void ControllerInPanValue ( int iChannelIdx, int iValue );
Expand Down
118 changes: 40 additions & 78 deletions src/clientdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
/* Implementation *************************************************************/
CClientDlg::CClientDlg ( CClient* pNCliP,
CClientSettings* pNSetP,
const QString& strConnOnStartupAddress,
const QString& strMIDISetup,
const bool bNewShowComplRegConnList,
const bool bShowAnalyzerConsole,
Expand Down Expand Up @@ -272,14 +271,6 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
TimerCheckAudioDeviceOk.setSingleShot ( true ); // only check once after connection
TimerDetectFeedback.setSingleShot ( true );

// Connect on startup ------------------------------------------------------
if ( !strConnOnStartupAddress.isEmpty() )
{
// initiate connection (always show the address in the mixer board
// (no alias))
Connect ( strConnOnStartupAddress, strConnOnStartupAddress );
}

// File menu --------------------------------------------------------------
QMenu* pFileMenu = new QMenu ( tr ( "&File" ), this );

Expand Down Expand Up @@ -483,7 +474,11 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
// other
QObject::connect ( pClient, &CClient::ConClientListMesReceived, this, &CClientDlg::OnConClientListMesReceived );

QObject::connect ( pClient, &CClient::Disconnected, this, &CClientDlg::OnDisconnected );
QObject::connect ( pClient, &CClient::Connected, this, &CClientDlg::OnConnect );

QObject::connect ( pClient, &CClient::ConnectingFailed, this, &CClientDlg::OnConnectingFailed );

QObject::connect ( pClient, &CClient::Disconnected, this, &CClientDlg::OnDisconnect );

QObject::connect ( pClient, &CClient::ChatTextReceived, this, &CClientDlg::OnChatTextReceived );

Expand Down Expand Up @@ -618,11 +613,8 @@ void CClientDlg::closeEvent ( QCloseEvent* Event )
ConnectDlg.close();
AnalyzerConsole.close();

// if connected, terminate connection
if ( pClient->IsRunning() )
{
pClient->Stop();
}
// Disconnect if needed
pljones marked this conversation as resolved.
Show resolved Hide resolved
pClient->Disconnect();

// make sure all current fader settings are applied to the settings struct
MainMixerBoard->StoreAllFaderSettings();
ann0see marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -740,15 +732,11 @@ void CClientDlg::OnConnectDlgAccepted()
}
}

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

// initiate connection
Connect ( strSelectedAddress, strMixerBoardLabel );
pClient->Connect ( strSelectedAddress, strMixerBoardLabel );

// reset flag
bConnectDlgWasShown = false;
Expand All @@ -760,11 +748,12 @@ void CClientDlg::OnConnectDisconBut()
// the connect/disconnect button implements a toggle functionality
ann0see marked this conversation as resolved.
Show resolved Hide resolved
if ( pClient->IsRunning() )
{
Disconnect();
SetMixerBoardDeco ( RS_UNDEFINED, pClient->GetGUIDesign() );
pClient->Stop();
}
else
{
// If the client isn't running, we assume that we weren't connected. Thus show the connect dialog
// TODO: Refactor to have robust error handling
ShowConnectionSetupDialog();
}
}
Expand Down Expand Up @@ -869,7 +858,7 @@ void CClientDlg::OnLicenceRequired ( ELicenceType eLicenceType )
// disconnect from that server.
if ( !LicenceDlg.exec() )
{
Disconnect();
pClient->Disconnect();
}

// unmute the client output stream if local mute button is not pressed
Expand Down Expand Up @@ -1172,10 +1161,7 @@ void CClientDlg::OnSoundDeviceChanged ( QString strError )
if ( !strError.isEmpty() )
{
// the sound device setup has a problem, disconnect any active connection
if ( pClient->IsRunning() )
{
Disconnect();
}
pClient->Disconnect();

// show the error message of the device setup
QMessageBox::critical ( this, APP_NAME, strError, tr ( "Ok" ), nullptr );
Expand Down Expand Up @@ -1203,65 +1189,38 @@ 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 )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// set address and check if address is valid
if ( pClient->SetServerAddr ( strSelectedAddress ) )
{
// try to start client, if error occurred, do not go in
// running state but show error message
try
ann0see marked this conversation as resolved.
Show resolved Hide resolved
{
if ( !pClient->IsRunning() )
{
pClient->Start();
}
}

catch ( const CGenErr& generr )
{
// show error message and return the function
QMessageBox::critical ( this, APP_NAME, generr.GetErrorText(), "Close", nullptr );
return;
}

// hide label connect to server
lblConnectToServer->hide();
lbrInputLevelL->setEnabled ( true );
lbrInputLevelR->setEnabled ( true );
// hide label connect to server
lblConnectToServer->hide();
lbrInputLevelL->setEnabled ( true );
lbrInputLevelR->setEnabled ( true );

// change connect button text to "disconnect"
butConnect->setText ( tr ( "&Disconnect" ) );
// change connect button text to "disconnect"
butConnect->setText ( tr ( "&Disconnect" ) );

// set server name in audio mixer group box title
MainMixerBoard->SetServerName ( strMixerBoardLabel );
// set server name in audio mixer group box title
MainMixerBoard->SetServerName ( strMixerBoardLabel );

// start timer for level meter bar and ping time measurement
TimerSigMet.start ( LEVELMETER_UPDATE_TIME_MS );
TimerBuffersLED.start ( BUFFER_LED_UPDATE_TIME_MS );
TimerPing.start ( PING_UPDATE_TIME_MS );
TimerCheckAudioDeviceOk.start ( CHECK_AUDIO_DEV_OK_TIME_MS ); // is single shot timer
// start timer for level meter bar and ping time measurement
TimerSigMet.start ( LEVELMETER_UPDATE_TIME_MS );
TimerBuffersLED.start ( BUFFER_LED_UPDATE_TIME_MS );
TimerPing.start ( PING_UPDATE_TIME_MS );
TimerCheckAudioDeviceOk.start ( CHECK_AUDIO_DEV_OK_TIME_MS ); // is single shot timer

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

void CClientDlg::Disconnect()
{
// only stop client if currently running, in case we received
// the stopped message, the client is already stopped but the
// connect/disconnect button and other GUI controls must be
// updated
if ( pClient->IsRunning() )
{
pClient->Stop();
}
void CClientDlg::OnConnectingFailed ( const QString& strError ) { QMessageBox::critical ( this, APP_NAME, strError, tr ( "Close" ), nullptr ); }

ann0see marked this conversation as resolved.
Show resolved Hide resolved
void CClientDlg::OnDisconnect()
{
// change connect button text to "connect"
butConnect->setText ( tr ( "C&onnect" ) );

Expand Down Expand Up @@ -1303,6 +1262,9 @@ void CClientDlg::Disconnect()

// clear mixer board (remove all faders)
MainMixerBoard->HideAll();

// Reset the deco
SetMixerBoardDeco ( RS_UNDEFINED, pClient->GetGUIDesign() );
}

void CClientDlg::UpdateDisplay()
Expand Down
Loading
Loading