-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection status issue #2519 #2550
Conversation
src/client.h
Outdated
void Stop(); | ||
bool IsRunning() { return Sound.IsRunning(); } | ||
bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } | ||
void StartConnection(); // ---> pgScorpio: Was Start(), but Start what ? ( Should be Connect() ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Staart the client. It does more than just connection.
src/client.h
Outdated
bool IsRunning() { return Sound.IsRunning(); } | ||
bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } | ||
void StartConnection(); // ---> pgScorpio: Was Start(), but Start what ? ( Should be Connect() ?) | ||
void StopConnection(); // ---> pgScorpio: Was Stop(), but Stop what ? ( Should be Disconnect() ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stop the client. It does more than just disconnection.
src/clientdlg.cpp
Outdated
// set address and check if address is valid | ||
if ( pClient->SetServerAddr ( strSelectedAddress ) ) | ||
{ | ||
// try to start client, if error occurred, do not go in | ||
// running state but show error message | ||
try | ||
{ | ||
if ( !pClient->IsRunning() ) | ||
if ( !pClient->SoundIsStarted() ) // ---> pgScorpio: Again this is NOT connection started !() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's whether the client is running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly what I mean to say! This function does NOT indicate if we are connected to a server, but on;ly if Sound is Started.
And Sound started does say nothing about an connection to the server, since Sound.Start() can have failed (No check on that) but the connection still can be started with Channel.Enable(true) !
This is exactly where it goes wrong with an invalid device selected...
Sound is NOT started, but the Channel is enabled !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to indicate whether the client has started. Not the state of the sound system. Not the state of the network connection. But whether the client has started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to indicate whether the client has started. Not the state of the sound system. Not the state of the network connection. But whether the client has started.
I Think we mean the same, but use different vocabulary ;=))
And yes, I am very persistent in using variable- and function names that clearly state what they do (And unfortunately I encounter a lot of them in jamulus which absolutely don't, some of them even very misleading.)
So I think the Client is already started when you run jamulus (without a -s ;=))
It's about if the connection to the server is started, and the original IsRunning() (Used at many places) had nothing to do with the connection to the server, but with Sound. So if Sound.Start() fails (and there is no check on that!) they all think there is no connection to the server, but in reality we actually are connected/connecting to the server, so UI and reality don't match and you can't disconnect, since hitting the connect button will try to start a connection again while we are already connecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Client is definitely not started when you run Jamulus. It's not even created then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Client is definitely not started when you run Jamulus. It's not even created then.
I meant It is started automatically when you run jamulus (without a -s ;=)). (Created in main and started with pApp->exec();
)
As said we are apparently using different vocabulary...
Maybe you call " Started" what I call "Connected" (or "connecting" ?), since I think the client is already started if you see it's user interface and can interact with it. Though it's just not yet connected to a server, it's still "Running" in some way.
Simple verbs like " Start" and " Stop" are often ambiguous, and adverbs like " Running" and " Connected" are often even more ambiguous and often interpreted differently depending on the context.
i.e. When are you "Driving" a car ?, As soon as you take a seat behind the wheel? (Legally seen yes), When you "Started" the engine? (You can drive downhill without even starting the engine), or when your car is really moving ? (If so, also when it's on the back of a truck?, or being towed?)
So to come back on topic: " Started" does not mean "Running", nor "Connected". And not "Connected" is not the same as not "Running" (might be "Connecting", so " Started" but not yet " Connected" ). An easy confusion that can cause a lot of, often hard to find, bugs, like in this case.
But no offence, I'm the last one to say that writing clear code is easy. Even after so many years I'm still struggling almost daily with good, clear, naming's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t quote me on that – since my knowledge of the code is basic – but I think "the client" is not the UI but the CClient class. Maybe talking about the actual class names makes it easier to avoid confusion.
Also: I wouldn’t write your name next to comments since they might be updated/… in future by someone else who isn’t you. Having you in the git history should clearly show who wrote what if this is actually needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t quote me on that – since my knowledge of the code is basic – but I think "the client" is not the UI but the CClient class. Maybe talking about the actual class names makes it easier to avoid confusion.
I agree, so to be clearer then: I said "I think the client is already started if you see it's user interface and can interact with it..." where "the client" is CClient and "it's user interface" is CCLientDlg. And in my opinion CClient must be running when you can interact with it via CClientDlg, since it's CClient which actually changes the settings and controls the connection to the server.
Also: I wouldn’t write your name next to comments since they might be updated/… in future by someone else who isn’t you. Having you in the git history should clearly show who wrote what if this is actually needed.
Good to know... Thanks for the advise.
And auto-build failed on Android, Linux, macOS and iOS ? |
And again the code style check failed, while I have ran I'm really considering stopping my contribution to jamulus at all. Too much frustration! |
Maybe leave the clang format alone until it’s close to finished? |
Now I do see it! Thanks!. Somehow missed that one in the search and replace :=((, 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.... |
src/global.h
Outdated
#define CMDLN_CTRLMIDICH "--ctrlmidich", "--ctrlmidich" | ||
// Backwards compatibilyty: | ||
#define CMDLN_CENTRALSERVER "--centralserver", "--centralserver" | ||
// pgScorpio: TODO These are NOT in help !: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that they are mostly used for debugging/advanced use. Hence it was agreed not to document them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these options are documented on the wiki and the man page
So should I change this comment to "Debug options: (not in help)" ?
Also I think I'm messing things up, since this is a change from issue #2538.
And I only just now realize what you ment with "although merging from your master branch to our master branch is discouraged". I didn't branch this one ! So now, after my rebasing these changes also appeared in my branches i guess?
(Sorry, still learning...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "show all servers" and "analszer console" are really documented somewhere, something I wasn’t aware of changed (@gilgongo do you know anything?).
If you rebase your master branch, sure this will show up in the PR from master (but shouldn’t on this one).
My offer still stands: we can have a short call if you need it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the use of "show all servers" is mentioned as a way of debugging a problem. But "analyzerconsole" got added to the man page, because... the man page plays by its own rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -76,7 +76,7 @@ class CChannel : public QObject | |||
|
|||
void PrepAndSendPacket ( CHighPrioSocket* pSocket, const CVector<uint8_t>& vecbyNPacket, const int iNPacketLen ); | |||
|
|||
void ResetTimeOutCounter() { iConTimeOut = iConTimeOutStartVal; } | |||
void ResetTimeOutCounter() { iConTimeOut = bDisconnectAndDisable ? 1 : iConTimeOutStartVal; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do we use short hand if in the source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do we use short hand if in the source?
You mean conditional assignment ?
It is used at several places.
I use it whenever it makes the code more readable... (at least to me ;=))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use it whenever it makes the code more readable
I usually think it makes code less readable ;-). But in this case I think it's ok - but that depends on if this is used elsewhere too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's it's a simple "truth" test to pick between two values based on another value, it's okay, in general, I'd say. (I don't think Volker was keen on it.) Anything else (e.g. an expression getting evaluated as any part of the ?:
expression).
src/client.cpp
Outdated
{ | ||
if ( Channel.IsEnabled() ) | ||
{ | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented? Could you please remove debug code - if we want to merge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented? Could you please remove debug code - if we want to merge this?
What do you mean exactly ?
Most of the comment was already there, I adjusted to the new situation, but I see there is still some comment from stage 1, no longer applicable for stage 2
I will remove those.
And what "debug" code ? A check on failure of disconnect ? That's essential code to me....
@@ -1141,31 +1127,17 @@ void CClientDlg::OnTimerCheckAudioDeviceOk() | |||
// timeout to check if a valid device is selected and if we do not have | |||
// fundamental settings errors (in which case the GUI would only show that | |||
// it is trying to connect the server which does not help to solve the problem (#129)) | |||
if ( !pClient->IsCallbackEntered() ) | |||
if ( !pClient->SoundIsRunning() ) // ---> pgScorpio Was pClient->IsCallbackEntered() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( !pClient->SoundIsRunning() ) // ---> pgScorpio Was pClient->IsCallbackEntered() | |
if ( !pClient->SoundIsRunning() ) // Was pClient->IsCallbackEntered() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should put naming history into comments at all. That'll be rather something for the git history, IMO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should put naming history into comments at all. That'll be rather something for the git history, IMO?
ann0see already pointed me to that on. I will remove those.
// TODO is this still required??? | ||
// immediately update status bar | ||
OnTimerStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please briefly explain your ideas/changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please briefly explain your ideas/changes?
I like to use clear function names that are telling what they do...
The next line now calls UpdateSettingsAndChatButtons() which is now also used in OnTimerStatus() and at multiple other locations, instead of calling OnTimerStatus(), or repeating the same code, at multiple locations.
Since the name OnTimerStatus() tells exactly when it's called (Good name for a callback) this suggests that it's used for one thing only and does not give any clue it's used elsewhere too, so future changes to OnTimerStatus() could have unwanted side effects.
//============================================================================ | ||
// CMsgBoxes class: | ||
// Use this static class to show basic Error, Warning and Info messageboxes | ||
// For own created message boxes you should still use | ||
// CMsgBoxes::MainForm() and CMsgBoxes::MainFormName() | ||
//============================================================================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment syntax used throughout the code - except for ASIO? We should remain consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment syntax used throughout the code - except for ASIO? We should remain consistent.
AFAIK it was already used at some places (and also in many libraries), but various other styles are used too (so not very consistent anyway)...
Personally I find this the most readable style, especially when quick scrolling through a file. But maybe that is because this was the mandatory style I had to use for many, many years, so it became a habit ;=)
I can adapt if you want, but forgive me if I frequently fall back into those habits ;=)
@@ -78,6 +158,7 @@ int main ( int argc, char** argv ) | |||
#else | |||
bool bIsClient = true; | |||
#endif | |||
bool bSpecialOptions = false; // Any options after this option will be accepted ! (mostly used for debugging purpouses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's that exactly? Is it a new debugging feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's that exactly? Is it a new debugging feature?
Yes since we now can access the command line parameters anywhere in the code those are also very useful for debugging and testing purposes too. But since there is a check in main which would accept only valid, known parameters I introduced the "--special" option to allow any option for debugging and test purposes.
src/main.cpp
Outdated
@@ -127,14 +208,20 @@ int main ( int argc, char** argv ) | |||
// QT docu: argv()[0] is the program name, argv()[1] is the first | |||
// argument and argv()[argc()-1] is the last argument. | |||
// Start with first argument, therefore "i = 1" | |||
|
|||
// clang-format off | |||
// pgScorio TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comments should be avoided (best done) before a merge ;-)
@@ -148,7 +235,7 @@ int main ( int argc, char** argv ) | |||
// Common options: | |||
|
|||
// Initialization file ------------------------------------------------- | |||
if ( GetStringArgument ( argc, argv, i, "-i", "--inifile", strArgument ) ) | |||
if ( cmdLine.GetStringArgument ( i, CMDLN_INIFILE, strArgument ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmdLine sounds a bit strange and somewhat not consistent to the style - however, I don't know what the current style would want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmdLine sounds a bit strange and somewhat not consistent to the style - however, I don't know what the current style would want here.
Getting an argument from the Command line.. so cmdLine seams logical to me...
And consistent to what style ? Unfortunately I didn't find a consistent naming style in the code yet...
src/main.cpp
Outdated
// JSON-RPC port number ------------------------------------------------ | ||
if ( GetNumericArgument ( argc, argv, i, "--jsonrpcport", "--jsonrpcport", 0, 65535, rDbleArgument ) ) | ||
{ | ||
iJsonRpcPortNumber = static_cast<quint16> ( rDbleArgument ); | ||
qInfo() << qUtf8Printable ( QString ( "- JSON-RPC port number: %1" ).arg ( iJsonRpcPortNumber ) ); | ||
CommandLineOptions << "--jsonrpcport"; | ||
continue; | ||
} | ||
|
||
// JSON-RPC secret file name ------------------------------------------- | ||
if ( GetStringArgument ( argc, argv, i, "--jsonrpcsecretfile", "--jsonrpcsecretfile", strArgument ) ) | ||
{ | ||
strJsonRpcSecretFileName = strArgument; | ||
qInfo() << qUtf8Printable ( QString ( "- JSON-RPC secret file: %1" ).arg ( strJsonRpcSecretFileName ) ); | ||
CommandLineOptions << "--jsonrpcsecretfile"; | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did that get removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point !!
I have no idea how that happened !
@@ -556,14 +575,27 @@ int main ( int argc, char** argv ) | |||
continue; | |||
} | |||
|
|||
// Enable Special Options ---------------------------------------------- | |||
if ( cmdLine.GetFlagArgument ( i, CMDLN_SPECIAL ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking a bit more about the new style: I think it would still be good to see the options in the code like before. But I agree that the current way is not good since it leads to a lot of unneeded code.
Something like a function getting all arguments from the CLI and returning an array of enabled options (or similar) which we loop through to set the options would be another option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain further...
The code isn't that different from how it was, except for the defines for the short + long names. Is that what you have a problem with ?
Personally I think it makes the code a lot more readable, and it has another reason... since it is no longer necessary to pass command line parameters as variable true a lot of constructors, this could mean the long and short names could spread throughout the code and that didn't seem a good idea to me, so I use defines instead.
And I think there is hardly any more code, it's just in another place.
For my taste, that should be three (or more?) individual PRs, some with explicit Discussions/Issue ahead of time. |
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)
"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...
Yes those are from PR #2541 "Adding global messagebox and commandline parameter parsing classes." that was pushed from my master (Sorry again!!)
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. |
In this first stage I just renamed some functions in CClient to make the problem clear. See "---> pgScorpio:" comments in the problem area's. The next stage will actually change the code to solve this issue.
define the in headless non existing QDialog as void and send messages to the appropriate output stream
Co-Authored-By: pgScorpio <[email protected]>
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)
define the in headless non existing QDialog as void and send messages to the appropriate output stream
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)
This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>
@pgScorpio I've now opened #3364 Please continue with the other PR. |
This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>
This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>
This is a manual port of jamulussoftware#2550 by @pgScorpio Co-authored-by: ann0see <[email protected]>
Sorry, but this was years Ago! In the mean time, due to health issues and other priorities, I totally stopped programming (and even using Jamules). |
Ok. No problem. I hope the team will be able to pick up the changes. |
Best wishes for your health. |
Extracted from: jamulussoftware#2550 Related to: https://github.com/jamulussoftware/jamulus/pull/3364/files/21815c1a0708979fe0414c30e0294feaa7632be3#r1759185427 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
Extracted from: jamulussoftware#2550 Related to: https://github.com/jamulussoftware/jamulus/pull/3364/files/21815c1a0708979fe0414c30e0294feaa7632be3#r1759185427 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
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