Connect disconnect - #3813
Conversation
Adds two client JSON-RPC methods:
- jamulusclient/connect(directory, server): validates the server is (still)
listed under the given directory's last polled server list, then attempts
the connection, returning "ok" or a loose classification of why not
("Not Found", "Unauthorized", "Gone (server no longer listed)", "Upgrade
Required (obsolete protocol, upgrade Jamulus)", "Insufficient Storage
(Full)").
- jamulusclient/disconnect(directory, server): tears down the connection
established by jamulusclient/connect, validating the params identify the
current connection.
Also includes jamulusclient/getDirectories, needed by jamulusclient/connect's
callers to discover directory socket addresses in the first place.
(jamulusclient/getCurrentDirectory and jamulusclient/setCurrentDirectory are
left out of this branch for a separate, focused review.)
Both connect/disconnect share their core logic with the desktop UI's
Connect/Disconnect button instead of duplicating it:
- CClient::ConnectToServer() is a new method used by both CClientDlg::Connect
and jamulusclient/connect to attempt a connection and classify the outcome.
- CClient::Stop() now reliably emits Disconnected() regardless of caller, and
CClientDlg::OnDisconnected()/OnClientIDReceived() (rather than Connect()/
Disconnect() themselves) own the GUI sync, so the desktop UI stays correct
whether the connection was started/stopped via the UI or via the API.
- CClient::GetServerAddress() and GetMyChannelID() are new small accessors
needed to let the UI show a sensible default server name/channel ID
regardless of which path connected. GetMyChannelID() also fills in
jamulusclient/getChannelInfo's result.id, previously a TODO because
CChannelCoreInfo lacks that field.
|
But yes, your approach probably is also worth combining into his codebase but this definitely needs discussion |
|
Please use |
|
As I said in #3816 (comment) (this is a duplicate): Well I was taking the easy way out: When I see Jamulus main updated, I press the "Sync fork" button in Github - for all the branches that contain the PRs. However, Github uses merge and not rebase when I do that (https://github.com/orgs/community/discussions/48935#discussioncomment-14261151). There are instructions on how to rebase via the command line in that post - manually via the CLI. There's also https://kb.mautic.org/article/how-to-rebase-branch-and-sync-forked-repository-with-upstream-using-git.html I haven't actually modified my PR code. One reason I do this 'merge' is to keep my PRs up to date and reduce any conflicts when it's brought in to Jamulus main branch. However, the primary reason to merge-now-rebase is to test the latest code with my changes built in on my fork to see if it things still works. |
3 participants
Add new JSON-RPC API methods and channel info attribute. This includes re-factoring to share the connect and disconnect processing between the Jamulus client and the JSON-RPC API calls.
CHANGELOG: new connect/disconnect JSON-RPC API methods; add id attribute to getChannelInfo result
Context: Fixes an issue?
This is take 2 of an original submission of a closed issue, #3783. Reasons for this re-submission:
An attempt was made to fix the original PR submission issues, and link to that work was provided there. But as commented on that PR "I am a bit concerned about the message "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." that is displayed in that commit. I had not seen that before, and while I know it's not nefarious, the results from a search for that string suggested it (looks like it really is missing an original source for that commit?)". So there is confusion about whether that work actually still part of the PR pipeline, especially since that issue is now closed. I also took this opportunity to resolve the submission issues that caused this extra work to be done, so this one should be clean.
Does this change need documentation? What needs to be documented and how?
The documentation JSON-RPC.md has been updated (as you know, there's a tool for automating this).
Status of this Pull Request
What is missing until this pull request can be merged?
Checklist