Connection status issue #2519

[pgScorpio]

In the first stage I just renamed some functions in CClient to make the problem clear. The second stage should already solve this issue.

Short description of changes

First Stage: Make the cause of the problem clear.

Second Stage: Moved Connect/Disconnect code from CClientdlg to CClient. Now using the proper connected checks in several places. Added bDisconnectAndDisable to CChannel. (For a Client now Channel.Disconnect() will block audio data and auto disable the channel on disconnected)

CHANGELOG: Fix for issue #2519

Context: Fixes an issue?

fixes issue #2519

Does this change need documentation? What needs to be documented and how?

No documentation changes.

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

• [x ] I've verified that this Pull Request follows the general code principles
• [x ] I tested my code and it does what I want
• [x ] My code follows the style guide
• [x ] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• [x ] I've filled all the content above

[pgScorpio]

And auto-build failed on Android, Linux, macOS and iOS ? I should have to look into this, but that's exactly why I want the Sound-redesign ! Nothing changed in any CSound, but still build errors on other OS builds ??? This should never happen...

[pgScorpio]

And again the code style check failed, while I have ran make clang_format and again the result of the auto-build clang-format is NOT according the code style!

I'm really considering stopping my contribution to jamulus at all. Too much frustration!

[ann0see]

Maybe leave the clang format alone until it’s close to finished?

[ann0see]

I'm really considering stopping my contribution to jamulus at all. Too much frustration!

If nobody notices and reports the Problems they can’t be fixed, so yes, it’s painful to start but has a high value.

Did you see the error? Cross platform development is probably really hard.

[pgScorpio]

@ann0see

Did you see the error? Cross platform development is probably really hard.

Now I do see it! Thanks!. Somehow missed that one in the search and replace :=((, I Should have tested it on at least on macOS and Linux (and also headless, which isn't even in the Qt Creator builds, so I had to add it), but it is corrected now.

And Cross platform development Isn't that hard with the propper design and tooling. I've done it for many years, And as a matter of fact it always has been one of my specialties, but unfortunately Qt has little to no support for it and Jamulus is also not really well designed for multi platform either (it clearly has a history), hence my redesign proposal, but It will take a while till we are getting there. As I see it Sound is just the first, most obvious, step....

[hoffie]

• The PR title suggests that this PR is related to a connection status issue. There are logic adjustments in that area (bDisconnectAndDisable?). At the same time, the PR description says "No actual code changes". This doesn't match up for me. I guess that's where the work started, but it doesn't appropriately describe the changes in this PR, IMO. :)
• The PR contains a rework of command line flag processing. I don't think that's related? I think we should get better there, but please keep in mind that @pljones has already been working on some kind of refactoring in that area, IIRC.
• The PR contains dialog box refactoring. I don't see how this is related either. I'm not opposed to it at all (but haven't checked more deeply either). I just believe it doesn't belong here.

For my taste, that should be three (or more?) individual PRs, some with explicit Discussions/Issue ahead of time.

[pgScorpio]

@hoffie

I completely understand your confusion, but that's because I terribly messed up some things by opening a PR from my master (Sorry!!). This causing PR's, my master updates and branch rebasing to mingle up. (I hope we can still correct this)

• The PR title suggests that this PR is related to a connection status issue. There are logic adjustments in that area (bDisconnectAndDisable?). At the same time, the PR description says "No actual code changes". This doesn't match up for me. I guess that's where the work started, but it doesn't appropriately describe the changes in this PR, IMO. :)

"No actual code change" was the " first stage" (Just renaming functions to make the problem clear), in the mean time the second stage is committed too (solving the problem, so actually changing the code). Probably this should have been a new PR? I just don't know...

• The PR contains a rework of command line flag processing. I don't think that's related? I think we should get better there, but please keep in mind that @pljones has already been working on some kind of refactoring in that area, IIRC.
• The PR contains dialog box refactoring. I don't see how this is related either. I'm not opposed to it at all (but haven't checked more deeply either). I just believe it doesn't belong here.

Yes those are from PR #2541 "Adding global messagebox and commandline parameter parsing classes." that was pushed from my master (Sorry again!!)

For my taste, that should be three (or more?) individual PRs, some with explicit Discussions/Issue ahead of time.

So as a matter of fact it are two PR's, both with multiple commits, and updates of my own master also seem to appear here now, so I understand all is getting really unclear (also for me) and I hope we can find a way to solve this problem.

Probably it's better to solve my git problems first and than completely start over again, since I guess I'm already spending more time in solving all kind of problems then I did spent on the coding this PR itself.

[ann0see]

Closing as stale. If there's further development, please re-open