Load client settings before --connect startup

[seanogdelaney]

Summary

• prevent --connect from starting the client from the CClient constructor before INI-backed settings are loaded
• remove the startup-address parameter from CClient so startup connection handling is owned by the post-settings startup paths
• keep GUI startup connection handling in CClientDlg, where the dialog is created after Settings.Load() and Client.SetSettings()
• start the client explicitly in the no-GUI path after settings have been loaded and attached to the client

This ensures startup connection negotiation uses the loaded client settings, including audio channel mode, so stereo-dependent UI/state such as pan controls is not initialized from constructor defaults.

CHANGELOG: Client: Ensure INI file settings are loaded before doing -c/--connect startup.

Testing

• git diff --check
• make -j2

Fixes #3732

[softins]

To do this properly, if the CClient constructor is indeed the wrong place to start the connection, strConnOnStartupAddress should be removed from the signature and invocation of CClient::CClient(), rather than just declaring it as unused.

[seanogdelaney]

Done in 8d62998: removed strConnOnStartupAddress from the CClient constructor signature and the main.cpp invocation, and dropped the temporary Q_UNUSED. Also verified with git diff --check and make -j2.

[softins]

This looks like the wrong place to insert this code. Originally, the -c option worked in both GUI and headless modes. Putting it at this location makes it only work in headless mode.

I think the best place to move it to would be up a few lines to just before the #ifndef HEADLESS around line 968. That is after the settings have been loaded, but before the GUI is loaded (if any).

Otherwise if that doesn't fix your issue, move it down a few lines to around line 1001, just before the end of the if ( bIsClient ) block.

[softins]

Ah, looks like you're correct here. I've only just noticed that CClientDlg handles the connect-at-startup when in GUI mode.

So disregard the above comment. But The CClient constructor still needs the other change.

[seanogdelaney]

Thanks - cleaned up that code.
Also clarified the PR summary.

[softins]

@seanogdelaney thank you! I've built it and it tests out fine.

One remaining minor niggle is that the format check in the CI doesn't like the remaining blank line after Socket.Start() in client.cpp, as it leaves a blank line before the closing brace of a block.

If you have clang-format version 14 on your dev system, you can do make clang_format before committing, and it will fix any formatting anomalies to keep the CI happy.

[softins]

Ah, you got there first!

[softins]

Many thanks, looks good to me now.

[seanogdelaney]

Many thanks, looks good to me now.

Thanks Tony for the help. Let me know what to do next, if anything.

[softins]

Thanks Tony for the help. Let me know what to do next, if anything.

No problem. Nothing to do now, just wait for another team member to approve and merge. After that you can delete your branch.

[pljones]

Has this been tested without the GUI as well, just to make sure nothing unexpected happens? Is JSONRPC control of the connected client still possible, etc? Looks like it should just work, though.

[softins]

Has this been tested without the GUI as well, just to make sure nothing unexpected happens?

Yes, I built it, and tested both with GUI and headless. It behaved as expected.

Is JSONRPC control of the connected client still possible, etc?

I didn't try the JSONRPC, which I always find rather fiddly. We could do with a JSONRPC controller application to make testing easier. Is there one? But I can't see anything in this change that would affect JSONRPC either way.

Looks like it should just work, though.

Agreed!