-
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
Refactor Connect and Disconnect functionality out to CClient #3372
base: main
Are you sure you want to change the base?
Refactor Connect and Disconnect functionality out to CClient #3372
Conversation
src/client.cpp
Outdated
if ( !IsRunning() ) | ||
{ | ||
// Set server address and connect if valid address was supplied | ||
if ( SetServerAddr ( strServerAddress ) ) |
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 like this style at all. If there's some validation to happen, it should have happened before we get this far. We shouldn't try to connect to an address that's not invalid.
Perhaps this whole routine is redundant and SetServerAddr
should be called Connect
and do the emits?
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.
SetServerAddr
Should just do exactly what the name implies IMO - Otherwise we could introduce mixing of functionality.
Connect() would then emit the signals. That's cleaner and closer to what we do now.
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 think that nevertheless Connect(serverAddress) should exist. It simplifies needing to set and then start the client.
@@ -1193,65 +1193,36 @@ void CClientDlg::OnCLPingTimeWithNumClientsReceived ( CHostAddress InetAddr, int | |||
ConnectDlg.SetPingTimeAndNumClientsResult ( InetAddr, iPingTime, iNumClients ); | |||
} | |||
|
|||
void CClientDlg::Connect ( const QString& strSelectedAddress, const QString& strMixerBoardLabel ) | |||
void CClientDlg::OnConnect ( const QString& strMixerBoardLabel ) |
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.
Ideally, this signal would get issued only on successful connection.
As we're going cross-thread here, we need Client to supply any listener -- ClientDlg or the Client JSON-RPC -- with full details of the newly established connection.
We should follow this rule for all state changes in the Client -- that should be the only way for ClientDlg and the Client JSON-RPC to acquire knowledge of Client state, no calls to functions across threads.
(The same is true on the server side, too.)
Similarly, to change the state of the Client, the Client should expose slots and ClientDlg or Client JSON-RPC should signal those slots to effect the change in state. Not call methods across threads.
This does demand a major restructure but I think this could be a good place to start. Set out what state changes when the Client successfully establishes a connection to a server, create an object instance containing that information, and issue a signal passing that object. This gives a clearly defined API to that change in state.
Either the message on the signal could be minimal, covering only the state that changes on any connect, or extensive, supplying the whole Client state. In the latter case, every signal from the Client would pass a new state instance with the full current Client state, making the listener's job much easier. (I don't like it as much as the minimal approach but it's likely much easier to explain and maintain. It smacks of "global variables" a bit too much, though...)
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.
The global approach could also be wasteful while the small approach could be more difficult. If we restructure correctly, we should aim at the small approach.
|
8bacce4
to
4ec9c68
Compare
4ec9c68
to
a1d98aa
Compare
69246f5
to
57825bf
Compare
57825bf
to
8bc81d7
Compare
8bc81d7
to
0f9a605
Compare
0f9a605
to
596e42b
Compare
596e42b
to
65622ee
Compare
Now the coding style check seems to work. Not so sure about the style change in main.cpp: |
65622ee
to
1957532
Compare
e27569f
to
ae31a65
Compare
Please recheck if the important parts are in the two issues I opened and comment there. |
8b0b44d
to
8e48c03
Compare
{ | ||
throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) ); | ||
} | ||
} |
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.
Should this throw some indication it ignored the request?
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 emits the error message further down
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 do not understand why clang format complains about the list style here. |
This is an extract from jamulussoftware#2550 Co-authored-by: ann0see <[email protected]>
8e48c03
to
9f93619
Compare
This is an extract from #2550
Short description of changes
Refactor Connect() and disconnect functionality.
Introduces a new Connect() method for quick connection and adds signals to Start() and Stop() of client.
CHANGELOG: Refactoring: Move Connect() functionality to CClient.
Context: Fixes an issue?
Fixes: #3367
Does this change need documentation? What needs to be documented and how?
No
Status of this Pull Request
Ready for review. Start for refactoring. Most likely overlaps and to a large extent relates somehow to #3364
What is missing until this pull request can be merged?
Review, clarification, probably documentation and refactoring.
Checklist