search

jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
997 stars 222 forks source link

UI: "Set up your audio..." messages sometimes remains #1076

Closed pljones closed 3 years ago

pljones commented 3 years ago

Describe the bug The "Set up your audio..." message sometimes remains when connected to a server.

To Reproduce Couldn't pin it down. It could have been going straight from one server to another... not sure. I'll try again tomorrow.

Expected behavior The message should only be displayed when not connected.

Screenshots Unfortunately, wasn't able to save them whilst jamming.

Operating system image Version of Jamulus image

hoffie commented 3 years ago

I'd argue that this should block the 3.7.0 release as this would mean we accept new bugs, which would be non-optimal. I'll therefore add this to the milestone.

@pljones would be ok for you if I split this into two issues?

pljones commented 3 years ago

Yep, no problem.

I still failed to reproduce the first one. It might be visible in the code. I'll try to get some time to take a look - but don't let that stop others looking, too.

hoffie commented 3 years ago

@ann0see You worked on that feature in #983, maybe you've got an idea? (I suggest if any of you actively works on it that you'd assign the issue to yourself?)

ann0see commented 3 years ago

I can have a look at it, sure. Unfortunately the next few days are a bit full for me.

Concerning the bug: I think the question is when we say the client is connected and when not. I initially looked for the change of the Connect to Disconnect button to hide the label there. But as far as I remember, I changed that.

Note to self: Why didn't I hide the label here https://github.com/ann0see/jamulus/blob/a7ecdf4b26f5f774a5f2e32585949b644c64bf3b/src/clientdlg.cpp#L1213

pljones commented 3 years ago

the change of the Connect to Disconnect button

That should be the right place - there should be a method that's called from anywhere that needs to disconnect that goes through there.

pljones commented 3 years ago

So I can sleep...

This needs to be in CAudioMixerBoard:UpdateTitle(), like @dcorson-ticino-com's change, for the same reason. That method handles the connectionless, asynchronous nature of the protocol. Tge "trying to connect" title is key- you normally never notice it...

ann0see commented 3 years ago

Since I don't have much C++ experience, this might be a simple answer for many of you:

This needs to be in CAudioMixerBoard:UpdateTitle()

That method handles the connectionless, asynchronous nature of the protocol. Tge "trying to connect" title is key- you normally never notice it...

I don't really understand why.

In DonC's case I can understand that the server text might change if recording is enabled during runtime. But in my case, I don't really get the theory behind this bug if this is the only way to solve it.

Is it possible to jump to a new server without clicking on the disconnect button? Even if that's the case, the disconnect button should also show "disconnect"?

Why is it not possible to moved my code to show the message right before changing the text of disconnect to connect and vise versa?

Ok. Another thought:

If you're semi-connected to a server (blocked via the license message), I would hide the "Set up your audio message" which can be done by moving the hide() method of the label above the change of the disconnect button?

pljones commented 3 years ago

First off, the UpdateTitle() method should be responsible for this because that's its job.

That you've chosen to add another component above the group box just means you have to replicate all the calls to UpdateTitle() to the code that manages your new component.

That, or you put that management code in one place, UpdateTitle(), that was working.

ann0see commented 3 years ago

As far as I see, the only place where UpdateTitle() is called is in CAudioMixerBoard::SetRecorderState --> We'd need to move the lblConnectToServer->hide() etc. call to the mixer board which doesn't feel right.

For me, it's nothing about the mixer board? It's more something about the client UI and shouldn't be updated if we receive a new message that recording is now enabled?

Please also see the edit of my other comment. @pljones

ann0see commented 3 years ago

Ah, wait. Does this change prevent the trying to connect message? No. Just checked the r3_6_1 and I couldn't see any different behaviour (tested by disabling the network).

pljones commented 3 years ago

We'd need to move the lblConnectToServer->hide() etc. call to the mixer board which doesn't feel right.

I said it should be in the title of the mixer group box. It feels right to me.

ann0see commented 3 years ago

Ok. I assume for the next release (since I think a lot of UI related things will be changed) we can discuss the future of this message.