Add new JSON-RPC API calls, with documentation#3660
Add new JSON-RPC API calls, with documentation#3660digable1 wants to merge 3 commits intojamulussoftware:mainfrom
Conversation
|
Please add a note if this was coded with Claude (in this PR) |
|
line re-formatted, should now satisfy clang-format (surprised that C style doesn't like splitting lines to avoid left-right scroll, but ok - so used to Java/Javascript/Typescript styles where that is either encouraged or at least not flagged) Also added the callout to Claude (as stated in the PR, because I used Claude Pro there is no copyright/ownership issue - Anthropic explicitly calls that out). |
|
Please push the changes to this PR. It doesn't seem to be pushed. |
|
Take 2 (yeah I'm supposed to know what I'm doing - whoops!). |
|
Is there already a "set current directory" call? Without one, I'm not sure of the purpose here. Please clarify how this meets the "keep it simple and small" principle. |
|
The purpose is to obtain ping time for that server. This means I do need to know what the current directory is so I can monitor serverInfoReceived and match the address to get it. I don't need to play puppet to the Jamulus UI by being able to set the current directory (I personally don't have a use case for it, and couldn't image anybody else needing it either). And not considering setters avoids error handling for parameter validation. |
|
It needs more explanation. Set it in context. Again, you can write in PHP and use the protocol directly to get the ping time -- the source is available and in use in several tools. Why do you need to have an actual Jamulus Client available? |
|
I have a client-adjenct support tool in development. For this musician's interaction in a performance/rehearsal. Not within the context of 'general information'. So in order to do this direct from the server, the musician would have to duplicate their selection twice - once in the Jamulus client and once again in this support tool. And that would legitimately be a head-scratch as a musician would be correct in asking 'why', missing a critical 'this is what I already told Jamulus what I want' aspect the musician already makes with the client. |
|
More context on why I want to use the client with the 'this is what I already told Jamulus what I want' approach: My client-adjacent tool might be at a stage where it won't break after 10 seconds of trying to use it (it might take 15 seconds). As a result, I'm nervous while slightly comfortable with exposing it here to provide some insight as to why I want to easily get the ping time from Jamulus (it's FAR from release): https://sync-track.com/synctrack/ This work's got two purposes: There's a user manual button within the app, which might help. I wrote ~5 lines of code and a paragraph on the English language version of the manual - the rest done by Claude with a lot of watching what it does (much more needs to be done - it's very brittle, especially once you get away from happy path). Right now, Chrome-only (chromium counts) - Firefox has issues. So I want to automate the ping time value from the jamulus client (right now, it's a manual-entry field - there is a Chrome extension that translates from raw TCP connections (JSON-RPC) to HTTP connections to make this possible, and it's currently very clunky without these new API calls). |
|
Hm. That raises an interesting point. My aim is that CClient "owns" client capabilities, whatever they are (even if it has them in instances of other classes it creates for itself). Currently we're in a situation where quite a lot of client capability is scattered around in the UI classes - like knowing if a mixer channel is muted and what the "current" directory is. Ideally, if you're using JSON-RPC, you shouldn't need the GUI to know the chosen current directory - you should be able to off-load the whole of the connection dialogue to a JSON-RPC service controlling the client. Sadly not possible right now. If you fancy taking that on, too... 😄 |
| /// @brief Returns the currently selected directory socket address. | ||
| /// @param {object} params - No parameters (empty object). | ||
| /// @result {string} result - The socket address of the current directory, usable as params.directory in jamulusclient/pollServerList. | ||
| pRpcServer->HandleMethod ( "jamulusclient/getCurrentDirectory", [=] ( const QJsonObject& params, QJsonObject& response ) { |
There was a problem hiding this comment.
Go to Settings -> Advanced. The data item you're collecting here is the "current" entry in the custom directories list, I think.
Not Connect->Directory.
There was a problem hiding this comment.
So again, ideally:
- CClient would own "current directory index" which was an index into the list it returned when asked to "get the user's current directories"
- CConnectDlg would emit a "current directory index changed" signal when the selection changed
- CClient would handle that, storing the new value in CClientSettings
- CClientSettings would emit its own "current directory index changed" signal
- CConnectDlg would handle that by retrieving ensuring the dropdown was displayed correctly, then retrieving the new server list from the appropriate entry from "get the user's current directories"
I think there are JSON-RPC calls that can handle emitted signals by emitting new state, aren't there? They not just responses?
- So CClientRpc would handle the CClientSettings "current directory index changed" signal by sending the new address.
This also begs for a JSON-RPC "setCurrentDirectory", of course, to act the same as selecting a new entry in the dropdown.
| /// @param {object} params - No parameters (empty object). | ||
| /// @result {array} result - Array of directory socket address strings, usable as params.directory in jamulusclient/pollServerList. | ||
| pRpcServer->HandleMethod ( "jamulusclient/getDirectories", [=] ( const QJsonObject& params, QJsonObject& response ) { | ||
| QJsonArray arrDirectories; |
There was a problem hiding this comment.
Ideally this would be simplified:
- CClient would own "get the user's current directories"
- CConnectDlg would put that in the Directory drop down
- CClientRpc would return that here
Otherwise we're duplicating functionality for no benefit.
|
Yup - you're really going for an n-tiered architecture if I read this correctly. Where the "source of truth" for all data is encapsulated in the 'API engine'. And the Jamulus UI is really one of the clients, implemented in Qt with calls to the API based on user interactions (and my tool would be another client that doesn't need much - just ping times in the currently selected directory and server). That would definitely be ideal - would need to profile to ensure we didn't add too much overhead. And I noticed that wasn't where we are. But I am uncomfortable with this kind of re-factor right now being I really just don't have enough context at the present. For example, some of the choices might have been made for performance reasons to avoid the overhead of going through an API pattern, I really didn't now. And one can really make things worse if you don't know what you're doing - and I don't. At least in my case, they were tiny tweaks that didn't touch the core functionality of the client. Regarding "I think there are JSON-RPC calls that can handle emitted signals by emitting new state, aren't there? They not just responses?": Yup. And I have a listener consuming those changes as they are emitted, as that appears to be the only place where the ping time is located on the client. And your ideal is indeed close id nor on totally on target: This is not something for somebody just jumping in on first attempt - yet (lots of dialog and bigger picture to obtain first - I'm just poking at an edge to solve an immediate problem, and indeed potentially contributing to spaghetti code). If this really would be me, I'd be very annoying with the types and volume of questions being asked. |
Short description of changes
Adds two JSON-RPC API calls: jamulusclient/getDirectories and jamulusclient/getCurrentDirectory.
jamulusclient/getDirectories: Gets the current list of directories.
jamulusclient/getCurrentDirectory: Gets the current directory the client is referencing.
Does this change need documentation? What needs to be documented and how?
JSON-RPC.md has been updated to include documentation for these new API calls
Status of this Pull Request
What is missing until this pull request can be merged?
Nothing
Has been smoke tested to validate we're returning what we think we are.
AUTOBUILD: Please build all targets
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
(100% done under Claude Pro - which means Anthropic explicitly states that no ownership/control is asserted)