jamulus icon indicating copy to clipboard operation
jamulus copied to clipboard

Fix connection status issue (#2519)

Open ann0see opened this issue 1 year ago • 12 comments

This is a manual port of https://github.com/jamulussoftware/jamulus/pull/2550 by @pgScorpio

Short description of changes

First step to get to know the codebase of #2550

CHANGELOG: DO NOT MERGE

Context: Fixes an issue?

Fixes: #2519 Supersedes: #2550

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

No

Status of this Pull Request

Ready for first review and decision on how to move forward.

What is missing until this pull request can be merged?

Review. It compiles but not every part of the code is fully understood and commented.

Checklist

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

ann0see avatar Sep 08 '24 09:09 ann0see

Coding style check also fails on main. Seems to be an unrelated issue?

ann0see avatar Sep 08 '24 10:09 ann0see

Pasting from previous PR here:

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)

ann0see avatar Sep 08 '24 10:09 ann0see

Ok.

https://github.com/jamulussoftware/jamulus/pull/3364/commits/a5af8ccd478e405e0ccbc760315c366f12c97367 is for having a smaller diff in this PR. The renaming of methods can happen in another PR.

Client.Start() and Client.Stop() have been re-introduced in https://github.com/jamulussoftware/jamulus/pull/3364/commits/535f781eece9a2a20598dcdf40df6a44d1c1148f, but can be - of course - merged again if wanted.

I think, this PR should now get an in depth review.

ann0see avatar Sep 08 '24 16:09 ann0see

To decide:

  • What to keep
  • What to split into another PR
  • How and what to document.

ann0see avatar Sep 08 '24 16:09 ann0see

I've no idea what this change is trying to do. It appears to be making changes to both client and server. There's no explanation given as to whether one relies on the other or what happens when different versions interact.

pljones avatar Sep 08 '24 16:09 pljones

I think it should only do changes on the client side. The PR is still huge and potentially we need to extract the most important points. See the linked PR for old discussions. My goal is to use this to lay foundations for the JSON-RPC connect/disconnect calls.

ann0see avatar Sep 08 '24 16:09 ann0see

So we need to try understanding what gets changed.

ann0see avatar Sep 08 '24 16:09 ann0see

Maybe we can reconstruct something from https://github.com/pgScorpio/jamulus/commit/648880cbfec48ab10b3eaac06c846c9d6f365c20

ann0see avatar Sep 08 '24 17:09 ann0see

I looked quickly through the changes the other day and didn't notice anything of concern, but I haven't yet had time to study in detail.

softins avatar Sep 13 '24 11:09 softins

I believe there's a need to check if the changed conditions in the if statements are really correct or if the bug lays elsewhere

ann0see avatar Sep 13 '24 12:09 ann0see

Thanks for the review! I'll go through what I can answer later.

ann0see avatar Sep 13 '24 16:09 ann0see

Tagged for Jamulus 4. This PR should NOT be merged. However, the comments are still valuable.

ann0see avatar Dec 23 '24 20:12 ann0see