Refactor UpdateDisplay() to call UpdateUploadRate() #3369
Conversation
| @@ -1063,25 +1063,31 @@ void CClientSettingsDlg::OnSndCrdBufferDelayButtonGroupClicked ( QAbstractButton | |||
| UpdateDisplay(); | |||
| } | |||
|
|
|||
| /// @method | |||
There was a problem hiding this comment.
Probably no need to add @ method here.
Factored out into jamulussoftware#3369
Extracted from: jamulussoftware#2550 Related to: https://github.com/jamulussoftware/jamulus/pull/3364/files/21815c1a0708979fe0414c30e0294feaa7632be3#r1759185427 Co-authored-by: ann0see <[email protected]>
ann0see
force-pushed
the
refactoring/UploadRateSettings
branch
from
c710c60
to
052d875
Compare
| UpdateJitterBufferFrame(); | ||
| UpdateSoundCardFrame(); | ||
|
|
||
| if ( !pClient->IsRunning() ) |
There was a problem hiding this comment.
This is
bool IsRunning() { return Sound.IsRunning(); }
| lblUpstreamValue->setText ( QString().setNum ( pClient->GetUploadRateKbps() ) ); | ||
| lblUpstreamUnit->setText ( "kbps" ); | ||
| // update upstream rate information label if needed | ||
| if ( pClient->IsConnected() ) |
There was a problem hiding this comment.
This is
bool IsConnected() { return Channel.IsConnected(); }
There was a problem hiding this comment.
Yes. That's from https://github.com/jamulussoftware/jamulus/pull/2550/files#diff-d5cde46c72d0c085d1d2c0ca59957407b17931c2f194a33268b3535169006629R1042
A potential semantic change. Probably worth reverting to IsRunning() unless proven to be incorrect.
There was a problem hiding this comment.
I'd like to hear a second OK/not OK from someone nevertheless.
| // update upstream rate information label if needed | ||
| if ( pClient->IsConnected() ) | ||
| { | ||
| lblUpstreamValue->setText ( QString().setNum ( pClient->GetUploadRateKbps() ) ); |
There was a problem hiding this comment.
This is
int GetUploadRateKbps() { return Channel.GetUploadRateKbps(); }
So it makes far more sense to check the Channel.IsConnected().
But really, it's down to the audio settings, nothing to do with whether a sound card is connected or a server is connected:
It can be computed from those values, I think. (But it's probably the Channel that gets passed the values to do the computation - it gets passed uncompressed audio and does the number crunching.)
|
Currently we show the rate only on the settings page. The factor that connecting to a server affects in the calculation is the "Small Buffers" overhead, I think. I can't think of anything else that would need to check for server support before performing the calculation. Originally - before "Small Buffers" - I think it was calculated just from the audio settings, as the network overheads were fixed. So I guess this change makes sense and is, therefore, long overdue. |
|
@softins could you please have a look at this? |
Short description of changes
Refactor UpdateDispay() method. This was extracted from #2550
and is related to: https://github.com/jamulussoftware/jamulus/pull/3364/files/21815c1a0708979fe0414c30e0294feaa7632be3#r1759185427
It makes sense to call UpdateUploadRate() from UpdateDisplay() since it gets same functionality to the same place.
Not sure, if the condition is correct, as we do not check if pClient is running, but rather it is connected now.
CHANGELOG: SKIP
Context: Fixes an issue?
No.
Does this change need documentation? What needs to be documented and how?
No. This even adds documentation
Status of this Pull Request
Ready for review
What is missing until this pull request can be merged?
Review. Tested locally by disconnecting and connecting via GUI and changing Audio quality and enabling/disabling small network buffers. Network rate shows correctly. Also checked by starting Jamulus with
-cflag and seems to do what it should do: Label shows correct data if connected and---if not connected.Checklist