-
Notifications
You must be signed in to change notification settings - Fork 222
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
JSON-RPC - Extend jamulusserver/getClients to include additional client attributes #2488
Comments
FYI; My goal of not touching Now that that PR is merged, I think future work can change the |
@dtinth - yes, I tried to say “initial RPC” to make that point. However, not reaching into those files was cited as the early reason why the functionality from my UDP PR could not (would not) be addressed as part of the initial JSON-RPC merge. I completely get that — but that fact is the basis and justification for this request so I thought it was worth pointing out. |
Is serverdlg.cpp also available on headless? |
Changing server.cpp can affect serverdlg.cpp when it's not headless. |
So this would only work in GUI Mode? I thought the main point or at least one of the main points of JSON-RPC is to obtain more information on headless servers |
What makes you think that? |
I haven’t looked into serverdlg.cpp but the name implies that it’s somehow related to the UI. Probably it makes sense to try how the headless server behaves. |
I think there's confusion about which uses what. @Rob-NY proposed to change a method which was previously used by the UI only. JSON-RPC started using it as well. If we extend it, both callers (JSON-RPC and UI) have to be adjusted. That does not mean that JSON-RPC becomes dependent on the UI. I think what @Rob-NY suggested is a perfectly valid approach. This is not about introducing method UI method calls from JSON-RPC. |
Exactly. (And none of the above requires looking at any code. It's a general principle and doesn't just apply in Computer Science.) |
What is the current behaviour and why should it be changed?
The current getClients RPC method does not return skill level, country, city, or instrument for connected clients. In order to close the gap between desired functionality in (now closed) PR #2006, getClients can be extended to provide these additional properties.
Describe possible approaches
@dtinth had as specific goal of not touching server.cpp when initially authoring JSON-RPC. This limited the information available to the RPC interface. The most direct solution I see now is to modify GetConCliParam() to add an additional vector that can be populated with Channel Information (core). GetConCliParam() is already used by JSON-RPC within the getClients method.
Changing this method impacts server.cpp/h and serverdlg.cpp only. While serverdlg would not make use of this additional information, having connection attributes such as country, city, instrument, and skill would allow the serverdlg to make use of this information in the future within the UI's "connections" widget.
The modified GetConCliParam method proposed is:
The new channel attributes would then be added to the getClients JSON-RPC method:
The proposal here returns the enumerated values of some attributes to be consistent with the current approach. Issue #2468 proposes to change this. The solution proposed here could be refactored if that issue moves forward.
Has this feature been discussed and generally agreed?
No
The text was updated successfully, but these errors were encountered: