I have the feeling that we MUST clarify this.
The code in #3364 is different.

I've no idea what this is trying to encapsulate.

I mean we could probably emit in the Stop() method already...

Doesn't seem to call OnConnect() via signal.

I don't like both methods yet.

I don't like this. Connect says "Go off and connect". It should either emit "Connected" or "ConnectionFailed" signals, not return a value. The point is that ClientDlg and Client are meant to run on separate threads and you need to use the slots/signals to communicate.

Agree. Potentially a larger rewrite.

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.

Something called OnAnything should only be used as a connected signal handler, not called directly like this.

See https://github.com/jamulussoftware/jamulus/pull/3372/files/0b6a2bb3c0c70329c85b48f0a6ae52c7e226c2f4#r1759507277 as where I'm stuck at the moment.

Potentially, we could just emit "pClient->Connect(...)" and then really move everything to signals and slots only...

Probably the issue is that the connection happens in the constructor.
It would make sense to remove Connecting from the constructor and do it as in the other PR #3364 (comment)

Ok. See https://forum.qt.io/topic/111772/signals-and-slots-within-same-class/3 maybe that gives some insights

Yup: I said somewhere else, I think -- create the client, clientdlg and client json-rpc, wire them up, then tell the client to start running (passing in the start up details). It would be in the "start running" that the network connection was initiated and signals start getting emitted from the network channel. This is what I mean about this not being a small change. I don't think you get enough benefit from it if you don't do it right.

You rejected connecting in main.cpp here: https://github.com/jamulussoftware/jamulus/pull/3364/files#r1759236825 ?

So how would you do it?

1. Create client
2. Create GUI
3. Create json rpc
4. Connect here by calling pClient->Connect()?

See the changes.

I don't think ClientDlg should be responsible for any of this.

Client should handle it. It should get passed a ClientDlg instance (or null if headless), initialise the ClientDlg (if present), initialise audio and then initiate any "on start up" server connection. That should be enough to get everything to "just work".

Changed my mind a bit about this...

That's the problem with "small changes" that affect the way the application works. They're not really "small".

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.

It's good to see this gone. Direct method calls like this don't look good. (Qt says the pointers are thread-safe, I still don't like it -- I much prefer using only signal/slot calls.)

Maybe OnAboutToQuit should handle all this. It feels cleaner to me to have only the QCoreApplication::instance()->exit() call here and deal with everything else in the handlers.

Again, if different handlers are needed for UI/RPC/headless, they can be wired up appropriately on start-up.

Start up would be something like:

• Create a (default, unconfigured) Client instance
• If requested, create a (default, unconfigured) ClientDlg instance, passing only the pointer to the Client instance
  • Wire up Client public signals to ClientDlg private slots
  • Wire up ClientDlg private signals to Client public slots
• If requested, create a (default, unconfigured) Client JSON-RPC instance, passing only the pointer to the Client instance
  • Wire up Client public signals to Client JSON-RPC private slots
  • Wire up Client JSON-RPC private signals to Client public slots
• Pass Client the start up configuration
  • (emit a new "ConfigurationChanged(newConfiguration)" signal to let everything know something changed?)
  • effect Client start up, emitting any such signals as are needed when state changes

It wouldn't be until the Client said it was Running that the ClientDlg displayed itself, I think.

Thus, when exiting, it's the Client instance that's in control. It says "OK, time to quit". It should have itself wired up any internal Audio and (network) Channel slots to clean up the connection. All the ClientDlg and Client JSON-RPC need to know is that exit is happening and to shut themselves down cleanly.

that exit is happening and to shut themselves down cleanly.

Which should then in turn also fix the connection status issue if implemented correctly.

I'll have a thought about this.

I think it's not part of this PR though. Could you please open an issue?

IMO this is not nice too. Sound should emit a signal if it started correctly, else emit an error signal. However, implementing this opens another pandora box.

This should be implemented by switching the connect setting for the signal handler for the button, rather than having one signal handler with an if that consults the state of the client.

Button life-cycle:

• (public) ConnectDlg onClickConnect is connected to (private) ClientDlg onConnectDlgConnect
• onClickConnect launches ConnectDlg if it's not already open (and nothing else)
• onConnectDlgConnect disables the button and asks the client to connect to the provided address
• onConnect switches the onClick handler to onClickDisconnect and enables the button
• onConnectFailed just enables the button (keeping the onClickConnect handler) -- although, if Client is passing an error, here's where it would get displayed to the user
• onClickDisconnect disables the button and asks the client to disconnect
• onDisconnect (which should be handled) switches the onClick handler to onClickDisconnect and enables the button

For JSON-RPC, the equivalent is

• (get a list of servers from somewhere)
• Send a Client JSON-RPC request to connect to a server - get an "ACK" back to say message received
• At some asynchronous time in the future, receive a Client JSON-RPC push of either a successful connect or failed connect
• (assuming connected) Send a Client JSON-RPC request to disconnect the current connection - get an "ACK" back to say message received
• At some asynchronous time in the future, receive a Client JSON-RPC push when the client decides it has disconnected

Note that in both the UI and JSON-RPC cases, the actual change in client state is asynchronous. For disconnect, it's possible (given that the client might not actually be connected), that nothing will happen.

Now, for that "nothing happened" case, we've a potential problem: the UI/JSON-RPC "thought" we were connected and asked to disconnect. Is asking the client to disconnect all that's needed or should the UI/JSON-RPC wait for confirmation of the disconnect? If it should wait, we then MUST send that "disconnected" message back, whether or not we were connected.

Yes, this should be changed. But in a follow up PR

What does Sound.Stop do? Why can't Sound.Stop get called anyway and just do nothing if it's stopped?

What, indeed, does Client.Stop do and can't it just get called anyway?

(The method feels clunky -- too small to be useful.)

Sound.Stop() is driver specific, I think.
For ASIO, for example it calls ASIOStop(), iOS calls AudioOutputUnitStop(), JACK nothing, Android closeStreams(). But the important part is that all call CSoundBase::Stop().

In any way, CClient::Stop adds the 100ms timer additionally and sends a disconnect message to the server. Not sure if that's a problem (I'd rather think it isn't)

I've for now still included a small method.

... And removed it...

This is not nice, but it seems to crash the app if I move the check and pApp->exec() after the else

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.

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 )

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

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

Maybe closeEvent should just call QCoreApplication::instance()->exit(); and then do the work in OnAboutToQuit handlers? That should stop needing to call XYZ.Stop().

Of course, if that's what the "default implementation of this event handler routine" does, it would move all the code here into ClientDlg.OnAboutToQuit (plus handlers in each called class).